Fix Windows Command::env("PATH") #87863
Conversation
|
r? @dtolnay (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
I think this need's to be beta nominated because the PR who introduce the bug is the the current beta. |
The impl Borrow<OsStr> for EnvKey lower in this file needs to be removed. That impl was the root cause. The impl is wrong because the Ord and Eq impls have different behavior between OsStr and EnvKey, which violates:
In particular
Eq,OrdandHashmust be equivalent for borrowed and owned values:x.borrow() == y.borrow()should give the same result asx == y.
dtolnay
added
beta-nominated
ChrisDenton
force-pushed
the
command-env-path-fix
branch
from
593b0eb
to
419902e
Compare
|
Ok I've removed the borrow and changed |
|
@bors r+ |
|
|
bors
added
S-waiting-on-bors
…tolnay
Fix Windows Command::env("PATH")
Fixes rust-lang#87859
|
|
bors
added
S-waiting-on-review
|
The failure looks like a spurious timeout or something? |
|
@bors retry |
bors
added
S-waiting-on-bors
…tolnay
Fix Windows Command::env("PATH")
Fixes rust-lang#87859
|
|
bors
added
S-waiting-on-review
|
Hm, this seems to have failed downloading. Third time lucky? |
|
@bors retry Failed to download action 'https://api.github.com/repos/actions/checkout/tarball/5a4ac9002d0be2fb38bd78e4b4dbde5606d7042f'. Error: The operation was canceled. |
bors
added
S-waiting-on-bors
| impl PartialOrd<str> for EnvKey { | ||
| fn partial_cmp(&self, other: &str) -> Option<cmp::Ordering> { | ||
| Some(self.cmp(&EnvKey::new(other))) | ||
| } | ||
| } | ||
| impl PartialEq<str> for EnvKey { | ||
| fn eq(&self, other: &str) -> bool { | ||
| if self.os_string.len() != other.len() { | ||
| false | ||
| } else { | ||
| self.cmp(&EnvKey::new(other)) == cmp::Ordering::Equal | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we really need these implementations? Silently allocating on every comparison seems like a bad idea.
The PartialEq implementation seems to be used only for == "PATH" and nothing else, so it's not that bad, but it might be good to improve that at some point. The result of that comparison also seems to only affect sys/unix and to be entirely unused on Windows.
The PartialOrd implementation, however, seems unnecessary. It all still compiles when removing it.
There was a problem hiding this comment.
Good points. I could remove those implementations and maybe I should add a #[cfg(not(windows))] to have_changed_path? So as to guard against the possibility of it accidentally being used in the future, or at least until a longer term fix is made.
|
For the backport: Would backporting just 593b0eb be enough? It looks to me like the |
|
Yeah, remove needs either an EnvKey or else a |
|
In that case we should probably just backport the PR as is.
That's probably a good idea, yes. |
…laumeGomez Rollup of 4 pull requests Successful merges: - rust-lang#87819 (Use a more accurate span on assoc types WF checks) - rust-lang#87863 (Fix Windows Command::env("PATH")) - rust-lang#87885 (Link to edition guide instead of issues for 2021 lints.) - rust-lang#87941 (Fix/improve rustdoc-js tool) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
m-ou-se
added
the
beta-accepted
Mark-Simulacrum
removed
the
beta-nominated
…ulacrum [beta] backports This PR rolls up a number of beta backports: * Split critical edge targeting the start block rust-lang#88124 * Make BuildHasher object safe rust-lang#88031 * Fix Windows Command::env("PATH") rust-lang#87863 * Do not ICE on HIR based WF check when involving lifetimes rust-lang#87811 * Update compiler_builtins to fix i128 shift/mul on thumbv6m rust-lang#87633
Fixes #87859