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

Fix setting PATH during linkage on windows-gnu #42481

Merged
merged 1 commit into from
Jun 8, 2017
Merged

Conversation

brson
Copy link
Contributor

@brson brson commented Jun 6, 2017

This makes the behavior almost exactly the same as before the VS2017 patch, except that on MSVC builds the host bin path is no longer added to PATH. I am not sure that's actually necessary on any platform.

r? @alexcrichton

Fixes #42422

@brson brson added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 6, 2017
@brson
Copy link
Contributor Author

brson commented Jun 6, 2017

Oh actually I miststated - this is not like previously, now this is only frobbing PATH at all for linker invocation on windows, whereas before it was doing it on all platforms. Again we think it only matters on windows.

// On windows-gnu (msvc is handled separately), we bundle
// gcc and other tools with the official releases. This is
// how the other tools that gcc itself needs are located.
vec![("PATH".into(), command_path(sess, linker.map(PathBuf::from)))]
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem right. The second parameter to command_path is added to the end of %Path% and before #42225 it was only used by MSVC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes. I suspected that path had no effect. I'll update.

@brson
Copy link
Contributor Author

brson commented Jun 6, 2017

Ok, now the patch is written so that all non-msvc platforms set up the PATH during linking as previously. The command_path function only takes one argument now.

@alexcrichton alexcrichton added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jun 7, 2017
@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Jun 7, 2017

📌 Commit e8689c7 has been approved by alexcrichton

@aidanhs aidanhs added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jun 7, 2017
@bors
Copy link
Contributor

bors commented Jun 8, 2017

⌛ Testing commit e8689c7 with merge 76eea74...

bors added a commit that referenced this pull request Jun 8, 2017
Fix setting PATH during linkage on windows-gnu

This makes the behavior almost exactly the same as before the VS2017 patch, except that on MSVC builds the host bin path is no longer added to PATH. I am not sure that's actually necessary on any platform.

r? @alexcrichton

Fixes #42422
@bors
Copy link
Contributor

bors commented Jun 8, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 76eea74 to master...

@bors bors merged commit e8689c7 into rust-lang:master Jun 8, 2017
@brson brson mentioned this pull request Jun 12, 2017
@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 12, 2017
bors added a commit that referenced this pull request Jun 12, 2017
[beta] Fix setting PATH during linkage on windows-gnu

Beta backport of #42481
bors added a commit that referenced this pull request Jun 15, 2017
Beta next

- #42521
- #42512
- #42482
- #42481
- #42480

r? @nikomatsakis remember to untag 'beta-nominated' on linked issues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants