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

nixpkgs tarball: using ag alias breaks eval-release.nix check #44299

Closed
xeji opened this issue Aug 1, 2018 · 8 comments
Closed

nixpkgs tarball: using ag alias breaks eval-release.nix check #44299

xeji opened this issue Aug 1, 2018 · 8 comments

Comments

@xeji
Copy link
Contributor

xeji commented Aug 1, 2018

Issue description

Merging #44273 broke the nixpkgs tarball with this error in eval-release.nix, apparently because the alias ag for silver-searcher is not known in that context.

Bug or feature? If this behavior is intended, does it mean that aliases cannot be used in derivations?

Steps to reproduce

nix-build pkgs/top-level/release.nix -A tarball on master 696b426

@matthewbauer
Copy link
Member

Yes this was intentional. The relevant config is allowAliases: https://github.com/NixOS/nixpkgs/blob/master/pkgs/top-level/release.nix#L19. My opinion is that it's fine to use aliases /outside/ of Nixpkgs but inside we should always refer to the real name. The hope is that all of them are pretty straightforward to fix - but it looks like OfBorg is not yet catching them for some reason? Perhaps it was just an older PR.

@xeji
Copy link
Contributor Author

xeji commented Aug 1, 2018

Fine with me but ofborg needs to catch these so we don't accidentally block the channel.
The PR was less than a day old, so I guess ofborg should be modified to reject aliases.

@xeji
Copy link
Contributor Author

xeji commented Aug 1, 2018

cc @grahamc

@grahamc
Copy link
Member

grahamc commented Aug 1, 2018

@LnL7 had a good point about rejecting aliases during build checks: a badly written alias change could break nixpkgs for everyone, and we'd never know it until after everything was released. This is part of the reason the alias disabling check hasn't been deployed yet: I'm concerned about the potential problems this strictness will cause, and am less clear on the actual value of prohibiting aliases during normal evaluation. The reason I have only just now said so is due to my recovery from surgery.

@matthewbauer
Copy link
Member

Here is the PR for OfBorg that I think will do this: NixOS/ofborg#203

@matthewbauer
Copy link
Member

matthewbauer commented Aug 3, 2018

An alternative if people think blocking evaluation on aliases is too strong would be to move the allowAliases = false to make-tarball.nix (https://github.com/NixOS/nixpkgs/blob/master/pkgs/top-level/make-tarball.nix#L81-L84).

This would mean that all of Nixpkgs would still evaluate, but for the tarball to build it would need to evaluate with allowAliases = false;. I am not sure how closely most people follow the tarball job though, but I think this could solve some concerns.

@xeji
Copy link
Contributor Author

xeji commented Aug 3, 2018

This was probably discussed elsewhere before, but I really see no point in blocking aliases. Either we have them, and they should work interchangeably with the original names, or we don't.

Allowing them somewhere but not everywhere just causes confusion .

If we want an eval check for aliases to get rid of them eventually, can't we just make it issue warnings instead of errors?

@matthewbauer
Copy link
Member

It sounds like there is not a consensus on doing this. If so I am perfectly fine with someone reverting 0d8076b. I just hate to see us backsliding into using aliases once work has been done removing them.

xeji added a commit that referenced this issue Aug 6, 2018
didn't eval on Hydra as release.nix doesn't allow aliases, see #44299
Use samba instead.
ElvishJerricco pushed a commit to ElvishJerricco/nixpkgs that referenced this issue Aug 7, 2018
Fixes broken nixpkgs.tarball hydra build.
See NixOS#44299
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants