Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce libnixflake #9063

Merged
merged 3 commits into from
Jun 27, 2024
Merged

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Sep 29, 2023

Motivation

Flakes is a separate layer, so it should have its own library!

Context

See each commit for details. The most interesting code changes (that aren't just moving things around) happen in the first commit.

Priorities

Add 👍 to pull requests you find important.

@github-actions github-actions bot added new-cli Relating to the "nix" command fetching Networking with the outside (non-Nix) world, input locking labels Sep 29, 2023
@Ericson2314 Ericson2314 changed the title Fix a few things in libfetchers to prepare for stabilizing buitlins.fetchTree Tweaj a few things in libfetchers to prepare for stabilizing buitlins.fetchTree Sep 29, 2023
@Ericson2314 Ericson2314 changed the title Tweaj a few things in libfetchers to prepare for stabilizing buitlins.fetchTree Tweak a few things with libfetchers to prepare for stabilizing buitlins.fetchTree Sep 29, 2023
@Ericson2314 Ericson2314 changed the title Tweak a few things with libfetchers to prepare for stabilizing buitlins.fetchTree Tweak a few things with libfetchers to prepare for stabilizing builtins.fetchTree Sep 29, 2023
@edolstra
Copy link
Member

Please keep registry.* and indirect.* in libfetchers, since they're fetchers and not flake-specific (e.g. you can do fetchTree to fetch a repo looked up via the registry). This PR creates a libflake that isn't really about flakes at all but about the indirect fetcher, which IMHO belongs in libfetchers.

I prefer to keep all URL/flakeref-related regexes like flakeIdRegexS in one place (url-parts.hh) rather than scattered all over.

@Ericson2314
Copy link
Member Author

Ericson2314 commented Sep 29, 2023

 $ git grep flake src/libfetchers/
src/libfetchers/fetch-settings.hh:          Example `~/code/flake.nix`:
src/libfetchers/fetch-settings.hh:    Setting<std::string> flakeRegistry{this, "https://channels.nixos.org/flake-registry.json", "flake-registry",
src/libfetchers/fetch-settings.hh:          Path or URI of the global flake registry.
src/libfetchers/fetch-settings.hh:          When empty, disables the global flake registry.
src/libfetchers/fetch-settings.hh:        "Whether to use flake registries to resolve flake references.",
src/libfetchers/fetch-settings.hh:    Setting<bool> acceptFlakeConfig{this, false, "accept-flake-config",
src/libfetchers/fetch-settings.hh:        "Whether to accept nix configuration from a flake without prompting.",
src/libfetchers/fetch-settings.hh:          The commit summary to use when committing changed flake lock files. If
src/libfetchers/fetchers.cc:            // usages (e.g. `builtins.fetchTree` calls or flake inputs).
src/libfetchers/indirect.cc:std::regex flakeRegex("[a-zA-Z][a-zA-Z0-9_-]*", std::regex::ECMAScript);
src/libfetchers/indirect.cc:        if (url.scheme != "flake") return {};
src/libfetchers/indirect.cc:                throw BadURL("in flake URL '%s', '%s' is not a commit hash or branch/tag name", url.url, path[1]);
src/libfetchers/indirect.cc:                throw BadURL("in flake URL '%s', '%s' is not a branch/tag name", url.url, path[1]);
src/libfetchers/indirect.cc:                throw BadURL("in flake URL '%s', '%s' is not a commit hash", url.url, path[2]);
src/libfetchers/indirect.cc:        if (!std::regex_match(id, flakeRegex))
src/libfetchers/indirect.cc:            throw BadURL("'%s' is not a valid flake ID", id);
src/libfetchers/indirect.cc:        if (!std::regex_match(id, flakeRegex))
src/libfetchers/indirect.cc:            throw BadURL("'%s' is not a valid flake ID", id);
src/libfetchers/indirect.cc:        url.scheme = "flake";
src/libfetchers/registry.cc:            for (auto & i : json["flakes"]) {
src/libfetchers/registry.cc:            throw Error("flake registry '%s' has unsupported version %d", path, version);
src/libfetchers/registry.cc:        warn("cannot parse flake registry '%s': %s", path, e.what());
src/libfetchers/registry.cc:        warn("cannot read flake registry '%s': %s", path, e.what());
src/libfetchers/registry.cc:    json["flakes"] = std::move(arr);
src/libfetchers/registry.cc:        auto path = fetchSettings.flakeRegistry.get();
src/libfetchers/registry.cc:            auto storePath = downloadFile(store, path, "flake-registry.json", false).storePath;
src/libfetchers/registry.cc:                store2->addPermRoot(storePath, getCacheDir() + "/nix/flake-registry.json");
src/libfetchers/registry.cc:    if (n > 100) throw Error("cycle detected in flake registry for '%s'", input.to_string());
src/libfetchers/registry.cc:        throw Error("cannot find flake '%s' in the flake registries", input.to_string());
src/libfetchers/tarball.cc:            // FIXME: would be nice to support arbitrary flakerefs
src/libfetchers/tarball.cc:            // here, e.g. git flakes.

if we can cut this down I am fine leaving those files in libfetchers, but we need to actually make an attempt to prove to the user that the feature is not intrinsically part of flakes. Right now I would say it is an "easter egg" the that the Flake Registry is not actually about Flakes at all, but "easter egg" simply isn't good enough; it has to be de jure.

For example, if it isn't flakes-specific we shouldn't call it the Flakes Registry at all.

@Ericson2314
Copy link
Member Author

#9085 My crude attempt to make indirect/registry demonstrably Flake-agnostic by renaming things. I will need some help finishing that PR however.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-09-29-nix-team-meeting-minutes-90/33774/1

@roberth
Copy link
Member

roberth commented Oct 5, 2023

we shouldn't call it the Flakes Registry

This frees up the name for registries in the traditional sense that contain artifacts of sorts.

Still not happy that we overload the term "registry" so I think it's worth taking the opportunity to consider an entirely different name.

The indirect scheme is closer to something like DNS than to a registry.

How about source index?

EDIT: I'm aware that DNS has a prominent concept of registry, but our domain is software development and packaging, not internet infrastructure, so "registry" is more easily assumed to refer to package or artifact registries. Also DNS forms an abstraction where the registry is largely irrelevant to its users.

@Ericson2314 Ericson2314 marked this pull request as draft October 5, 2023 14:33
@Ericson2314 Ericson2314 changed the title Tweak a few things with libfetchers to prepare for stabilizing builtins.fetchTree Introduce libnixflake Oct 5, 2023
@Ericson2314
Copy link
Member Author

Ericson2314 commented Oct 5, 2023

@roberth I made #9085 for the approach of renaming it if we will keep it in libfetchers. I think your comment might go better there. I totally agree "registry" is not a good name. (I did fetcher-registry as a simple stupid sed-like de-flaking, but I agree it is not a good name at all.)

(I still think we should have a libnixflake, but it would have different contents given that plan. I just renamed this and marked this draft with the intention of basically rewriting it assuming that goes through. Sorry I didn't do this earlier.)

@Ericson2314 Ericson2314 force-pushed the libfetchers-prep branch 2 times, most recently from 7bf55c3 to cc2a434 Compare June 25, 2024 13:44
@Ericson2314 Ericson2314 marked this pull request as ready for review June 25, 2024 13:45
@Ericson2314
Copy link
Member Author

(The registry stuff is kept in libfetchers now)

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surprisingly short diff. I like it.
Just a few comments, nothing major.

src/libexpr/eval.cc Outdated Show resolved Hide resolved
src/libflake/flake-settings.hh Outdated Show resolved Hide resolved
src/libexpr/eval-settings.hh Show resolved Hide resolved
src/libexpr/eval-settings.hh Show resolved Hide resolved
@fricklerhandwerk fricklerhandwerk added the RFC Related to an accepted RFC label Jun 26, 2024
@fricklerhandwerk
Copy link
Contributor

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-06-26-nix-team-meeting-minutes-156/47740/1

Ericson2314 and others added 3 commits June 26, 2024 19:56
Co-authored-by: Robert Hensing <[email protected]>
Factored out code is now elegible for formatting.
@Ericson2314 Ericson2314 merged commit 7e66f0d into NixOS:master Jun 27, 2024
11 checks passed
@Ericson2314 Ericson2314 deleted the libfetchers-prep branch June 27, 2024 00:38
@cole-h
Copy link
Member

cole-h commented Jun 28, 2024

Looks like this accidentally reverted part of #10691 (commit-lock-file-summary is gone). #10988 fixes that.

cole-h added a commit to cole-h/nix that referenced this pull request Jun 28, 2024
It was originally renamed in NixOS#10691,
but NixOS#9063 accidentally removed the new
name and alias.
@Ericson2314
Copy link
Member Author

Sorry! A negligent rebase conflict resolution on my part, no doubt. Thank you @cole-h for fixing my mistake ❤️.

@roberth
Copy link
Member

roberth commented Jun 28, 2024

No need to take all the blame; this feature wasn't tested.

@cole-h
Copy link
Member

cole-h commented Jun 28, 2024

Implication heard loud and clear -- I'll add a test in my PR resurrecting it.

EDIT: test added

@Ericson2314
Copy link
Member Author

Ericson2314 commented Jun 28, 2024

Tests are def good, and objectively the right way to prevent this, but I also pride myself on being a slicing-and-dicing-like-a-sushi-chef rebaser :), so I really was dissapointed I slipped up.

cole-h added a commit to cole-h/nix that referenced this pull request Jun 28, 2024
It was originally renamed in NixOS#10691,
but NixOS#9063 accidentally removed the new
name and alias.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation fetching Networking with the outside (non-Nix) world, input locking new-cli Relating to the "nix" command RFC Related to an accepted RFC
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants