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

literalExample -> literalExpression #2371

Merged
merged 2 commits into from
Oct 12, 2021

Conversation

ncfavier
Copy link
Member

@ncfavier ncfavier commented Oct 5, 2021

NixOS/nixpkgs#136909 renamed literalExample to literalExpression and deprecated literalExample, so this PR brings home-manager up to speed.

This PR probably shouldn't be merged before NixOS/nixpkgs#136909 lands in nixos-unstable (https://nixpk.gs/pr-tracker.html?pr=136909) so that people have a chance to upgrade (and before the tests pass, in any case).

I intend to make a follow-up PR fixing issues with default and example values in the same spirit as NixOS/nixpkgs#136909, but this one should be easy to review.

See also https://gitlab.com/rycee/nmd/-/merge_requests/3

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

    • Added myself and the module files to .github/CODEOWNERS.

@berbiche
Copy link
Member

berbiche commented Oct 5, 2021

Thanks for the PR and the heads up.

This will be an issue when we merge it since some people mix HM master with a stable version of Nixpkgs.
At the same time, if we ignore the changes for too long then new issues are going to be opened because of all the lib.warn noise.

In either case we should create a new issue ahead of the merge.

@ncfavier
Copy link
Member Author

ncfavier commented Oct 5, 2021

This will be an issue when we merge it since some people mix HM master with a stable version of Nixpkgs.

I don't think that should be supported: the master branch follows nixos-unstable, release branches follow release branches.

@berbiche
Copy link
Member

berbiche commented Oct 5, 2021

This will be an issue when we merge it since some people mix HM master with a stable version of Nixpkgs.

I don't think that should be supported: the master branch follows nixos-unstable, release branches follow release branches.

It's not supported at all but we still have to help users when issues arise.
See for instance the introduction of pkgs.formats: https://github.com/nix-community/home-manager/issues/1516

@ncfavier
Copy link
Member Author

ncfavier commented Oct 5, 2021

Right, a pinned issue is probably a good idea

@rycee
Copy link
Member

rycee commented Oct 8, 2021

I would suggest changing Nixpkgs to have literalExample = literalExpression for one release cycle, then add the warning for another release cycle, then for the third cycle remove it entirely.

@rycee
Copy link
Member

rycee commented Oct 8, 2021

About compatibility I think that instead of switching to literalExpression you could add hm.literalExpression with a definition along the lines of

literalExpression = lib.literalExpression or lib.literalExample

@ncfavier
Copy link
Member Author

ncfavier commented Oct 9, 2021

About compatibility I think that instead of switching to literalExpression you could add hm.literalExpression with a definition along the lines of

literalExpression = lib.literalExpression or lib.literalExample

Can do, but I'd rather add it as lib.literalExpression to keep things transparent.

As @berbiche pointed out on Matrix, a better long-term solution might be to use a pinned version of nixpkgs for the lib while still using the user-supplied nixpkgs for packages.

@rycee
Copy link
Member

rycee commented Oct 9, 2021

If it's possible to add as lib.literalExpression without introducing an infinite loop then that would be great.

I think using a pinned Nixpkgs would be good if we only used lib internally, but since lib is exposed as an argument to all modules it would cause problems if people want to use their own extended lib or want to use lib from another Nixpkgs commit.

We could conceivably leave lib as-is and add a _lib or something that is pinned and reserved for HM use.

@ncfavier ncfavier force-pushed the literalExpression branch 2 times, most recently from 8422cf5 to 2d1bff4 Compare October 9, 2021 09:57
@ncfavier
Copy link
Member Author

ncfavier commented Oct 9, 2021

NixOS/nixpkgs#136909 is in nixos-unstable, I have confirmed that this PR passes all the tests and works fine both before and after NixOS/nixpkgs#136909. As far as I'm concerned it's ready to merge.

@berbiche
Copy link
Member

berbiche commented Oct 9, 2021

We could wait for: NixOS/nixpkgs#141099

@ncfavier
Copy link
Member Author

ncfavier commented Oct 9, 2021

Doesn't bdf8e92 make that useless in our case?

@ncfavier
Copy link
Member Author

@rycee rycee merged commit 4fa1ba7 into nix-community:master Oct 12, 2021
@rycee
Copy link
Member

rycee commented Oct 12, 2021

Thanks! Merged to master now.

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.

3 participants