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

builtins.getFlake: Allow inputs to be overridden #11952

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

edolstra
Copy link
Member

Motivation

Fixes #9154.

This uses the same syntax as flake inputs in flake.nix, e.g.

builtins.getFlake {
  url = "github:NixOS/nix/55bc52401966fbffa525c574c14f67b00bc4fb3a";
  inputs.nixpkgs.url = "github:NixOS/nixpkgs/c69a9bffbecde46b4b939465422ddc59493d3e4d";
}

Note that currently it's not supported to set flake = false or to
use follows (because lockFlake() doesn't support that for CLI
overrides), but it could be implemented in the future.

Context


Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

This uses the same syntax as flake inputs in flake.nix, e.g.

  builtins.getFlake {
    url = "github:NixOS/nix/55bc52401966fbffa525c574c14f67b00bc4fb3a";
    inputs.nixpkgs.url = "github:NixOS/nixpkgs/c69a9bffbecde46b4b939465422ddc59493d3e4d";
  }

Note that currently it's not supported to set `flake = false` or to
use `follows` (because lockFlake() doesn't support that for CLI
overrides), but it could be implemented in the future.

Fixes NixOS#9154.
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Nov 25, 2024
Comment on lines +853 to +857
std::function<void(const InputPath & inputPath, const FlakeInput & input)> recurse;

recurse = [&](const InputPath & inputPath, const FlakeInput & input)
{
if (!input.ref)
Copy link
Contributor

@xokdvium xokdvium Nov 25, 2024

Choose a reason for hiding this comment

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

It's not strictly necessary to use std::function and its inherent indirection for recursive lambdas. Though in this case this shouldn't matter much. With C++23 it's possible to use "deducing this" https://en.cppreference.com/w/cpp/language/member_functions#Explicit_object_member_functions. For pre C++23 one can use the following hack:

auto recurse_impl = [&](auto& self, const InputPath & inputPath, const FlakeInput & input) {
....
  recurse_impl(self, ....);
};

auto recurse = [&](const InputPath & inputPath, const FlakeInput & input) {
  recurse_impl(recurse_impl, inputPath, input);
};

This avoids the virtual call via std::function

Copy link
Member Author

@edolstra edolstra Nov 26, 2024

Choose a reason for hiding this comment

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

Wow nice, didn't know about "deducing this". Apparently it will be available in gcc 14.

@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-11-27-nix-team-meeting-minutes-198/56691/1

@edolstra edolstra changed the title builtins.getFlake: Allow inputs to overridden builtins.getFlake: Allow inputs to be overridden Nov 28, 2024
@edolstra
Copy link
Member Author

edolstra commented Dec 9, 2024

Added a release note.

flakeRef;
})
: ({
auto flakeInput = parseFlakeInput(state, std::nullopt, args[0], pos, {}, {});
Copy link
Member

Choose a reason for hiding this comment

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

parseFlakeInput expects a "trivial" value, but this is not a reasonable expectation for args[0], which is part of the "normal" expression language where evaluation just happens.
(e.g, url = "git+https://" + foo; will fail)

Possible solutions:

  • deepSeq
    • gets the job done if all is ok, but is too strict: worse error message for errors in invalid (misplaced) attributes.
  • force the relevant values here
    • code gets out of sync with parseFlakeInput, and bugs won't be found immediately, because literals and trivial values are common
  • add a flag to parseFlakeInput to allow it to evaluate when true
    • don't see much of a problem with this

My preference is with the parseFlakeInput flag.

Such a flag would perhaps also be useful for a feature where an "evaluated" flake can be passed as an input, e.g. getFlake { url = ./tests; inputs.foo.value = self; }, although this has also been requested as a feature for static flake inputs:

@@ -99,7 +99,7 @@ static std::map<FlakeId, FlakeInput> parseFlakeInputs(
const std::optional<Path> & baseDir, InputPath lockRootPath);

static FlakeInput parseFlakeInput(EvalState & state,
std::string_view inputName, Value * value, const PosIdx pos,
std::optional<std::string_view> inputName, Value * value, const PosIdx pos,
Copy link
Member

Choose a reason for hiding this comment

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

Could inputName be removed? An input shouldn't have to know its name.

I think this might be an issue with the data model, but it seems that we have low hanging fruit, in that we could turn the call with inputName into a separate overload that calls parseFlakeInput(state,value,pos) and postprocesses the return value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation with-tests Issues related to testing. PRs with tests have some priority
Projects
Status: 🏁 Review
Development

Successfully merging this pull request may close these issues.

Override inputs when using builtins.getFlake
5 participants