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

Jump to bottom

lib/modules: Change mkAliasOptionModule to use the priority for the alias #54528

Merged
merged 2 commits into from Feb 23, 2019
Merged

lib/modules: Change mkAliasOptionModule to use the priority for the alias #54528

merged 2 commits into from Feb 23, 2019

Conversation

cdepillabout
Copy link
Member

@cdepillabout cdepillabout commented Jan 24, 2019 •
edited

This commit changes the mkAliasOptionModule function to make sure that the priority for the aliased option is propagated to the non-aliased option.

This also affects the mkRenamedOptionModule function in a similar fashion.

This also removes the mkAliasOptionModuleWithPriority function, since its functionality is now subsumed by mkAliasOptionModule.

This change was recommended by @nbp to see what would be the effect of doing this:
#53397 (comment)

Motivation for this change

The motivation for this change (as well as a use-case) is listed in #53397 (comment).

Pinging @nbp, since he suggested this change. Also pinging @infinisil since he helped with the original PR that inspired this change.

Things done
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@cdepillabout
lib/modules: Change mkAliasOptionModule to use the priority for the a…
81fa1e3 
…lias.

This commit changes the `mkAliasOptionModule` function to make sure that
the priority for the aliased option is propagated to the non-aliased
option.

This also affects the `mkRenamedOptionModule` function in a similar
fashion.

This also removes the `mkAliasOptionModuleWithPriority` function, since
its functionality is now subsumed by `mkAliasOptionModule`.

This change was recommended by @nbp:
NixOS#53397 (comment)
cdepillabout added a commit to cdepillabout/ofborg that referenced this pull request Jan 24, 2019
@cdepillabout
add cdepillabout to config.extra-known-users.json
38bdb28 
This is so I can run tests with ofborg.

For instance I'd like to be able to run the tests affected by this PR: NixOS/nixpkgs#54528
@cdepillabout cdepillabout mentioned this pull request Jan 24, 2019
Merged
@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/nix-online-meetup/1919/4

@cdepillabout
Copy link
Member Author

@nbp Would you be able to take a look at this? It might be nice to get this in for 19.03.

@oxij
Copy link
Member

oxij commented Feb 23, 2019 via email

@cdepillabout
Copy link
Member Author

cdepillabout commented Feb 23, 2019 •
edited

@oxij Thanks for doing a review of this. You bring up some good questions. Let me try to answer them.

Having read the aforementioned thread I still don't understand what is going on here, wasn't it just a bug that mkAliasOptionModule didn't copy the priority?

Originally, the documentation for the functions in question mentioned that they don't copy the priority. I assumed it wasn't a bug, but intentional. In the few previous PRs I sent surrounding this issue, no one seemed to think it was a bug.

This PR changes mkAliasOptionModule (and mkRenamedOptionModule) to copy over the priority.

My assumption is that this isn't really "fixing a bug", but adding an additional feature to these functions.

I think that most people who use mkAliasOptionModule (and mkRenamedOptionModule) will want this new functionality.

Shouldn't mkRenamed* do the same too?

Yes, this PR changes mkRenamedOptionModule to also copy over the priority. (I guess this really should be more prominent in the title and explanation.)

Why was that second function (mkAliasOptionModuleWithPriority) introduced in the first place?

mkAliasOptionModuleWithPriority was introduced to do the same thing as mkAliasOptionModule, but also additionally copy over the priority.

It makes sense to have mkAliasOptionModuleWithPriority if you think that mkAliasOptionModule shouldn't copy over the priority. Since this PR changes mkAliasOptionModule to copy over the priority, then mkAliasOptionModuleWithPriority is no longer needed.

Shouldn't this PR just remove all withPriority stuff and always propagate the priority?

Yeah, that is one option. I'd be fine with that (since it would make the code slightly simpler), but I'm not sure why mkAliasOptionModule originally did not copy over the priority. If this was just an oversight, then I imagine it wouldn't have been added to the documentation. If it was intentional, then I didn't want to completely change it if someone was relying on that functionality.

This PR does change that functionality, but at least it provides a backdoor for someone to use who is depending on that functionality (by leaving the option available in doRename).

@oxij
Copy link
Member

oxij commented Feb 23, 2019

I see. I guess it would be nice to hear what @nbp thinks about this then. I see no explanation for not propagating the priority in the first place.

@infinisil
Copy link
Member

I think this is fine, this change makes sense, and I also even doubt anybody relied on the previous behavior

@infinisil infinisil merged commit d3216be into NixOS:master Feb 23, 2019
@cdepillabout cdepillabout deleted the module-alias-uses-priority branch February 24, 2019 13:06
@infinisil infinisil added the 6.topic: module system About NixOS module system internals label Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danbst danbst danbst left review comments

@edolstra edolstra Awaiting requested review from edolstra

@nbp nbp Awaiting requested review from nbp

Assignees
No one assigned
Labels
6.topic: module system About NixOS module system internals 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@cdepillabout @nixos-discourse @oxij @infinisil @danbst @GrahamcOfBorg