Skip to content

Comments

nixos/top-level: improve replaceRuntimeDependencies#257234

Merged
yu-re-ka merged 8 commits intoNixOS:masterfrom
alois31:replacedependencies
Sep 24, 2024
Merged

nixos/top-level: improve replaceRuntimeDependencies#257234
yu-re-ka merged 8 commits intoNixOS:masterfrom
alois31:replacedependencies

Conversation

@alois31
Copy link
Contributor

@alois31 alois31 commented Sep 25, 2023 •
edited
Loading

Description of changes

Instead of iterating over all replacements and applying them one by one, use the newly introduced replaceDependencies function to apply them all at once for replaceRuntimeDependencies. The advantages are twofold in case there are multiple replacements:

  • Performance is significantly improved, because there is only one pass over the closure to be made.
  • Correctness is improved, because replaceDependencies also replaces dependencies of the replacements themselves if applicable.

Further, the option is moved to a system.replaceDependencies namespace more consistent with the arguments to the replaceDependencies function. This allows wiring up the cutoffPredicate functionality, which is also configured to skip the initrd by default because of uselessness in the best case and breakage in the worst case.

Fixes: #4336
Fixes: #199162

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

@alois31 alois31 requested a review from dasJ as a code owner September 25, 2023 09:20
@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 Sep 25, 2023
@alois31 alois31 force-pushed the replacedependencies branch from 8845fc2 to 1940199 Compare September 25, 2023 09:25
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Sep 25, 2023
@alois31 alois31 force-pushed the replacedependencies branch from 1940199 to 476a209 Compare October 12, 2023 15:02
@alois31 alois31 marked this pull request as draft October 13, 2023 06:19
@alois31 alois31 marked this pull request as ready for review October 14, 2023 16:59
@alois31 alois31 force-pushed the replacedependencies branch from c05cabd to 5b650f1 Compare November 11, 2023 09:10
@bryango
Copy link
Member

bryango commented Dec 7, 2023

This is awesome! I don't know whether it is impolite to ping people, but maybe @ncfavier would be interested in taking a look?

@RaitoBezarius RaitoBezarius requested a review from roberth December 9, 2023 19:51
@roberth
Copy link
Member

roberth commented Dec 9, 2023

Can't really review now, but tests will be needed, and, more superficially, use let inherit (lib) or if necessary let inherit (builtins) to make the code a little more readable, and a bit faster (don't know if this has hot paths, but it's the second argument for it ;) ).

@alois31 alois31 force-pushed the replacedependencies branch from 5b650f1 to e6bd44f Compare December 10, 2023 10:38
@alois31
Copy link
Contributor Author

alois31 commented Dec 10, 2023

Can't really review now, but tests will be needed

Between this stuff relying on IFD and Hydra not supporting IFD, I'm stuck in a bit of a bad place here. A test that can't run on Hydra seems pretty pointless, and any ideas I have come up with for circumventing this problem incur a significant cost to other people or infrastructure. If you or anyone else can come up with even a vague idea of how to reasonably test things, I will be glad to implement it.

and, more superficially, use let inherit (lib) or if necessary let inherit (builtins) to make the code a little more readable, and a bit faster (don't know if this has hot paths, but it's the second argument for it ;) ).

I was unaware that this style is prefered, and also unaware of the performance benefit (although I doubt it matters that much between creating hundreds of derivations). Changed it.

@roberth
Copy link
Member

roberth commented Dec 10, 2023

You could write a shell script that calls Nix and performs all the assertions, use that for (further) development, and wrap it into a NixOS tests so that Hydra runs it as well.

@alois31
Copy link
Contributor Author

alois31 commented Dec 10, 2023

Yeah, that would have been the "significant cost to infrastructure" solution. I'm under the impression that NixOS tests are already on the expensive side, and having to include nixpkgs itself (so that that the configuration with replaceDependencies applied can be evaluated) would be particularly bad. If people think this fear is unjustified, I will implement it next week (there should be no need to rush this anyway).

@alois31 alois31 force-pushed the replacedependencies branch from e6bd44f to 41b3087 Compare December 16, 2023 18:06
@alois31
Copy link
Contributor Author

alois31 commented Dec 16, 2023

Some tests are now implemented (in a separate commit, so that they can be removed easily if they turn out to be too heavy). I also found two bugs (the replaceDependency compatibility function not working properly at all, and a precedence issue), which have been fixed in the most recent version.

@alois31

This comment was marked as outdated.

Sign in to view
@viperML
Copy link
Contributor

viperML commented Dec 19, 2023

Today I just faced with replaceRuntimeDependencies requiring IFD.

@alois31
Copy link
Contributor Author

alois31 commented Dec 19, 2023

Today I just faced with replaceRuntimeDependencies requiring IFD.

Note that even this improved version requires IFD. Really, that requirement is very unlikely to go away, because in order to know the references, the build result has to be known. (Technically, you could fetch the entire build input closure as a superset of the references (which would be doable in a very hacky way), but that tends to be extremely large.)

@alois31 alois31 marked this pull request as draft January 27, 2024 12:11
@alois31 alois31 force-pushed the replacedependencies branch from a5f35de to 56ddab9 Compare January 27, 2024 13:06
@alois31 alois31 marked this pull request as ready for review January 27, 2024 13:08
bryango
bryango reviewed Feb 26, 2024
View reviewed changes
Copy link
Member

