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

pkgs/stdenv: normalize attributes where setup.sh makes no distinction #201131

Closed
wants to merge 1 commit into from
Closed

pkgs/stdenv: normalize attributes where setup.sh makes no distinction #201131

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 14, 2022

This should wait until after staging for 22.11 has branched-off.

Description of changes

This PR normalizes the final attrset immediately before it is passed to builtins.derivation, deleting attributes whose value is [], "", or false if setup.sh treats that particular value for that particular attribute indentically to the attribute being absent.

As a result of this PR, it is possible to add, to an existing derivation, an attribute with the form:

  hardeningFlags = lib.optionals stdenv.hostPlatform.isFrobnicator [
    ...

without triggering an unnecessary mass-rebuild unless Frobnicator is one of the Hydra supportedPlatforms. Previously it was necessary to either use the non-idiomatic:

  if !stdenv.hostPlatform.isFrobnicator then null else [

or to trigger a mass-rebuild and suffer OfBorg's "red ovals of doom" on the PR. The latter case tends to lead to stale and forgotten PRs.

Recent example: #199978 (comment)

Things done
  • Built on platform(s)
    • x86_64-linux (in progress)
    • powerpc64le-linux (in progress)
    • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Nov 14, 2022
@ghost ghost changed the title pkgs/stdenv: normalize [] to null where setup.sh makes no distinction pkgs/stdenv: normalize attributes where setup.sh makes no distinction Nov 14, 2022
This PR normalizes the final attrset immediately before it is passed
to `builtins.derivation`, deleting attributes whose value is `[]`,
`""`, or `false` if `setup.sh` treats that particular value for that
particular attribute indentically to the attribute being absent.

As a result of this PR, it is possible to add, to an existing
derivation, an attribute with the form:

```
  hardeningFlags = lib.optionals stdenv.hostPlatform.isFrobnicator [
    ...
```

without triggering an unnecessary mass-rebuild unless Frobnicator is
one of the Hydra `supportedPlatforms`.  Previously it was necessary
to either use the non-idiomatic:

```
  if !stdenv.hostPlatform.isFrobnicator then null else [
```

or to trigger a mass-rebuild and suffer OfBorg's "red ovals of doom"
on the PR.  The latter case tends to lead to stale and forgotten PRs.

Recent example: #199978 (comment)
@roberth
Copy link
Member

roberth commented Nov 17, 2022

This will have a significant evaluation performance cost. It also increases the size of .drv files, making them less readable, but that might be a minor problem.

Possible optimizations:

  • move the attrset of normalized attrs to a let binding on line 2, so that it is only constructed once
  • apply the normalized attrs as late as possible, and don't return them in the package attribute set:
- derivationArgBeforeNormalization =
+ derivationArg =
    (removeAttrs attrs
      ["meta" "passthru" "pos"
-  (derivation derivationArg);
+  (legacyDerivationAttrs // derivationStrict (normalizedAttrs derivationArg))
legacyDerivationAttrs = ..... derivationArg;

legacyDerivationAttrs will need to be written to restore the (sub-optimal) functionality of derivation, that it returns its arguments alongside the proper derivation output attributes. We can't just break that, but we can elide the normalized attrs from it, so that the normalized attrs can be garbage collected, saving peak memory usage.

The implementation details won't be too complicated, but has to match what Nix does internally.

Also we're going to need non-automated benchmarks for this PR, because ofborg won't be providing its eval performance report. This one is a mass rebuild after all.

@roberth
Copy link
Member

roberth commented Nov 17, 2022

I can't promise that the performance cost can be overcome.

Perhaps at some point a fresh and more modular replacement for mkDerivation may fix this problem out of the box, but that's highly speculative and years from now.

@ghost
Copy link
Author

ghost commented Nov 22, 2022

@roberth, thank you so much for looking at this!

As you surely noticed, it currently does not work. As you probably also noticed, it turns out to be much more difficult than I initially expected.

I have to bump it down my priority list a few notches -- so it may be a month or two before I can revisit it.

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 18, 2023
@ghost ghost closed this Oct 22, 2023
@ghost ghost deleted the pr/stdenv/normalize branch October 22, 2023 07:36
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: stdenv Standard environment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant