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

treewide: handle *Phases variables __structuredAttrs-agnostically (round 2) #352709

Open
wants to merge 7 commits into
base: staging
Choose a base branch
from

Conversation

ShamrockLee
Copy link
Contributor

This PR clears the remaining non-__structuredAttrs-agnostic appending to the *Phase list not caught by #339117.

For the "insert the new phase before certain phase" operation, this commit adds a new Bash function replaceElement to setup.sh to provide a more graceful solution to the string-replacement hack in #339117 and previous implementations. (Previous discussion: #339117 (comment)).

Cc: @philiptaron @wolfgangwalther

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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Copy link
Contributor

@wolfgangwalther wolfgangwalther left a comment

Choose a reason for hiding this comment

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

I think we should not introduce any new helper to stdenv here, but instead fix the 4 cases "properly" as suggested in the other comments. This will remove all those hacks depending on order of the preXPhases.

pkgs/development/libraries/glib/setup-hook.sh Outdated Show resolved Hide resolved
pkgs/development/python-modules/pytest-xdist/setup-hook.sh Outdated Show resolved Hide resolved
@wolfgangwalther
Copy link
Contributor

I think we should not introduce any new helper to stdenv here, but instead fix the 4 cases "properly" as suggested in the other comments. This will remove all those hacks depending on order of the preXPhases.

I opened #360450 to address those special cases. If we do this, then we don't need introduce any new helpers here.

@ShamrockLee ShamrockLee force-pushed the structured-attrs-phases-2 branch from 292a749 to 2db8439 Compare December 16, 2024 17:34
@github-actions github-actions bot removed 6.topic: python 6.topic: stdenv Standard environment labels Dec 16, 2024
@ShamrockLee
Copy link
Contributor Author

I opened #360450 to address those special cases. If we do this, then we don't need introduce any new helpers here.

@wolfgangwalther Thank you for fixing them. I dropped the commits already adjusted by #360450.

@ShamrockLee
Copy link
Contributor Author

There are still a lot of rebuilds. I'll re-target this feature branch to staging.

@ShamrockLee ShamrockLee force-pushed the structured-attrs-phases-2 branch from 2db8439 to 01dc95c Compare December 16, 2024 17:46
@ShamrockLee ShamrockLee changed the base branch from master to staging December 16, 2024 17:46
@ShamrockLee ShamrockLee removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 16, 2024
@ShamrockLee
Copy link
Contributor Author

@ofborg build tests.devShellTools.nixos
@ofborg build tests.dotnet
@ofborg build tests.testers
@ofborg build tests.trivial-builders.references
@ofborg build tests.vim

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

Successfully merging this pull request may close these issues.

4 participants