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.

Sign up for GitHub

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

nixos/top-level: improve replaceRuntimeDependencies #257234

Merged
merged 8 commits into from
Sep 24, 2024
Merged

nixos/top-level: improve replaceRuntimeDependencies #257234

merged 8 commits into from
Sep 24, 2024
+501 −123

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
@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
@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?

@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
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
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 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!

pkgs/build-support/replace-dependencies.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
@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
@alois31 alois31 force-pushed the replacedependencies branch 2 times, most recently from acf5ae3 to e6fdfbe Compare June 14, 2024 18:00
@RossComputerGuy
Copy link
Member

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

@alois31
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
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
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
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
replaceDirectDependencies: split off from replaceDependencies
ef10039 
This allows both swapping out and reusing the rewrite machinery.
@alois31
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
replaceDependencies: show warnings as such
3b78fd1
@alois31
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
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bryango bryango bryango left review comments

@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 philiptaron is a code owner

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
8 participants
@alois31 @bryango @roberth @viperML @RossComputerGuy @dasJ @wegank @yu-re-ka