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

replaceVarsWith: init #360771

Conversation

wolfgangwalther
Copy link
Contributor

Working my way towards removing substituteAll in #237216.

This takes the extended features of nix substituteAll to a replaceVars variant to get rid of those cases that use substituteAll to build a full package with meta information etc.

There are at least 15 more packages which will make use of replaceVarsWith, but I didn't include the changes here to keep the number of notifications lower while we iterate on the function itself. All those other cases that I currently know of use isExecutable = true.

Part of my overall effort for structuredAttrs in #205690.

@philiptaron @emilazy

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Dec 1, 2024
@nix-owners nix-owners bot requested a review from philiptaron December 1, 2024 13:56
@wolfgangwalther wolfgangwalther force-pushed the substitute-all-replace-vars-with branch 2 times, most recently from 3e8137e to 0147123 Compare December 1, 2024 14:04
@philiptaron
Copy link
Contributor

Welcome to the committers, @wolfgangwalther! Glad to have you here, and thanks as always for your high quality and well-thought-out PRs and collaboration.

@wolfgangwalther
Copy link
Contributor Author

Looking through the current merge conflict I see #360576 - which would be an alternative way of dealing with those cases, without introducing replaceVarsWith.

Opinions, anyone?

@wolfgangwalther wolfgangwalther force-pushed the substitute-all-replace-vars-with branch from 0147123 to 15719f9 Compare December 8, 2024 18:24
Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

I should be able to give this a thorough review this week. Thank you for your patience with the silence so far, @wolfgangwalther.

The interface I expected for replaceVarsWith was an adapted stdenv.mkDerivation, rather than a function of three arguments. I'm open to a function of three arguments, but I need to wrap my head around it.

The short version I had in mind was:

{ replaceVarsWith }:

path: attrs:

replaceVarsWith {
  src = path;
  replacements = attrs;
}

It's sort of fundamentally odd to me for a With variant to have anything other than one attrset as its input.

Regarding #360576, it's sort of a brute-force approach, and I don't like it as much as I like a general-purpose replaceVarsWith.

@wolfgangwalther
Copy link
Contributor Author

The interface I expected for replaceVarsWith was an adapted stdenv.mkDerivation, rather than a function of three arguments.

That works for me, too.

I'm not bound to the current interface at all - I'm all ears for alternative suggestions.

@wolfgangwalther
Copy link
Contributor Author

It's sort of fundamentally odd to me for a With variant to have anything other than one attrset as its input.

Hm. For me it's the opposite. For example, compare zipAttrsWith and zipAttrs. The latter is a specialized / partially applied variant of the former. We do the same here.

Now the question whether partial applications is any useful in this case... probably not.

I also found another example: cleanSource and cleanSourceWith. This seems to be much more like what you have in mind.

@wolfgangwalther wolfgangwalther force-pushed the substitute-all-replace-vars-with branch 2 times, most recently from 93b9dfa to 340ed46 Compare December 14, 2024 13:31
@wolfgangwalther
Copy link
Contributor Author

The interface I expected for replaceVarsWith was an adapted stdenv.mkDerivation, rather than a function of three arguments. I'm open to a function of three arguments, but I need to wrap my head around it.

I tried the approach with one attrset as argument in a fixup commit. I like it more as well. It also makes the diff nicer to read.

Regarding #360576, it's sort of a brute-force approach, and I don't like it as much as I like a general-purpose replaceAttrsWith.

I put lsb-release back in and moved it to the new replaceVarsWith as well.

Copy link
Contributor

@philiptaron philiptaron 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 it a lot. One request for the copy-and-paste machine to come out, then I'd like to merge. This looks great in all the users.

@philiptaron
Copy link
Contributor

If the PR is just the replaceVarsWith, is it able to target master?

@wolfgangwalther
Copy link
Contributor Author

If the PR is just the replaceVarsWith, is it able to target master?

Nope, see #357395, which caused a lot of darwin rebuilds, too. Since replaceVars is also changed here, this will cause the same. Also #357395 has not made its way to master, yet, so there would be a few annoying merge conflicts coming up.

Takes the extended features of nix substituteAll to a replaceVars
variant to get rid of those cases that use substituteAll to build a full
package with meta information etc.
@wolfgangwalther wolfgangwalther force-pushed the substitute-all-replace-vars-with branch from 340ed46 to 4ce241c Compare December 14, 2024 19:17
@philiptaron
Copy link
Contributor

Looks great, tastes great. Want to use your new committer powers to push the button?

@wolfgangwalther wolfgangwalther merged commit b3fb9af into NixOS:staging Dec 14, 2024
23 of 24 checks passed
@wolfgangwalther wolfgangwalther deleted the substitute-all-replace-vars-with branch December 14, 2024 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 5001+ 10.rebuild-linux: 5001+
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants