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

haskell-modules/generic-builder.nix: use mktemp instead of TMPDIR #171685

Conversation

matthewbauer
Copy link
Member

Using $TMPDIR here is problematic because it is not always cleared at
the end of each build, for instance when using "nix-shell --run
genericBuild". This can cause confusing errors when a nix-shell build
is trying to pull in dependencies from a previous build since it tries
to use older package conf files.

To fix, we can just use mktemp which will guarantee us a clean
directory for each build. Should have no effect in nix-build, but will
fix a common issue with using generic-builder in nix-shell.

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/)
  • 22.05 Release Notes (or backporting 21.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.

Using $TMPDIR here is problematic because it is not always cleared at
the end of each build, for instance when using "nix-shell --run
genericBuild". This can cause confusing errors when a nix-shell build
is trying to pull in dependencies from a previous build since it tries
to use older package conf files.

To fix, we can just use mktemp which will guarantee us a clean
directory for each build. Should have no effect in nix-build, but will
fix a common issue with using generic-builder in nix-shell.
Copy link
Contributor

@9999years 9999years left a comment

Choose a reason for hiding this comment

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

LGTM

@sternenseemann
Copy link
Member

We should then maybe also deliberate cleaning up the temporary directory if nix-shell is to be an explicit use case.

@Artturin
Copy link
Member

Artturin commented May 9, 2022

Needs to target staging
Check contributing.md for how to do that

@sternenseemann
Copy link
Member

sternenseemann commented May 10, 2022 via email

Copy link
Member

@sternenseemann sternenseemann 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 this is okay, it pollutes the temporary directory in case of interactive usage, but this was also the case before this change.

@@ -338,9 +338,10 @@ stdenv.mkDerivation ({
echo "Build with ${ghc}."
${optionalString (isLibrary && hyperlinkSource) "export PATH=${hscolour}/bin:$PATH"}

setupPackageConfDir="$TMPDIR/setup-package.conf.d"
builddir="$(mktemp -d)"
Copy link
Member

Choose a reason for hiding this comment

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

Can we set a meaningful template here (e.g. mentioning nix, pname,..?)

@sternenseemann sternenseemann changed the base branch from master to haskell-updates May 10, 2022 11:07
@maralorn maralorn merged commit 10dcd7d into NixOS:haskell-updates May 21, 2022
@maralorn
Copy link
Member

Thank you!

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.

5 participants