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

Command: also print removed env vars #114379

Merged
merged 6 commits into from Sep 22, 2023
Merged

Command: also print removed env vars #114379

merged 6 commits into from Sep 22, 2023

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Aug 2, 2023

There is no real shell syntax for unsetting an env var so easily, so we have to make one up. But we already do that for showing the 'program' name so I hope that's okay here, too. No strong opinion on what that should look like, I went with unset(VAR_NAME) for now.

@rustbot
Copy link
Collaborator

rustbot commented Aug 2, 2023

r? @joshtriplett

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 2, 2023
@the8472
Copy link
Member

the8472 commented Aug 2, 2023

https://unix.stackexchange.com/a/388849 suggests wrapping a command in a subshell. It's gnarly syntax but it would be copy-pastable.

(unset ENV_VAR; exec command)

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@joshtriplett
Copy link
Member

joshtriplett commented Aug 2, 2023 •
edited

I do feel like being able to easily copy-paste the command is a desirable feature. Not worth substantially sacrificing readability for, though. And the subshell syntax, while easily copy-pasteable, is...not exactly appealing. But those seem like the two options here.

@the8472
Copy link
Member

the8472 commented Aug 2, 2023

The subshell syntax could be applied conditionally only when an environment variable was unset. For adding variables the prefix approach is sufficient.

@RalfJung
Copy link
Member Author

RalfJung commented Aug 3, 2023

We already have cd which alters the surrounding environment, so I don't think we need a subshell just for unset.

Or conversely, if we want to worry about subshells, we should also do it for cd.

@joshtriplett
Copy link
Member

@RalfJung Good call. Having it as an executable command seems like the important part; the subshell doesn't seem worth it, and as you said we already alter the surrounding environment. If someone pastes in the command, they can see that it has unset and cd lines in it, and if they don't want that they can wrap it themselves easily enough.

@joshtriplett
Copy link
Member

Since this has a noticeable change in user-visible behavior, let's doublecheck consensus:

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Aug 3, 2023 •
edited by m-ou-se

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 3, 2023
@the8472
Copy link
Member

the8472 commented Aug 3, 2023

I don't think this really needs an FCP since the behavior was changed #97176 not too long ago without much fanfare, but ok

RalfJung
RalfJung commented Aug 3, 2023
View reviewed changes
library/std/src/process.rs
Comment on lines +790 to 793
/// After calling [`Command::env_clear`], the iterator from [`Command::get_envs`] will be
/// empty.
Copy link
Member Author

Choose a reason for hiding this comment

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

While at it I fixed a typo here. At least I am fairly sure this was a typo?

@RalfJung
Copy link
Member Author

RalfJung commented Aug 3, 2023

@the8472 do you know if there is an easy way to express "clear the entire environment" in unix commands?

I'm thinking maybe I want to represent that as well. It could be something like clearenv && FOO=bar cmd args? That doesn't actually work but it should be clear enough, hopefully?

@the8472
Copy link
Member

the8472 commented Aug 3, 2023

since clearing the environment means that no variable needs to be explicitly unset anymore (since none are set) it can be expressed this way:

$ env -i FOO=BAR BAZ=QUUX cat /proc/self/environ
FOO=BARBAZ=QUUX

@RalfJung
Copy link
Member Author

RalfJung commented Aug 3, 2023

Ah, that is neat.

@RalfJung
Copy link
Member Author

RalfJung commented Aug 3, 2023

In fact we could use env -u FOO BAR="val" prog to represent unsetting in a way that doesn't alter the current shell...

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Aug 17, 2023
@rfcbot
Copy link

rfcbot commented Aug 17, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Aug 17, 2023
@rustbot rustbot added the O-unix Operating system: Unix-like label Aug 24, 2023
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. to-announce Announce this issue on triage meeting and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Aug 27, 2023
@rfcbot
Copy link

rfcbot commented Aug 27, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Sep 1, 2023
@RalfJung
Copy link
Member Author

RalfJung commented Sep 5, 2023

r? libs

@rustbot rustbot assigned m-ou-se and unassigned joshtriplett Sep 5, 2023
@RalfJung @mrcnski
fix typo
98c94ec 
Co-authored-by: Marcin S. <marcin@realemail.net>
@m-ou-se
Copy link
Member

m-ou-se commented Sep 21, 2023

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 21, 2023

📌 Commit 98c94ec has been approved by m-ou-se

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 21, 2023
@RalfJung RalfJung added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 21, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Sep 22, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 22, 2023
@bors
Auto merge of rust-lang#116054 - matthiaskrgr:rollup-3pusno6, r=matth…
03c199a 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#114379 (Command: also print removed env vars)
 - rust-lang#116034 (add UI test for delimiter errors)
 - rust-lang#116036 (tests/ui: Split large_moves.rs and move to lint/large_assignments)
 - rust-lang#116038 (Fall back to _SC_NPROCESSORS_ONLN if sched_getaffinity returns an empty mask)
 - rust-lang#116039 (Account for nested `impl Trait` in TAIT)
 - rust-lang#116041 (Add note to `is_known_rigid`)
 - rust-lang#116049 (give FutureIncompatibilityReason variants more explicit names)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 9887dfa into rust-lang:master Sep 22, 2023
11 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 22, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 22, 2023
@rust-timer
Unrolled build for rust-lang#114379
daa60e5 
Rollup merge of rust-lang#114379 - RalfJung:command-removed-env-vars, r=m-ou-se

Command: also print removed env vars

There is no real shell syntax for unsetting an env var so easily, so we have to make one up. But we already do that for showing the 'program' name so I hope that's okay here, too. No strong opinion on what that should look like, I went with `unset(VAR_NAME)` for now.
@RalfJung RalfJung deleted the command-removed-env-vars branch September 22, 2023 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrcnski mrcnski mrcnski left review comments

@coolreader18 coolreader18 coolreader18 left review comments

Assignees

@m-ou-se m-ou-se

Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. O-unix Operating system: Unix-like relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.74.0
Development

Successfully merging this pull request may close these issues.

None yet