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

build-dotnet-module: reintroduce tmpdir fix #338377

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

build-dotnet-module: reintroduce tmpdir fix #338377

wants to merge 3 commits into from

Conversation

erikarvstedt
Copy link
Member

Reintroduce the fix from #209357 which was deleted in #327651.

@corngood, was there a reason for deleting this?

@erikarvstedt erikarvstedt requested a review from corngood as a code owner August 30, 2024 10:38
@github-actions github-actions bot added the 6.topic: dotnet Language: .NET label Aug 30, 2024
@erikarvstedt
build-dotnet-module: reintroduce tmpdir fix
7db358b 
Reintroduce patch a98e520 that
was removed in d3ca502.
@erikarvstedt
build-dotnet-module: simplify tmpdir deletion
d01b4b9 
No need for `chmod`.
`rm -rf DIR` succeeds when:
- Some contents of DIR are not writable
- DIR itself is not writable
@erikarvstedt erikarvstedt force-pushed the fix-dotnet-fetch-tmpdir branch 2 times, most recently from a698256 to 8c09ea9 Compare August 30, 2024 10:40
@erikarvstedt
build-dotnet-module: name tmpdir
6529368 
Useful for debugging.
@erikarvstedt erikarvstedt force-pushed the fix-dotnet-fetch-tmpdir branch from 8c09ea9 to 6529368 Compare August 30, 2024 10:58
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Aug 30, 2024
@corngood
Copy link
Contributor

I didn't intentionally remove it, I just replaced all of fetch-deps with the one from dotnet VMR. Sorry about that.

@corngood
Copy link
Contributor

Inside nix-shell, TMPDIR (used by mktemp) is set to /run/user/ which is usually a tmpfs stored in RAM.

To me this sounds like nix-shell is setting it, but I believe it uses /tmp unless you have TMPDIR set when you call nix-shell. Is that right?

@erikarvstedt
Copy link
Member Author

erikarvstedt commented Aug 30, 2024 •
edited
Loading

The current Nix version 2.18 indeed sets TMPDIR to /tmp (introduced in NixOS/nix#10883), but all previous Nix versions set TMPDIR to /run/user/....
I think we should keep the tmpdir fix until newer Nix versions are widely used.
The error this prevents is annoying to debug and to work around.

corngood
corngood reviewed Aug 30, 2024
View reviewed changes
pkgs/build-support/dotnet/build-dotnet-module/fetch-deps.sh
@@ -1,7 +1,14 @@
set -e

tmp=$(mktemp -d)
trap 'chmod -R +w "$tmp" && rm -fr "$tmp"' EXIT
Copy link
Contributor

Choose a reason for hiding this comment

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

If I cp -r /nix/store/[some package] foo && rm -rf foo, I get

rm: cannot remove 'foo/[...]': Permission denied

I believe it's when a file is in a readonly directory?

@corngood
Copy link
Contributor

I think we should keep the tmpdir fix until newer Nix versions are widely used.

can we add a comment to that effect?

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corngood corngood corngood left review comments

Assignees
No one assigned
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: dotnet Language: .NET 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@erikarvstedt @corngood @wegank