Skip to content

treewide: use writableTmpDirAsHomeHook#378110

Merged
GaetanLepage merged 14 commits intoNixOS:masterfrom
drupol:push-mquukywqotzq
Jan 31, 2025
Merged

treewide: use writableTmpDirAsHomeHook#378110
GaetanLepage merged 14 commits intoNixOS:masterfrom
drupol:push-mquukywqotzq

Conversation

@drupol
Copy link
Contributor

@drupol drupol commented Jan 30, 2025

Reference to #370869

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

@github-actions github-actions bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Jan 30, 2025
@drupol drupol requested a review from GaetanLepage January 30, 2025 20:27
@drupol drupol force-pushed the push-mquukywqotzq branch from 59cff8b to d6fbdc1 Compare January 30, 2025 20:54
@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Jan 30, 2025
@nix-owners nix-owners bot requested a review from Ericson2314 January 30, 2025 20:55
@drupol drupol force-pushed the push-mquukywqotzq branch from d6fbdc1 to ab37260 Compare January 30, 2025 21:15
@github-actions github-actions bot added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. and removed 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. labels Jan 30, 2025
Copy link
Contributor

@GaetanLepage GaetanLepage left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this !

@drupol drupol marked this pull request as draft January 31, 2025 08:51
@drupol drupol force-pushed the push-mquukywqotzq branch from ab37260 to 194aeb2 Compare January 31, 2025 09:01
@drupol drupol marked this pull request as ready for review January 31, 2025 09:17
@drupol drupol force-pushed the push-mquukywqotzq branch from 194aeb2 to 71cfe3b Compare January 31, 2025 09:21
@GaetanLepage GaetanLepage merged commit 07de736 into NixOS:master Jan 31, 2025
23 checks passed
@drupol drupol deleted the push-mquukywqotzq branch January 31, 2025 10:33
Comment on lines +30 to +32
nativeCheckInputs = [
writableTmpDirAsHomeHook
];
Copy link
Contributor

Choose a reason for hiding this comment

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

I find that this pattern does not work for go packages. The writableTmpDirAsHome bash function is never called and $HOME remains to be HOME=/homeless-shelter. Any ideas? cc @ShamrockLee

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried recently ?
The hook was initially broken but got fixed afterwards.

Copy link
Contributor

Choose a reason for hiding this comment

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

My test is on the master branch 8b564bb.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the right commit ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It works if I do one of these things:

  • also add writableTmpDirAsHomeHook to checkInputs
  • disable strictDeps: nix-repl> :b apx.overrideAttrs { strictDeps = false; }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, I have no clue who to contact. Can you ping the relevant nicknames here perhaps?

Copy link
Contributor

Choose a reason for hiding this comment

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

@NixOS/stdenv @NixOS/golang @infinisil @roberth Sorry for bothering, but does anyone know why setup hooks using addEnvHooks "$targetOffset" don't take effect when passing to buildGoModule?

Copy link
Contributor

@jian-lin jian-lin Feb 2, 2025

Choose a reason for hiding this comment

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

@NickCao told me how setup hooks work and why this issue happens only for go packages here. Thank you, @NickCao .

The following patch fixes it:

-addEnvHooks "$targetOffset" writableTmpDirAsHome
+addEnvHooks "$hostOffset" writableTmpDirAsHome

Here is the explanation. Assume writableTmpDirAsHomeHook is added to nativeBuildInputs (or nativeCheckInputs). If hostOffset is used, the bash function writableTmpDirAsHome is run against each dependency in depsBuildBuild depsBuildHost(nativeBuildInputs) depsBuildTarget. If targetOffset is used, it is run against each dependency in depsHostHost depsHostTarget(buildInputs). Go packages usually have no dependencies in depsHostHost depsHostTarget(buildInputs), so the bash function writableTmpDirAsHome is never run and we observe this issue. Since we assume writableTmpDirAsHomeHook is added to nativeBuildInputs, there is at least one dependency (the hook itself) in depsBuildBuild depsBuildHost(nativeBuildInputs) depsBuildTarget. As a result, the bash function writableTmpDirAsHome will always run when hostOffset is used.

The implementation of addEnvHooks may be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in: #378927

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for making the investigation, very interesting !!!

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

Labels

6.topic: python Python is a high-level, general-purpose programming language. 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants