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

[WIP] environment.etc: loaOf -> attrsOf #67458

Closed
wants to merge 36 commits into from

Conversation

alexarice
Copy link
Contributor

Motivation for this change

Having environment.etc set using a list makes it very hard to override with mkForce
see: https://logs.nix.samueldr.com/nixos/2019-08-25#2523375;

Things done

Changed the type of environment.etc to attrsOf and changed every instance of it being set with a list in nixos

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.
Notify maintainers

cc @aanderse @cleverca22

I'm sure there are a lot of mistakes in what I've done, especially with mapAttrs'. Is there any way to go about checking this automatically?
Also I expect my commit names aren't very good or maybe they should be squashed, I'm happy to rebase if someone suggests something better

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Aug 26, 2019

Checkout PR #63103: I'm also trying to deprecate loaOf and remove its use in nixpkgs.

@alexarice
Copy link
Contributor Author

Ah I did not know about this PR. Let me know if anything here would help with it and I could try to move it over?

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Aug 26, 2019

It just needs a lot of testing but I don't have the time/resources to test all the software I touched.
It may be actually better to split it into several PRs, for example:

  • remove uses of lists in loaOf types in nixpkgs: one for environment.etc, another for users.users...
  • actually deprecated loaOf

@alexarice
Copy link
Contributor Author

I agree though even environment.etc is a lot of files and I don't feel I have a good way to check a lot of them

@alexarice
Copy link
Contributor Author

this feels like the sort of thing that should be an rfc?

@alexarice
Copy link
Contributor Author

I'll leave this up for the moment in case it is useful for checking / comparing

@aanderse
Copy link
Member

this feels like the sort of thing that should be an rfc?

I personally don't agree with that at all. You are trying to fix a bug. This just needs adequate awareness and testing, IMO.

@alexarice alexarice closed this Dec 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants