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.

Sign up for GitHub

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

Jump to bottom

Fix Windows Command::env("PATH") #87863

Merged
merged 1 commit into from
Aug 12, 2021
Merged

Fix Windows Command::env("PATH") #87863

merged 1 commit into from
Aug 12, 2021

Conversation

ChrisDenton
Copy link
Member

Fixes #87859

@rust-highfive
Copy link
Collaborator

r? @dtolnay

(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 Aug 8, 2021
@Urgau
Copy link
Member

Urgau commented Aug 8, 2021

I think this need's to be beta nominated because the PR who introduce the bug is the the current beta.

dtolnay
dtolnay requested changes Aug 8, 2021
View reviewed changes
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

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, Ord and Hash must be equivalent for borrowed and owned values: x.borrow() == y.borrow() should give the same result as x == y.

@dtolnay dtolnay added beta-nominated Nominated for backporting to the compiler in the beta channel. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 8, 2021
@ChrisDenton
Copy link
Member Author

Ok I've removed the borrow and changed sys_common::process::CommandEnv to work with EnvKey instead of OsStr/OsString. I wonder if it might ultimately be worth Windows using its own implementation of CommandEnv.

@dtolnay
Copy link
Member

dtolnay commented Aug 8, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Aug 8, 2021

📌 Commit 419902e has been approved by dtolnay

@bors 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 Aug 8, 2021
@bors
Copy link
Contributor

bors commented Aug 9, 2021

⌛ Testing commit 419902e with merge 03e3dd097a4919dea6607213cffdbf33e65c0121...

@bors
Copy link
Contributor

bors commented Aug 9, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 9, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@ChrisDenton
Copy link
Member Author

The failure looks like a spurious timeout or something?

@dtolnay
Copy link
Member

dtolnay commented Aug 9, 2021

@bors retry

@bors 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 Aug 9, 2021
@bors
Copy link
Contributor

bors commented Aug 9, 2021

⌛ Testing commit 419902e with merge 32994af1891b398c863fe81080d08df924e0a069...

@bors
Copy link
Contributor

bors commented Aug 9, 2021

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 9, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@ChrisDenton
Copy link
Member Author

Hm, this seems to have failed downloading. Third time lucky?

@ehuss
Copy link
Contributor

ehuss commented Aug 10, 2021

@bors retry

Failed to download action 'https://api.github.com/repos/actions/checkout/tarball/5a4ac9002d0be2fb38bd78e4b4dbde5606d7042f'. Error: The operation was canceled.

@bors 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 Aug 10, 2021
m-ou-se
m-ou-se reviewed Aug 11, 2021
View reviewed changes
library/std/src/sys/windows/process.rs
Comment on lines +108 to +121
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
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@m-ou-se
Copy link
Member

m-ou-se commented Aug 11, 2021

For the backport: Would backporting just 593b0eb be enough? It looks to me like the remove() function also had a bug that was discovered by removing the Borrow implementation, so no, correct?

@ChrisDenton
Copy link
Member Author

ChrisDenton commented Aug 11, 2021 •
edited
Loading

Yeah, remove needs either an EnvKey or else a Borrow that has the same ordering as EnvKey. I could limit the allocation to right before self.vars.remove? That would fix the immediate problem and shouldn't have too much impact.

@m-ou-se
Copy link
Member

m-ou-se commented Aug 11, 2021

In that case we should probably just backport the PR as is.

I wonder if it might ultimately be worth Windows using its own implementation of CommandEnv.

That's probably a good idea, yes.

@GuillaumeGomez GuillaumeGomez mentioned this pull request Aug 12, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 12, 2021
@bors
Auto merge of rust-lang#87963 - GuillaumeGomez:rollup-e54sbez, r=Guil…
4498e30 
…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
@bors bors merged commit cc54fda into rust-lang:master Aug 12, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 12, 2021
@ChrisDenton ChrisDenton mentioned this pull request Aug 15, 2021
Closed
@m-ou-se m-ou-se added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Aug 18, 2021
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 27, 2021
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.56.0, 1.55.0 Aug 27, 2021
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Aug 27, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 28, 2021
@bors
Auto merge of rust-lang#88400 - Mark-Simulacrum:beta-next, r=Mark-Sim…
27e88d3 
…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
seritools added a commit to rust9x/rust that referenced this pull request Dec 29, 2023
@seritools
Fall back to old env arg behavior if CompareStringOrdinal is not av…
02bd38f 
…ailable

See rust-lang#85270 and rust-lang#87863
mbilker pushed a commit to mbilker/rust that referenced this pull request Jul 17, 2024
@seritools @mbilker
Fall back to old env arg behavior if CompareStringOrdinal is not av…
b0a801e 
…ailable

See rust-lang#85270 and rust-lang#87863
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@m-ou-se m-ou-se m-ou-se left review comments

@dtolnay dtolnay dtolnay approved these changes

Assignees

@dtolnay dtolnay

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.55.0
Development

Successfully merging this pull request may close these issues.

Command.env not working since nightly-2021-07-05
10 participants
@ChrisDenton @rust-highfive @Urgau @dtolnay @bors @rust-log-analyzer @ehuss @m-ou-se @Mark-Simulacrum @rustbot