@bryango bryango left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @alois31, thank you very much for this wonderful work! I would really love this PR to be merged, and I am slowly reviewing the changes here. This is not easy for me as I am not very familiar with nix internals so it may take a long time... But I will try to post some feedback and questions here as I go through the code. Thank you again for this work!

@alois31 alois31 force-pushed the replacedependencies branch from 56ddab9 to 87d7fee Compare February 27, 2024 18:02
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label Mar 24, 2024
@alois31 alois31 marked this pull request as draft April 3, 2024 16:58
@alois31 alois31 force-pushed the replacedependencies branch 2 times, most recently from da79a19 to ff697c6 Compare April 3, 2024 17:18
@alois31 alois31 marked this pull request as ready for review April 4, 2024 16:40
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one person. label Apr 5, 2024
@alois31 alois31 force-pushed the replacedependencies branch from ff697c6 to 202b37c Compare April 19, 2024 17:29
@alois31 alois31 force-pushed the replacedependencies branch from 202b37c to c4b32ac Compare May 11, 2024 11:25
@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label May 11, 2024
@alois31 alois31 force-pushed the replacedependencies branch 2 times, most recently from acf5ae3 to e6fdfbe Compare June 14, 2024 18:00
@github-actions github-actions bot removed the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Jun 14, 2024
@RossComputerGuy RossComputerGuy self-requested a review September 15, 2024 17:23
@RossComputerGuy
Copy link
Member

RossComputerGuy commented Sep 15, 2024

Will take a look at this when I have time to.

@alois31 @yuyuyureka
replaceDependencies: evolve from replaceDependency
36a60bc 
Rewrite replaceDependency so that it can apply multiple replacements in
one go. This includes correctly handling the case where one of the
replacements itself needs to have another replacement applied as well.
This rewritten function is now aptly called replaceDependencies.

For compatibility, replaceDependency is retained as a simple wrapper
over replaceDependencies. It will cause a rebuild because the unpatched
dependency is now referenced by derivation instead of by storePath, but
the functionality is equivalent.

Fixes: NixOS#199162
@alois31 @yuyuyureka
nixos/top-level: improve replaceRuntimeDependencies
1dc5fd5 
Instead of iterating over all replacements and applying them one by one,
use the newly introduced replaceDependencies function to apply them all
at once for replaceRuntimeDependencies. The advantages are twofold in
case there are multiple replacements:
* Performance is significantly improved, because there is only one pass
  over the closure to be made.
* Correctness is improved, because replaceDependencies also replaces
  dependencies of the replacements themselves if applicable.

Fixes: NixOS#4336
@alois31 @yuyuyureka
nixos/top-level: wire up cutoffPackages for replaceDependencies
2ab2aed 
Move replaceRuntimeDependencies to the replaceDependencies namespace,
where the structure is more consistent with the replaceDependencies
function. This makes space for wiring up cutoffPackages as an option
too.

By default, the system's initrd is excluded. The replacement process does not
work properly anyway due to the structure of the initrd (the files being copied
into it, and it being compressed). In the worst case (which has been observed
to actually occur in practice), a store path makes it into the incompressible
parts of the archive, checksums are broken, and the system won't boot.
@alois31 @yuyuyureka
replaceDependencies: add tests
9846cc6 
The tests cannot be directly built by Hydra, because replaceDependencies relies
on IFD. Instead, they are put inside a NixOS test where they are built on the
guest.
@alois31 @yuyuyureka
replaceDirectDependencies: split off from replaceDependencies
ef10039 
This allows both swapping out and reusing the rewrite machinery.
@alois31 @yuyuyureka
replaceDependencies: add support for ca-derivations
3e2ff7f 
Unlike regular input-addressed or fixed-output derivations, floating and
deferred derivations do not have their store path available at evaluation time,
so their outPath is a placeholder. The following changes are needed for
replaceDependencies to continue working:
* Detect the placeholder and retrieve the store path using another IFD hack
  when collecting the rewrite plan.
* Try to obtain the derivation name needed for replaceDirectDependencies from
  the derivation arguments if a placeholder is detected.
* Move the length mismatch detection to build time, since the placeholder has a
  fixed length which is unrelated to the store path.
@alois31 @yuyuyureka
replaceDependencies: show warnings as such
3b78fd1
@alois31 @yuyuyureka
replaceDependencies: do not build unused replacements
8bbea7f 
To prevent excessive build times when replacement lists are shared between
partially overlapping closures, skip the build of unused replacements.
Unfortunately, this also means that the replacement won't be applied any more
if another replacement that is applied introduces its source. But this is a
corner case, and we already show a warning, so make it clearer that handling
this situation (should it ever arise) is the responsibility of the user.
@yu-re-ka yu-re-ka merged commit 965289e into NixOS:master Sep 24, 2024
@roconnor
Copy link
Contributor

roconnor commented Oct 20, 2024

Nice work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dasJ dasJ dasJ approved these changes

@RossComputerGuy RossComputerGuy RossComputerGuy approved these changes

@roberth roberth Awaiting requested review from roberth

@philiptaron philiptaron Awaiting requested review from philiptaron

+1 more reviewer

@bryango bryango bryango left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

replaceDependency can't be used in pure evaluation mode Mutiple replaceRuntimeDependencies may reintroduce depenendices

9 participants

@alois31 @bryango @roberth @viperML @RossComputerGuy @roconnor @dasJ @wegank @yu-re-ka