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

check-meta.nix: Fix message for nix build users as well #101367

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

@doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Oct 22, 2020

As discussed in:
https://discourse.nixos.org/t/why-isnt-config-nixpkgs-config-nix-evaluated-for-nix-build/9579

It's a bit nontrivial that nix build needs the --impure flag in
order to evaluate ~/.config/nixpkgs/config.nix.

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Oct 22, 2020

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

https://discourse.nixos.org/t/why-isnt-config-nixpkgs-config-nix-evaluated-for-nix-build/9579/4

doronbehar added a commit to doronbehar/nixpkgs that referenced this issue Oct 22, 2020
Fixes NixOS#101367 - Since KDE's maintainers don't allow non official kde
applications under the `kdeApplications` path, many packages that depend
on KDE libraries and applications are defined in all-packages.nix.
Overriding all kdeApplications to use qt514 while all other
`libsForQt5.callPackage` in all-packages.nix are using qt515, causes
collisions.
@NobbZ
Copy link
Contributor

@NobbZ NobbZ commented Oct 22, 2020

Isn't this more about flake than unstable?

As for me it works using non-flake syntax, though fails when using flake syntax:

$ NIXPKGS_ALLOW_UNFREE=1 nix build -f '<nixpkgs>' steam
# builds successfully
$ NIXPKGS_ALLOW_UNFREE=1 nix build nixpkgs#steam
# does not
$ NIXPKGS_ALLOW_UNFREE=1 nix build --impure nixpkgs#steam
# does build again

@xfix
Copy link
Contributor

@xfix xfix commented Oct 22, 2020

I don't think nixpkgs should point flakes users to --impure as this breaks caching. It is convenient for sure, but it isn't a proper solution.

Instead you may consider defining a flake that is nixpkgs which is configured to allow unfree packages. This isn't ideal, but well, there is a good reason why nixUnstable is in fact unstable :).

{
  inputs.nixpkgs.url = "https://nixos.org/channels/nixos-20.09/nixexprs.tar.xz";
  outputs = { self, nixpkgs }: {
    defaultPackage.x86_64-linux = import nixpkgs {
      system = "x86_64-linux";
      config.allowUnfree = true;
    };
  };
}

It's also possible to avoid flakes entirely which avoids the issue.

@doronbehar
Copy link
Contributor Author

@doronbehar doronbehar commented Oct 22, 2020

{
  inputs.nixpkgs.url = "https://nixos.org/channels/nixos-20.09/nixexprs.tar.xz";
  outputs = { self, nixpkgs }: {
    defaultPackage.x86_64-linux = import nixpkgs {
      system = "x86_64-linux";
      config.allowUnfree = true;
    };
  };
}

This is indeed worth knowing, but I think it's too much information to put on a trace message like here. However, regarding:

I don't think nixpkgs should point flakes users to --impure as this breaks caching. It is convenient for sure, but it isn't a proper solution.

It depends on what you define as "proper". The use case I had in mind, (which was not specific enough indeed) is testing updates via e.g nix build .#zoom-us or nix profile install nixpkgs#zoom-us. I've adjusted the trace message according to your recommendation - it recommends not using --impure and it mentions it's an issue specific to # flakes syntax.

@doronbehar doronbehar force-pushed the stdenv/impure-nixUnstable-checkMeta branch from f0a22dc to c9024ac Oct 22, 2020
@xfix
Copy link
Contributor

@xfix xfix commented Oct 22, 2020

It depends on what you define as "proper". The use case I had in mind, (which was not specific enough indeed) is testing updates via e.g nix build .#zoom-us or nix profile install nixpkgs#zoom-us. I've adjusted the trace message according to your recommendation - it recommends not using --impure and it mentions it's an issue specific to # flakes syntax.

You can easily avoid flakes by using nix build -f . zoom-us instead. Flakes are experimental and I would say they should be avoided for now.

@doronbehar
Copy link
Contributor Author

@doronbehar doronbehar commented Oct 22, 2020

You can easily avoid flakes by using nix build -f . zoom-us instead. Flakes are experimental and I would say they should be avoided for now.

Hmm I see, but what about nix profile? It won't accept -f. so it seems.

Just because something is experimental doesn't mean we can't at least explain better a tiny detail to users of this experimental feature. We've had contributions before to nixos-rebuild which included flakes support, and this is what helps adopting such an experimental feature.

Besides, what is the worst thing that could happen? If at some point the flakes syntax would change, we'd only have to slightly update that message, and the price is 0 rebuilds, just as it is today. Please note @xfix that nothing in the new message's text recommends using flakes.

@xfix
Copy link
Contributor

@xfix xfix commented Oct 23, 2020

Providing instructions for flakes makes the error message more noisy for users who don't use flakes.

However, I think an improvement is still possible. It's possible to recognize whether an user is using flakes or not and provide a different message depending on that.

That being said, I think there should be a way to pass configuration options into Nix flakes, it's just that it wasn't implemented yet. Worth noting that this isn't --impure however, as --impure does way too much.

@doronbehar
Copy link
Contributor Author

@doronbehar doronbehar commented Oct 23, 2020

However, I think an improvement is still possible. It's possible to recognize whether an user is using flakes or not and provide a different message depending on that.

Indeed that would be ideal, though I'm not sure how would I do that - should it be a check of the current nix.package.version? Is that possible in the scope of that meta?

As discussed in:
https://discourse.nixos.org/t/why-isnt-config-nixpkgs-config-nix-evaluated-for-nix-build/9579

It's a bit nontrivial that `nix build` needs the `--impure` flag in
order to evaluate ~/.config/nixpkgs/config.nix.
@doronbehar doronbehar force-pushed the stdenv/impure-nixUnstable-checkMeta branch from c9024ac to e8b62f4 Dec 9, 2020
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Dec 9, 2020

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

https://discourse.nixos.org/t/using-non-free-libraries-in-flakes/8632/15

@rjpcasalino
Copy link

@rjpcasalino rjpcasalino commented May 20, 2021

@doronbehar sorry for bumping something from over a year ago but I managed to find my way here mostly because of your comments on discourse (in the above mentioned and others). Are there any updates on this? I know things might be a bit wild in your neck of the woods right about now.

@doronbehar
Copy link
Contributor Author

@doronbehar doronbehar commented May 20, 2021

@doronbehar sorry for bumping something from over a year ago but I managed to find my way here mostly because of your comments on discourse (in the above mentioned and others). Are there any updates on this?

Not really, the last suggestion by @xfix was to make that eval message different for nixUnstable and nixStable users, but (now) I think it's not possible to do it - nixpkgs on it's own doesn't know what nix version is used. Hence my opinion is that to give "unstable" nix more respect, this PR is one of the things we could improve.

I know things might be a bit wild in your neck of the woods right about now.

Thanks a lot for your concern ☺️. It's not such a big deal in the area where I live in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants