Skip to content

Conversation

@FKouhai
Copy link
Member

@FKouhai FKouhai commented Apr 22, 2025

Things done

taking over #398764

  • 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 This PR causes 1 package to rebuild on Darwin. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Apr 22, 2025
@nix-owners nix-owners bot requested review from ItsCrem and emilytrau April 22, 2025 20:17
@FKouhai FKouhai added the 8.has: package (update) This PR updates a package to a newer version label Apr 22, 2025
@aspauldingcode
Copy link
Contributor

aspauldingcode commented Apr 23, 2025

What is export $HOME= variable for

@wolfgangwalther
Copy link
Contributor

What is export $HOME= variable for

@FKouhai ftr, that's why the split in #398764 was strictly better: There was a commit message with an explanation for that change. Imho there are really 3 independent changes here, so the other PR was perfect. Ah, no it's 4 changes now, because you also added yourself as maintainer. That's certainly not related to the update.

@FKouhai
Copy link
Member Author

FKouhai commented Apr 23, 2025

What is export $HOME= variable for

@FKouhai ftr, that's why the split in #398764 was strictly better: There was a commit message with an explanation for that change. Imho there are really 3 independent changes here, so the other PR was perfect. Ah, no it's 4 changes now, because you also added yourself as maintainer. That's certainly not related to the update.

yeah after seeing the PR that updates the CONTRIBUTING I can see it a bit clearer now, when I made the review, based on reviews I was given and trying to keep the number of commits minimal. also I could make the argument that it would've been better to make a comment on the update rather making a separate commit and use its message to point out the why's imho it's a trade off because atomic commits can also be really meaningful specially when a change has to be reverted

@wolfgangwalther
Copy link
Contributor

@aspauldingcode

What is export $HOME= variable for

The original PR has more info on it. There was an error like this:

2025/04/15 04:48:10 Error: Could not create config directory: mkdir /homeless-shelter: permission denied
2025/04/15 04:48:10 Error: Could not create config directory: mkdir /homeless-shelter: permission denied
2025/04/15 04:48:10 Error: Could not create config directory: mkdir /homeless-shelter: permission denied

/homeless-shelter is the $HOME directory, but it doesn't exist and can't be created. That's why for this tool, we set HOME to a temporary directory, which is writable.

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.

Non-blocking comment. We can either look into that or merge as is.

Let me know how you would like to proceed.

@wolfgangwalther
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 400919


x86_64-linux

✅ 1 package built:
  • gowall

aarch64-linux

✅ 1 package built:
  • gowall

x86_64-darwin

✅ 1 package built:
  • gowall

aarch64-darwin

✅ 1 package built:
  • gowall

@wolfgangwalther wolfgangwalther merged commit ecefe9d into NixOS:master Apr 23, 2025
23 of 27 checks passed
@FKouhai FKouhai deleted the gowall branch April 23, 2025 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

8.has: package (update) This PR updates a package to a newer version 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants