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

Prohibit use of nixpkgs.extend from inside Nixpkgs #258

Closed
wants to merge 2 commits into from

Conversation

roberth
Copy link
Member

@roberth roberth commented Oct 23, 2018

extend is a new function on Nixpkgs that lets users extend Nixpkgs with overlays after calling the Nixpkgs function. This makes Nixpkgs similar to package sets that were created with lib.makeExtensible.

Nixpkgs should probably not start using this, because re-evaluating the Nixpkgs is somewhat expensive.

See NixOS/nixpkgs#47430 (comment)

I am not familiar with ofborg and I wasn't sure how to test this, so it is mostly cargo culted. I did test the ad-hoc overlay itself.

@LnL7
Copy link
Member

LnL7 commented Oct 24, 2018

I'm not sure if it makes sense to run a separate eval for each feature like this. I can't really think of any issues with just running a single strict eval to check for issues.

@roberth
Copy link
Member Author

roberth commented Oct 24, 2018

@LnL7 better?

@LnL7
Copy link
Member

LnL7 commented Oct 24, 2018

Yeah, but maybe this should be a config option in nixpkgs. That way it's a bit less obscure to reproduce.

@roberth
Copy link
Member Author

roberth commented Oct 26, 2018

@LnL7 I don't want people to avoid extend when they actually need it, so, to be honest, I think this check is not for users, and therefore I don't think it should be an option. If someone really wants this check in their own tools, it's not too complicated to reproduce.

If someone is motivated to make it an option, I can review their PR.

@roberth
Copy link
Member Author

roberth commented Oct 26, 2018

cc @Ericson2314

@LnL7
Copy link
Member

LnL7 commented Oct 26, 2018

Having the error message inline in the command just feels a bit weird to me, but yeah it's not that complicated.

@grahamc What do you think?

Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

I like config option too, but this is fine for now. I've asked enough things from @roberth with this :).

@grahamc
Copy link
Member

grahamc commented Oct 31, 2018

I'm sorry to be annoying and not provide more actionable feedback sooner.

This test should be applied to the evaluation which happens before and after a PR is merged. Please move the configuration to https://github.com/NixOS/ofborg/blob/ae982a2a683dfb5062ef8fb1b03bb7c3804b7e31/ofborg/src/outpaths.nix and delete the changes to the massrebuilder.rs.

Aliases are tested this way because aliases are desirable for external users, but can easily rot unless they are checked regularly. This is why the check is performed once with, and once without aliases.

I don't believe extends will ever rot like aliases can, so no need to perform the different checks.

@roberth
Copy link
Member Author

roberth commented Aug 14, 2022

I'm cleaning up my open PR list. After almost four years, this has proven itself to be unimportant in many ways. If anyone disagrees, feel free to modify and use the code in any way you like.

@roberth roberth closed this Aug 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants