Skip to content

versionCheckHook: add versionCheckDontIgnoreEnvironment parameter#403971

Merged
drupol merged 3 commits intoNixOS:stagingfrom
drupol:push-qqqlvqmqyqqx
May 12, 2025
Merged

versionCheckHook: add versionCheckDontIgnoreEnvironment parameter#403971
drupol merged 3 commits intoNixOS:stagingfrom
drupol:push-qqqlvqmqyqqx

Conversation

@drupol
Copy link
Contributor

@drupol drupol commented May 3, 2025 •
edited
Loading

This parameter is set to false by default to make sure that the current hook behaviour is not altered.
When explicitly set to true, the flag --ignore-environment will not be added to the env command.

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.

@drupol drupol changed the title versionCheckHook: add versionCheckIgnoreEnvironment parameter versionCheckHook: add versionCheckDontIgnoreEnvironment parameter May 3, 2025
@drupol drupol force-pushed the push-qqqlvqmqyqqx branch from 746b52d to 23da3a2 Compare May 3, 2025 17:32
@drupol drupol mentioned this pull request May 3, 2025
Merged
13 tasks
@github-actions github-actions bot added 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin 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: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. labels May 3, 2025
@nix-owners nix-owners bot requested review from 06kellyjac, eihqnh and yurrriq May 3, 2025 17:38
@Defelo
Copy link
Member

Defelo commented May 3, 2025 •
edited
Loading

I think it would be great if there was an option to specify a list of environment variables to pass to the version command (e.g. only HOME), so you wouldn't have to allow either all or none of them.

Also, it looks like this PR should go to staging 😉

@drupol
Copy link
Contributor Author

drupol commented May 3, 2025

I don't think this is possible with env. Perhaps we could do that in another PR ?

@Defelo
Copy link
Member

Defelo commented May 3, 2025 •
edited
Loading

There doesn't seem to be a flag in env for that, but I think you could just do something like env --ignore-environment HOME="$HOME" USER="$USER" .... Of course you would need to get the actual names of the variables from a variable itself...

Perhaps we could do that in another PR ?

Sure, this is already a really nice improvement!

@drupol drupol marked this pull request as draft May 3, 2025 18:01
@drupol drupol force-pushed the push-qqqlvqmqyqqx branch from 23da3a2 to b1fe673 Compare May 3, 2025 18:49
@drupol drupol changed the base branch from master to staging May 3, 2025 18:49
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: haskell General-purpose, statically typed, purely functional programming language 8.has: changelog This PR adds or changes release notes 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: emacs Text editor 6.topic: vim Advanced text editor 6.topic: erlang General-purpose, concurrent, functional high-level programming language 6.topic: R R is a programming language for statistical computing and data visualization. 6.topic: vscode A free and versatile code editor that supports almost every major programming language. 6.topic: lib The Nixpkgs function library 6.topic: teams Relating to team creation, updates, other management actions and removed 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: haskell General-purpose, statically typed, purely functional programming language 8.has: changelog This PR adds or changes release notes labels May 3, 2025
@drupol drupol marked this pull request as ready for review May 3, 2025 19:05
jpotier

This comment was marked as outdated.

Sign in to view

@drupol drupol force-pushed the push-qqqlvqmqyqqx branch 2 times, most recently from 435be41 to 0508dca Compare May 3, 2025 20:57
@nix-owners nix-owners bot requested review from matthewpi and tricktron May 3, 2025 21:06
@drupol drupol mentioned this pull request May 3, 2025
Merged
13 tasks
@drupol drupol force-pushed the push-qqqlvqmqyqqx branch 2 times, most recently from 89c6899 to 44b6bf8 Compare May 4, 2025 05:36
@nix-owners nix-owners bot requested a review from piotrkwiecinski May 4, 2025 05:41
@drupol drupol force-pushed the push-qqqlvqmqyqqx branch from 44b6bf8 to 1486eeb Compare May 4, 2025 08:54
@piotrkwiecinski
Copy link
Contributor

piotrkwiecinski commented May 6, 2025

I would like to see a version with whitelisted/passed env vars, but I agree this is a good improvement in meantime.

drupol added 3 commits May 6, 2025 21:18
@drupol
versionCheckHook: add versionCheckDontIgnoreEnvironment parameter
a4b5a2c 
This parameter is set to `true` by default to make sure that the current behaviour of the hook is not altered.
When explicitly set to `false`, the flag `--ignore-environment` will not be added to the `env` command.
@drupol
cargo-seek: add versionCheckDontIgnoreEnvironment = true;
d28271d
@drupol
crc: add versionCheckDontIgnoreEnvironment = true;
c3b3bec
@drupol drupol force-pushed the push-qqqlvqmqyqqx branch from 1486eeb to c3b3bec Compare May 6, 2025 19:18
eihqnh
eihqnh approved these changes May 12, 2025
View reviewed changes
Copy link
Contributor

@eihqnh eihqnh left a comment

Choose a reason for hiding this comment

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

Cool.When can this be merged?

@drupol drupol merged commit 7f2bebc into NixOS:staging May 12, 2025
28 checks passed
@drupol drupol deleted the push-qqqlvqmqyqqx branch May 12, 2025 06:58
@nixpkgs-ci
Copy link
Contributor

nixpkgs-ci bot commented May 12, 2025

Backport failed for staging-24.11, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin staging-24.11
git worktree add -d .worktree/backport-403971-to-staging-24.11 origin/staging-24.11
cd .worktree/backport-403971-to-staging-24.11
git switch --create backport-403971-to-staging-24.11
git cherry-pick -x a4b5a2ce4a5c6b88cabe1b2d444a5b06d4664ebb d28271d7ef04ec3caad2e9a244c2eb06ea48bd7a c3b3bec2cba1df1180b1d5e6a9197683881ce668

This was referenced May 18, 2025
Draft
Merged
@Defelo Defelo mentioned this pull request May 28, 2025
Merged
13 tasks
@Defelo
Copy link
Member

Defelo commented May 28, 2025

I think it would be great if there was an option to specify a list of environment variables to pass to the version command (e.g. only HOME), so you wouldn't have to allow either all or none of them.

#411609

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

Reviewers

@yurrriq yurrriq Awaiting requested review from yurrriq

@06kellyjac 06kellyjac Awaiting requested review from 06kellyjac

@teutat3s teutat3s Awaiting requested review from teutat3s

@nshalman nshalman Awaiting requested review from nshalman

@dywedir dywedir Awaiting requested review from dywedir

@Br1ght0ne Br1ght0ne Awaiting requested review from Br1ght0ne

@matthewpi matthewpi Awaiting requested review from matthewpi

@tricktron tricktron Awaiting requested review from tricktron

@piotrkwiecinski piotrkwiecinski Awaiting requested review from piotrkwiecinski

2 more reviewers

@jpotier jpotier jpotier approved these changes

@eihqnh eihqnh eihqnh approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@drupol @Defelo @piotrkwiecinski @jpotier @eihqnh