Skip to content

Add unstable env::set and env::remove - #92431

Closed
sugar700 wants to merge 1 commit into
rust-lang:masterfrom
sugar700:unsafe_env
Closed

Add unstable env::set and env::remove#92431
sugar700 wants to merge 1 commit into
rust-lang:masterfrom
sugar700:unsafe_env

Conversation

@sugar700

@sugar700 sugar700 commented Dec 30, 2021

Copy link
Copy Markdown
Contributor

I think it makes sense to have an alternative to set_var and remove_var on nightly that is actually unsafe. Name of this function needs bike-shedding, but I imagine this could be done during stabilization. See https://internals.rust-lang.org/t/synchronized-ffi-access-to-posix-environment-variable-functions/15475?u=xfix for context.

@rust-highfive

Copy link
Copy Markdown
Contributor

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 30, 2021
@camelid camelid added the T-libs-api [DEPRECATED; DO NOT USE] label Jan 1, 2022
Comment thread library/std/src/env.rs Outdated
Comment thread library/std/src/env.rs Outdated
Comment thread library/std/src/env.rs Outdated
Comment thread library/std/src/env.rs Outdated
@joshtriplett joshtriplett added the I-libs-api-nominated [DEPRECATED; DO NOT USE] label Jan 1, 2022
@sugar700
sugar700 requested a review from joshtriplett January 2, 2022 11:37
Comment thread library/std/src/env.rs
Comment on lines +307 to +308
/// This function must not be called in the presence of concurrent threads that
/// may simultaneously read or write the environment. Also, it's not allowed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It should be noted that this isn't the case on Windows.

@Mark-Simulacrum Mark-Simulacrum removed the I-libs-api-nominated [DEPRECATED; DO NOT USE] label Jan 13, 2022
@Mark-Simulacrum

Copy link
Copy Markdown
Member

The libs-api team discussed this at last weeks' meeting. There were a couple concerns with this PR as-is:

  • We want to push for migration to future-proof safe alternative APIs, which don't modify the environment (these need to be designed and implemented)
    • In particular, a migration to the unsafe functions makes a subsequent migration to the safe alternative APIs less likely.
  • We want to avoid using up these names (which are relatively nice) just to provide equivalent (but unsafe) APIs
    • In the future, a Rust-only environment or other alternatives to the env modification/reading design space may open up, and in those cases we may want these names.

To that effect, the next steps in this space are (roughly):

  • Set of PRs for key rust-lang maintained APIs which currently are controllable only via the environment (RUST_BACKTRACE in std, RUST_LOG in log, potentially a few others)
  • Once these land, blog.rust-lang.org post which asks library authors to work on their own libraries and provide alternative APIs to set configuration. Point at how std's APIs were designed/modified as a case study for a good approach. (in particular, most cases will want to keep the option to set environment variables, but also provide an alternative API for programmatic access).
  • Mark set_var/remove_var (at least) as unsafe, likely with a custom #[rustc_safe_deprecated] or so attribute which allows us to reuse the same names and eases migration.
    • At this time, we'll want to consider the "risk" of users using these APIs within existing unsafe blocks (and so not knowing about the new obligation), but that seems relatively unlikely to me. Crater may provide some partial data here.
  • Some time later, likely several years, examine whether we can provide safe APIs (e.g., because libc's and similar have added new APIs we can rely on).

Would you be interested in helping to drive the first step here? I believe there may already be some design discussions (or PRs) open. Otherwise, I plan to invest in the coming week(s) in pushing this plan forward.

@sugar700

sugar700 commented Jan 18, 2022

Copy link
Copy Markdown
Contributor Author

I think I like the idea of #[rustc_safe_deprecated] more, as such I will be closing this pull request.

As far RUST_LOG environment variable is concerned, it's worth noting that this isn't log thing but rather env_logger thing, and env_logger already provides an API that allows providing custom RUST_LOG: env_logger::init_from_env. Of course, it has an issue - this makes it much more difficult to configure an env_logger using a library that isn't aware of env_logger such as dotenv. For instance, currently it's possible to configure env_logger with env variables retrieved from .env file like this:

dotenv::dotenv().ok();
env_logger::init();

This works because dotenv can set RUST_LOG environment variable if it's specified in a file. I imagine this is something that dotenv crate author will need to figure out the solution for (marking dotenv as unsafe, having a private copy of environment, maybe providing some integrations with popular crates that use environment variables (dotenv::env_logger_init maybe), who knows).

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

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api [DEPRECATED; DO NOT USE]

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants