Add Command environment variable inheritance docs #109272

schneems · 2023-03-17T18:01:49Z

The interaction between the environment variable methods can be confusing. Specifically env_clear and remove_env have a side effects not mentioned: they disable inheriting environment variables from the parent process. I wanted to fully document this behavior as well as explain relevant edge cases in each of the Command env methods.

This is further confused by the return of get_envs which will return key/None if remove_env has been used, but an empty iterator if env_clear has been called. Or a non-empty iterator if env_clear was called and later explicit mappings are added. Currently there is no way (that I'm able to find) of observing whether or not the internal env_clear=true been toggled on the Command struct via its public API.

Ultimately environment variable mappings can be in one of several states:

Explicitly set value (via envs / env) will take precedence over parent mapping
Not explicitly set, will inherit mapping from parent
Explicitly removed via remove_env, this single mapping will not inherit from parent
Implicitly removed via env_clear, no mappings will inherit from parent

I tried to represent this in the relevant sections of the docs.

This is my second-ever doc PR (whoop!). I'm happy to take specific or general doc feedback. Also happy to explain the logic behind any changes or additions I made.

rustbot · 2023-03-17T18:01:56Z

r? @cuviper

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

rustbot · 2023-03-17T18:01:59Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

The interaction between the environment variable methods can be confusing. Specifically `env_clear` and `remove_env` have a side effects not mentioned: they disable inheriting environment variables from the parent process. I wanted to fully document this behavior as well as explain relevant edge cases in each of the `Command` env methods. This is further confused by the return of `get_envs` which will return key/None if `remove_env` has been used, but an empty iterator if `env_clear` has been called. Or a non-empty iterator if `env_clear` was called and later explicit mappings are added. Currently there is no way (that I'm able to find) of observing whether or not the internal `env_clear=true` been toggled on the `Command` struct via its public API. Ultimately environment variable mappings can be in one of several states: - Explicitly set value (via `envs` / `env`) will take precedence over parent mapping - Not explicitly set, will inherit mapping from parent - Explicitly removed via `remove_env`, this single mapping will not inherit from parent - Implicitly removed via `env_clear`, no mappings will inherit from parent I tried to represent this in the relevant sections of the docs. This is my second ever doc PR (whoop!). I'm happy to take specific or general doc feedback. Also happy to explain the logic behind any changes or additions I made.

rustbot · 2023-03-17T18:02:17Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

library/std/src/process.rs

cuviper · 2023-03-23T20:41:32Z

AFAICT you've accurately captured the current behavior, but I'm going to flip this over to API review, since documentation implies a stronger promise about maintaining that exactly.

r? libs-api

Amanieu · 2023-04-08T19:35:02Z

r=me once @cuviper's comment is addressed

Fixes documentation. I wrote `env_clear` when I meant `env_remove`. Good catch. Co-authored-by: Josh Stone <cuviper@gmail.com>

schneems · 2023-04-09T14:22:54Z

Sorry for the delay. I applied @cuviper's suggestion, it was a good catch. Thanks for the reviews and all your work on rust ❤️

Let me know if this needs anything else.

schneems · 2023-04-14T16:37:21Z

r=me once @cuviper's comment is addressed

I didn't realize this was shorthand for literal syntax. But now I see it above. Not sure If I can trigger it but trying now:

r? @Amanieu

rustbot · 2023-04-14T16:37:22Z

Could not assign reviewer from: Amanieu.
User(s) Amanieu are either the PR author or are already assigned, and there are no other candidates.
Use r? to specify someone else to assign.

cuviper · 2023-04-14T16:41:33Z

The intent of saying "r=me when..." is that anyone with review privileges can submit it on their behalf. But somebody also has to notice that it's ready, sorry about that! So...

@bors r=Amanieu

bors · 2023-04-14T16:41:36Z

📌 Commit e612d78 has been approved by Amanieu

It is now in the queue for this repository.

cuviper · 2023-04-14T19:24:35Z

@bors rollup -- just docs here

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#108687 (Reformulate `point_at_expr_source_of_inferred_type` to be more accurate) - rust-lang#109272 (Add Command environment variable inheritance docs) - rust-lang#109947 (Add links from `core::cmp` derives to their traits) - rust-lang#110110 (Use `Display` in top-level example for `PanicInfo`) - rust-lang#110154 (Fix typos in library) - rust-lang#110244 (Remove some unneeded imports / qualified paths) - rust-lang#110328 ([rustdoc] Add explanations for auto-disambiguation when an intra doc link is resolved to a proc-macro and a trait at the same time) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned cuviper Mar 17, 2023

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 Mar 17, 2023

schneems force-pushed the schneems/add-docs-to-command-env-methods branch from 5faaf33 to 75657d5 Compare March 17, 2023 18:02

cuviper reviewed Mar 23, 2023

View reviewed changes

library/std/src/process.rs Outdated Show resolved Hide resolved

rustbot assigned Amanieu and unassigned cuviper Mar 23, 2023

schneems mentioned this pull request Mar 23, 2023

Fix:Introduce an interface to expose the current Command captured env var logic rust-lang/libs-team#194

Open

ChrisDenton added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 27, 2023

Apply suggestions from code review

e612d78

Fixes documentation. I wrote `env_clear` when I meant `env_remove`. Good catch. Co-authored-by: Josh Stone <cuviper@gmail.com>

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 Apr 14, 2023

matthiaskrgr mentioned this pull request Apr 14, 2023

Rollup of 7 pull requests #110331

Merged

bors merged commit 13790be into rust-lang:master Apr 14, 2023

rustbot added this to the 1.70.0 milestone Apr 14, 2023

schneems mentioned this pull request Apr 21, 2023

Add the ability to see inherited envs on Command #110327

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Command environment variable inheritance docs #109272

Add Command environment variable inheritance docs #109272

schneems commented Mar 17, 2023 •

edited

Loading

rustbot commented Mar 17, 2023

rustbot commented Mar 17, 2023

rustbot commented Mar 17, 2023

cuviper commented Mar 23, 2023

Amanieu commented Apr 8, 2023

schneems commented Apr 9, 2023

schneems commented Apr 14, 2023

rustbot commented Apr 14, 2023

cuviper commented Apr 14, 2023

bors commented Apr 14, 2023

cuviper commented Apr 14, 2023

Add Command environment variable inheritance docs #109272

Add Command environment variable inheritance docs #109272

Conversation

schneems commented Mar 17, 2023 • edited Loading

rustbot commented Mar 17, 2023

rustbot commented Mar 17, 2023

rustbot commented Mar 17, 2023

cuviper commented Mar 23, 2023

Amanieu commented Apr 8, 2023

schneems commented Apr 9, 2023

schneems commented Apr 14, 2023

rustbot commented Apr 14, 2023

cuviper commented Apr 14, 2023

bors commented Apr 14, 2023

cuviper commented Apr 14, 2023

schneems commented Mar 17, 2023 •

edited

Loading