Provide clearenv() #1185
Provide clearenv() #1185
Conversation
| /// environment access in the program is via `std::env`, but the requirement on | ||
| /// thread safety must still be upheld. | ||
| pub unsafe fn clearenv() -> Result<()> { | ||
| let ret; |
There was a problem hiding this comment.
As a matter of style, I suggest:
let ret = cfg_if! {
if something {
libc::clearenv()
} else {
0
}
};There was a problem hiding this comment.
I tried that first, but it looks like that's not allowed with cfg_if!. It generates a lot of compilation errors; the most relevant looks like:
error: macro expansion ignores token `$crate` and any following
--> <::cfg_if::cfg_if macros>:23:37
|
23 | { @ __identity $ ($ tokens) * } $ crate :: cfg_if !
| ^^^^^^^
|
::: src/env.rs:28:15
|
28 | let ret = cfg_if! {
| _______________-
29 | | if #[cfg(any(target_os = "fuchsia",
30 | | target_os = "wasi",
31 | | target_env = "wasi",
... |
43 | | }
44 | | };
| | -
| | |
| |_____caused by the macro expansion here
| help: you might be missing a semicolon here: `;`
|
= note: the usage of `cfg_if!` is likely invalid in expression context
= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)
|
The build failed on nightly. Could you please investigate, @jgallagher ? |
|
@asomers Squashed down to one commit. I'm not sure what to make of the nightly failure; I can't reproduce it locally, and the error message itself is pretty bizarre. Do you know off hand if there's a description of the travis environment somewhere? I could try spinning up a VM that more closely matches it. |
|
That test was run on the latest nightly. It may well be a bug in Rust. Could you try updating your nightly toolchain and reproducing? |
|
I had already tried updating nightly without any luck, but I was just able to reproduce it. I hadn't noticed the Do you want me to squash that bump into this PR? |
|
Yes please! Good sleuthing. |
|
Looks like that did the trick. 👍 |
| @@ -18,7 +18,7 @@ exclude = [ | |||
| [dependencies] | |||
| libc = { version = "0.2.60", features = [ "extra_traits" ] } | |||
| bitflags = "1.1" | |||
| cfg-if = "0.1.2" | |||
| cfg-if = "0.1.10" | |||
There was a problem hiding this comment.
Did you actually verify that 0.1.10 is the minimum version that works?
There was a problem hiding this comment.
I think I mentioned before that the error is still there on 0.1.9. I didn't try every version in between 0.1.2 and 0.1.9; it seems unlikely any of those work work if the same error is in both 0.1.2 and 0.1.9, but I guess it's not impossible. Is there an impact to jumping to 0.1.10?
There was a problem hiding this comment.
Well, given that 0.1.10 comes immediately after 0.1.9, I'd say that confirms it. Too bad that the cfg-if crate doesn't maintain a CHANGELOG.
1185: Provide clearenv() r=asomers a=jgallagher Maybe closes #481. "Maybe" for a couple reasons that are discussed on that issue: * The issue mentions adding the other environment-variable-related functions (`setenv`, `getenv`, etc.), but I omitted those because they're already covered by `std::env` AFAICT, plus they're wildly unthreadsafe. It looks like `std::env` avoids the thread unsafety via a library-level lock and a warning to be careful when using ffi functions that might modify the environment concurrently. * I opted to return `Error::UnsupportedOperation` in the (impossible?) case of `libc::clearenv()` returning non-zero instead of having the function return a `Result<(), ()>`. The latter seems like it would be onerous (since every other possibly-failing function in nix returns a `nix::Result`), but if you'd prefer a different error return I'm not going to put up any argument. Co-authored-by: John Gallagher <jgallagher@bignerdranch.com>
Build succeeded
|
Maybe closes #481.
"Maybe" for a couple reasons that are discussed on that issue:
setenv,getenv, etc.), but I omitted those because they're already covered bystd::envAFAICT, plus they're wildly unthreadsafe. It looks likestd::envavoids the thread unsafety via a library-level lock and a warning to be careful when using ffi functions that might modify the environment concurrently.Error::UnsupportedOperationin the (impossible?) case oflibc::clearenv()returning non-zero instead of having the function return aResult<(), ()>. The latter seems like it would be onerous (since every other possibly-failing function in nix returns anix::Result), but if you'd prefer a different error return I'm not going to put up any argument.