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 the ability to see inherited envs on Command #110327

Closed

Conversation

schneems
Copy link
Contributor

@schneems schneems commented Apr 14, 2023

Add the ability to see inherited envs on Command

This is a reference implementation for one of the solution "sketches" in rust-lang/libs-team#194.

Problem statement(s)

  • Problem 1: Cannot observe effects of Command::env_clear
  • Problem 2: The "capture" logic has no exposed single source of truth

Problem 1 is a capability problem (I can't work around this). Problem 2 is a usability and stability problem.

Problem 1: Cannot observe effects of Command::env_clear

As documented in my merged PR, calling env_clear will cause the "capture" logic to skip inheriting from the parent process. While a developer can set this value, they cannot programmatically observe if it has been set (though they can manually observe it via "alternative" debug formatting {:#?}).

The end goals of programmatically observing that effect is listed below in "Motivation".

This problem prevents me from reproducing the capture logic in a library where I cannot observe if env_clear has been called. Even if this problem is solved, a related problem is detailed below.

Problem 2: The "capture" logic has no exposed single source of truth

Even if we could directly observe the effects of calling env_clear, every developer who needed this information would need to reproduce this "capture" logic

pub fn capture(&self) -> BTreeMap<EnvKey, OsString> {
let mut result = BTreeMap::<EnvKey, OsString>::new();
if !self.clear {
for (k, v) in env::vars_os() {
result.insert(k.into(), v);
}
}
for (k, maybe_v) in &self.vars {
if let &Some(ref v) = maybe_v {
result.insert(k.clone(), v.clone());
} else {
result.remove(k);
}
}
result
}
and hope that it does not change or that their implementation does not diverge.

If the outcome of this logic is exposed, then it will:

  • Solve problem 1
  • Prevent divergent reimplementation of "capture" logic

[sketch] Expose captured env logic via a new capture_envs method on Command

We could add a method that exposes that information directly:

let command = Command::new("ls");

let envs: HashMap<OsString, OsString> = command.capture_envs().collect();

The name capture matches internal naming concepts and helps to reinforce that the value method will return a point in time value.

This sketch would solve problems 1 and 2. I'm open to name suggestions.

@rustbot
Copy link
Collaborator

rustbot commented Apr 14, 2023

r? @thomcc

(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 Apr 14, 2023
@bors
Copy link
Contributor

bors commented Apr 14, 2023

☔ The latest upstream changes (presumably #110331) made this pull request unmergeable. Please resolve the merge conflicts.

@rust-log-analyzer

This comment has been minimized.

@schneems schneems force-pushed the schneems/expose-command-env-clear branch from 0a3a825 to 98c5aaa Compare April 21, 2023 15:55
@schneems schneems changed the title Add ability to see inherited envs on Command Add the ability to see inherited envs on Command Apr 21, 2023
@rust-log-analyzer

This comment has been minimized.

@schneems schneems force-pushed the schneems/expose-command-env-clear branch from 98c5aaa to e841217 Compare April 21, 2023 17:19
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@thomcc thomcc added the S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. label May 5, 2023
@Dylan-DPC Dylan-DPC removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 22, 2023
@bors
Copy link
Contributor

bors commented Sep 9, 2023

☔ The latest upstream changes (presumably #114590) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 9, 2023
@Dylan-DPC Dylan-DPC removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 21, 2023
This is a reference implementation for one of the solution "sketches" in rust-lang/libs-team#194.

## Problem statement(s)

- Problem 1: Cannot observe effects of `Command::env_clear`
- Problem 2: The "capture" logic has no exposed single source of truth

Problem 1 is a capability problem (I can't work around this). Problem 2 is a usability and stability problem.

### Problem 1: Cannot observe effects of `Command::env_clear`

As documented in [my merged PR](#109272), calling `env_clear` will cause the "capture" logic to skip inheriting from the parent process. While a developer can set this value, they cannot programmatically observe if it has been set (though they can manually observe it via "alternative" debug formatting `{:#?}`).

The end goals of programmatically observing that effect is listed below in "Motivation".

This problem prevents me from reproducing the capture logic in a library where I cannot observe if `env_clear` has been called. Even if this problem is solved, a related problem is detailed below.

### Problem 2: The "capture" logic has no exposed single source of truth

Even if we could directly observe the effects of calling `env_clear`, every developer who needed this information would need to reproduce this "capture" logic https://github.com/rust-lang/rust/blob/3a5c8e91f094bb1cb1346651fe3512f0b603d826/library/std/src/sys_common/process.rs#L36-L51 and hope that it does not change or that their implementation does not diverge.

If the outcome of this logic is exposed, then it will:

- Solve problem 1
- Prevent divergent reimplementation of "capture" logic

### [sketch] Expose captured env logic via a new `capture_envs` method on `Command`

We could add a method that exposes that  information directly:

```rust
let command = Command::new("ls");

let envs: HashMap<OsString, OsString> = command.capture_envs().collect();
```

The name `capture` matches internal naming concepts and helps to reinforce that the value method will return a point in time value.

This sketch would solve problems 1 and 2. I'm open to name suggestions.
@schneems schneems force-pushed the schneems/expose-command-env-clear branch from 9f35c5b to a2776c1 Compare November 12, 2023 20:47
@rustbot rustbot added O-unix Operating system: Unix-like O-windows Operating system: Windows labels Nov 12, 2023
@rust-log-analyzer

This comment has been minimized.

@schneems schneems force-pushed the schneems/expose-command-env-clear branch from a2776c1 to c9e8d86 Compare November 12, 2023 21:05
@rust-log-analyzer

This comment has been minimized.

@schneems schneems force-pushed the schneems/expose-command-env-clear branch from c9e8d86 to 4266fea Compare November 13, 2023 15:29
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
GITHUB_ACTION=__run_7
GITHUB_ACTIONS=true
GITHUB_ACTION_REF=
GITHUB_ACTION_REPOSITORY=
GITHUB_ACTOR=schneems
GITHUB_API_URL=https://api.github.com
GITHUB_BASE_REF=master
GITHUB_ENV=/home/runner/work/_temp/_runner_file_commands/set_env_41c58c97-347f-4de6-bc1c-6bbcd40e57f9
GITHUB_EVENT_NAME=pull_request
GITHUB_EVENT_NAME=pull_request
GITHUB_EVENT_PATH=/home/runner/work/_temp/_github_workflow/event.json
GITHUB_GRAPHQL_URL=https://api.github.com/graphql
GITHUB_HEAD_REF=schneems/expose-command-env-clear
GITHUB_JOB=pr
GITHUB_PATH=/home/runner/work/_temp/_runner_file_commands/add_path_41c58c97-347f-4de6-bc1c-6bbcd40e57f9
GITHUB_REF=refs/pull/110327/merge
GITHUB_REF_NAME=110327/merge
GITHUB_REF_PROTECTED=false
---
GITHUB_SERVER_URL=https://github.com
GITHUB_SHA=58dcadd3f7c871d6813804181d7894eca09d820e
GITHUB_STATE=/home/runner/work/_temp/_runner_file_commands/save_state_41c58c97-347f-4de6-bc1c-6bbcd40e57f9
GITHUB_STEP_SUMMARY=/home/runner/work/_temp/_runner_file_commands/step_summary_41c58c97-347f-4de6-bc1c-6bbcd40e57f9
GITHUB_TRIGGERING_ACTOR=schneems
GITHUB_WORKFLOW_REF=rust-lang/rust/.github/workflows/ci.yml@refs/pull/110327/merge
GITHUB_WORKFLOW_SHA=58dcadd3f7c871d6813804181d7894eca09d820e
GITHUB_WORKSPACE=/home/runner/work/rust/rust
GOROOT_1_19_X64=/opt/hostedtoolcache/go/1.19.13/x64
---
    Checking miniz_oxide v0.7.1
    Checking object v0.32.0
    Checking hashbrown v0.14.2
    Checking addr2line v0.21.0
error[E0277]: the trait bound `core::option::Option<(OsString, OsString)>: core::convert::From<core::option::Option<(EnvKey, OsString)>>` is not satisfied
    |
    |
124 |         self.iter.next().into()
    |                          ^^^^ the trait `core::convert::From<core::option::Option<(EnvKey, OsString)>>` is not implemented for `core::option::Option<(OsString, OsString)>`
    = help: the following other types implement trait `core::convert::From<T>`:
    = help: the following other types implement trait `core::convert::From<T>`:
              <core::option::Option<&'a T> as core::convert::From<&'a core::option::Option<T>>>
              <core::option::Option<&'a mut T> as core::convert::From<&'a mut core::option::Option<T>>>
              <core::option::Option<T> as core::convert::From<T>>
    = note: required for `core::option::Option<(EnvKey, OsString)>` to implement `core::convert::Into<core::option::Option<(OsString, OsString)>>`
For more information about this error, try `rustc --explain E0277`.
error: could not compile `std` (lib) due to previous error
Build completed unsuccessfully in 0:00:14
  local time: Mon Nov 13 15:34:11 UTC 2023

@bors
Copy link
Contributor

bors commented Jan 13, 2024

☔ The latest upstream changes (presumably #117285) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 13, 2024
@Dylan-DPC Dylan-DPC removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 8, 2024
@schneems schneems closed this by deleting the head repository Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-unix Operating system: Unix-like O-windows Operating system: Windows S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants