Skip to content

makeSetupHook: support depsTargetTargetPropagated (fix using hooks when strictDeps)#208537

Merged
Artturin merged 6 commits intoNixOS:stagingfrom
Artturin:makesetuphooksupportstrictdeps
Feb 10, 2023
Merged

makeSetupHook: support depsTargetTargetPropagated (fix using hooks when strictDeps)#208537
Artturin merged 6 commits intoNixOS:stagingfrom
Artturin:makesetuphooksupportstrictdeps

Conversation

@Artturin
Copy link
Member

@Artturin Artturin commented Dec 31, 2022 •
edited
Loading

Closes #49132

Description of changes
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.05 Release Notes (or backporting 22.11 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.

@Artturin Artturin changed the base branch from master to staging December 31, 2022 19:13
@github-actions github-actions bot added 6.topic: lua Lua is a powerful, efficient, lightweight, embeddable scripting language. 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: stdenv Standard environment labels Dec 31, 2022
@Artturin Artturin mentioned this pull request Dec 31, 2022
Closed
13 tasks
@Artturin Artturin force-pushed the makesetuphooksupportstrictdeps branch from 78600dd to 07472d2 Compare December 31, 2022 19:17
@Artturin Artturin added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label Dec 31, 2022
@Artturin Artturin force-pushed the makesetuphooksupportstrictdeps branch 2 times, most recently from c2a9dae to 6bef692 Compare December 31, 2022 19:53
@Artturin Artturin mentioned this pull request Dec 31, 2022
Closed
@Artturin Artturin force-pushed the makesetuphooksupportstrictdeps branch from 6bef692 to edc083d Compare December 31, 2022 20:48
@ofborg ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Dec 31, 2022
SuperSandro2000
SuperSandro2000 reviewed Jan 1, 2023
View reviewed changes
pkgs/build-support/setup-hooks/wrap-gapps-hook/default.nix Outdated
Comment on lines +75 to +79
Copy link
Member

@SuperSandro2000 SuperSandro2000 Jan 1, 2023

Choose a reason for hiding this comment

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

Suggested change
basic-contains-gdk-pixbuf = let
tested = basic;
in testLib.runTest "basic-contains-gdk-pixbuf" (
testLib.skip stdenv.isDarwin ''
${expectSomeLineContainingYInFileXToMentionZ "${tested}/bin/foo" "GDK_PIXBUF_MODULE_FILE" "${lib.getLib librsvg}/lib/gdk-pixbuf-2.0/2.10.0/loaders.cache"}
${expectSomeLineContainingYInFileXToMentionZ "${tested}/libexec/bar" "GDK_PIXBUF_MODULE_FILE" "${lib.getLib librsvg}/lib/gdk-pixbuf-2.0/2.10.0/loaders.cache"}
basic-contains-gdk-pixbuf = testLib.runTest "basic-contains-gdk-pixbuf" (
testLib.skip stdenv.isDarwin ''
${expectSomeLineContainingYInFileXToMentionZ "${basic}/bin/foo" "GDK_PIXBUF_MODULE_FILE" "${lib.getLib librsvg}/lib/gdk-pixbuf-2.0/2.10.0/loaders.cache"}
${expectSomeLineContainingYInFileXToMentionZ "${basic}/libexec/bar" "GDK_PIXBUF_MODULE_FILE" "${lib.getLib librsvg}/lib/gdk-pixbuf-2.0/2.10.0/loaders.cache"}

Copy link
Member Author

@Artturin Artturin Jan 1, 2023

Choose a reason for hiding this comment

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

@jtojnar what do you think? since you added these tests

jtojnar
jtojnar reviewed Jan 1, 2023
View reviewed changes
Copy link
Member

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

Nice work.

pkgs/stdenv/generic/setup.sh Outdated
Copy link
Member

@jtojnar jtojnar Jan 1, 2023

Choose a reason for hiding this comment

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

Rest of the fixupPhase function does not seem to use any of these variables (substituteAll only captures environment variables) so this looks good to me.

pkgs/build-support/setup-hooks/wrap-gapps-hook/default.nix Outdated
Copy link
Member

@jtojnar jtojnar Jan 1, 2023

Choose a reason for hiding this comment

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

Maybe move the comment above then. I was quite confused until I noticed it.

Personally, I would just rip the band-aid off. I would not expect this to be very prevalent.

Copy link
Member Author

@Artturin Artturin Jan 20, 2023

Choose a reason for hiding this comment

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

Maybe move the comment above then. I was quite confused until I noticed it.

this comment and the librsvg above it are closer now after rearranging the list

Personally, I would just rip the band-aid off. I would not expect this to be very prevalent.

Should be a separate PR

@Artturin Artturin force-pushed the makesetuphooksupportstrictdeps branch 3 times, most recently from bc38703 to ddfe275 Compare January 1, 2023 14:35
@github-actions github-actions bot added 6.topic: qt/kde Object-oriented framework for GUI creation 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 6.topic: vim Advanced text editor labels Jan 1, 2023
@Artturin Artturin force-pushed the makesetuphooksupportstrictdeps branch from 172f7f5 to 11b50f4 Compare January 1, 2023 20:27
@Artturin Artturin force-pushed the makesetuphooksupportstrictdeps branch from 11b50f4 to 2ba4434 Compare January 2, 2023 14:27
@Artturin Artturin force-pushed the makesetuphooksupportstrictdeps branch 3 times, most recently from a801fc2 to ef6495b Compare January 20, 2023 18:21
@ofborg ofborg bot requested a review from lovek323 January 20, 2023 20:07
@Artturin Artturin marked this pull request as ready for review January 28, 2023 19:15
@github-actions github-actions bot added the 8.has: documentation This PR adds or changes documentation label Jan 28, 2023
@Artturin Artturin force-pushed the makesetuphooksupportstrictdeps branch from a286a76 to 3ec0c3b Compare January 28, 2023 20:00
@winterqt
Copy link
Member

winterqt commented Jan 28, 2023

Should every instance of e.g. Cargo and pip be from buildPackages, as makeWrapper is?

@Artturin
Copy link
Member Author

Artturin commented Jan 29, 2023

Should every instance of e.g. Cargo and pip be from buildPackages, as makeWrapper is?

No, thats just a workaround for hooks in buildInputs / hook in nativeBuildInputs but splicing not working

@Artturin Artturin force-pushed the makesetuphooksupportstrictdeps branch from 3ec0c3b to 680309f Compare February 7, 2023 19:02
@Artturin Artturin requested a review from zowoq as a code owner February 7, 2023 19:02
@Artturin Artturin merged commit aaa9ea8 into NixOS:staging Feb 10, 2023
@Artturin Artturin deleted the makesetuphooksupportstrictdeps branch February 10, 2023 18:08
@Artturin Artturin mentioned this pull request Feb 13, 2023
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jtojnar jtojnar jtojnar left review comments

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

@adevress adevress Awaiting requested review from adevress

@agbrooks agbrooks Awaiting requested review from agbrooks

@lovek323 lovek323 Awaiting requested review from lovek323

@winterqt winterqt Awaiting requested review from winterqt

@IvarWithoutBones IvarWithoutBones Awaiting requested review from IvarWithoutBones

@figsoda figsoda Awaiting requested review from figsoda

@jonringer jonringer Awaiting requested review from jonringer

@ttuegel ttuegel Awaiting requested review from ttuegel

@FRidh FRidh Awaiting requested review from FRidh

@adisbladis adisbladis Awaiting requested review from adisbladis

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

@zowoq zowoq Awaiting requested review from zowoq

Assignees

No one assigned

Labels

6.topic: cross-compilation Building packages on a different platform than they will be used on 6.topic: lua Lua is a powerful, efficient, lightweight, embeddable scripting language. 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: qt/kde Object-oriented framework for GUI creation 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 6.topic: stdenv Standard environment 6.topic: vim Advanced text editor 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Artturin @winterqt @jtojnar @SuperSandro2000