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

Add MixEnvironment to CmdRun #11490

Merged

Conversation

bryanhonof
Copy link
Member

@bryanhonof bryanhonof commented Sep 12, 2024

Motivation

Ref: #10338 (comment)

Context

I got a bit carried away when playing in MixEnvironment::setEnviron().
If those changes are too invasive, please let me know, I don't mind working on what was already there.

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@github-actions github-actions bot added the new-cli Relating to the "nix" command label Sep 12, 2024
@bryanhonof
Copy link
Member Author

I also wanted to add functional tests, but I wasn't quite sure how.
I did some ad-hoc testing locally, though.

(nix:nix-env) Bryans-MBP:nix bryanhonof$ nix --extra-experimental-features 'nix-command flakes' run nixpkgs#coreutils --add foo bar --unset TERM -- --coreutils-prog=printenv | rg foo=
foo=bar
(nix:nix-env) Bryans-MBP:nix bryanhonof$ nix --extra-experimental-features 'nix-command flakes' run nixpkgs#coreutils --add foo bar --unset TERM -- --coreutils-prog=printenv | rg TERM=
COLORTERM=truecolor
(nix:nix-env) Bryans-MBP:nix bryanhonof$ nix --extra-experimental-features 'nix-command flakes' run nixpkgs#coreutils --add foo bar -- --coreutils-prog=printenv | rg TERM=
COLORTERM=truecolor
TERM=xterm-ghostty
(nix:nix-env) Bryans-MBP:nix bryanhonof$ nix --extra-experimental-features 'nix-command flakes' run nixpkgs#coreutils --keep COLORTERM --keep TERM --add foo bar --ignore-environment -- --coreutils-prog=printenv
COLORTERM=truecolor
TERM=xterm-ghostty
foo=bar
__CF_USER_TEXT_ENCODING=0x1F5:0x0:0x0

This might be utterly wrong, especially since I'm not quite sure how __CF_USER_TEXT_ENCODING managed to still find its way into the env.

@@ -311,27 +311,42 @@ MixEnvironment::MixEnvironment() : ignoreEnvironment(false)
.labels = {"name"},
.handler = {[&](std::string s) { unset.insert(s); }},
});

addFlag({
.longName = "add",
Copy link
Member

Choose a reason for hiding this comment

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

--add is probably too generic, since there are potentially many kinds of objects that we might want to add. Maybe --set-var?

Copy link
Member Author

@bryanhonof bryanhonof Sep 12, 2024

Choose a reason for hiding this comment

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

Sounds good. Would it also make sense to rename unset to unset-var then?

Copy link
Member Author

@bryanhonof bryanhonof Sep 12, 2024

Choose a reason for hiding this comment

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

Is it reasonable to make those flags even more specific?
E.g.

--set-env-var vs --set-var
--unset-env-var vs --unset
--keep-host-env-var vs --keep
--ignore-host-env vs --ignore-environment

Copy link
Member

Choose a reason for hiding this comment

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

Maybe but that would be an incompatible change, so best to do that in a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Factored out the MixEnvironment-specific changes into #11494

@bryanhonof bryanhonof force-pushed the bryanhonof.add-mixenvironment-to-run branch from 8addedf to ce5a9c2 Compare September 12, 2024 12:30
@bryanhonof bryanhonof force-pushed the bryanhonof.add-mixenvironment-to-run branch from ce5a9c2 to 2226f98 Compare September 12, 2024 16:27
@edolstra edolstra merged commit 11452ce into NixOS:master Sep 12, 2024
14 checks passed
infinisil added a commit to NixOS/SC-election-2024 that referenced this pull request Sep 20, 2024
Some of these are a bit after the specified time period, but I also believe some of these items are difficult to gauge how much "impact" they had.

Helped organize, host, and run, the Summer of Nix Lecture Series 2022

- https://www.youtube.com/playlist?list=PLt4-_lkyRrOMWyp5G-m_d1wtTcbBaOxZk
- NixOS/infra#213

Helped organize, and host infra for, the Summer of Nix Lecture Series 2023

- https://www.youtube.com/playlist?list=PLt4-_lkyRrOPcBuz_tjm6ZQb-6rJjU3cf
- NixOS/infra#240

Helped organize, host, and was responsible for livestreaming infra during, NixCon Paris 2022

- https://www.youtube.com/playlist?list=PLgknCdxP89ReD6gxl755B6G_CI65z4J2e

Maintenance of some nixpkgs packages

- NixOS/nixpkgs#340223 (contribution after 2024-05-01)
- NixOS/nixpkgs#290084
- NixOS/nixpkgs#170089

Organized, and assembled a team for the FOSDEM 2023 Nix/NixOS Devroom

- https://discourse.nixos.org/t/fosdem-2023-nix-and-nixos-devroom/23133

Organizer & sole maintainer of the Config Management Camp Nix track

- https://discourse.nixos.org/t/config-management-camp-2023-ghent/23455
- https://discourse.nixos.org/t/config-management-camp-2024-ghent/33852
- https://discourse.nixos.org/t/cfgmgmtcamp-2025-is-looking-for-nix-presentations/51658 (contribution after 2024-05-01)

Public speaking & spreading awareness of Nix/NixOS

- https://youtu.be/gUjvnZ9ZwMs?si=nDiZTCpQj53wwq8P
- https://www.youtube.com/watch?v=hNcYPH5Q_pA&t=862s

The occasional dabble into the Nix C++ code base

- NixOS/nix#11494 (contribution after 2024-05-01)
- NixOS/nix#11490 (contribution after 2024-05-01)
- NixOS/nix#11489 (contribution after 2024-05-01)
- NixOS/nix#11349 (contribution after 2024-05-01)
- NixOS/nix#11241 (contribution after 2024-05-01)
- NixOS/nix#9557
- NixOS/nix#8788
- NixOS/nix#8212
- NixOS/nix#5147

General evangelism

Pretty much every event I attend, I'm talking about Nix, showing off Nix/NixOS, and just trying to get people to see how awesome this tool is.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-cli Relating to the "nix" command
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants