Skip to content

make shared_helpers exe function work for both cygwin and non-cygwin hosts #141374

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jeremyd2019
Copy link
Contributor

On Cygwin, it needs to not append .exe, because /proc/self/exe (and therefore std::env::current_exe) does not include the .exe extension, breaking bootstrap's rustc wrapper. On hosts other than Cygwin, it does need to append .exe because the file really does have a .exe extension, and non-Cygwin hosts won't be doing the same filename rewriting that Cygwin does when looking for a file X but finding only X.exe in its place.

Arising from discussion in #140154 (review)
@mati865 @Berrysoft

@rustbot
Copy link
Collaborator

rustbot commented May 22, 2025

r? @onur-ozkan

rustbot has assigned @onur-ozkan.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels May 22, 2025
@rust-log-analyzer

This comment has been minimized.

@onur-ozkan
Copy link
Member

Thank you!

Could you please squash your commits into single commit? I am also not familiar with the Windows environment, so passing this to person that I kind a know that they are using Windows.

r? @jieyouxu

@rustbot rustbot assigned jieyouxu and unassigned onur-ozkan May 22, 2025
@jeremyd2019
Copy link
Contributor Author

sure, that was the point of the fixup! commit. I had a minor mishap pasting the change into github's web editor, and had an extra space in there.

Copy link
Contributor

@mati865 mati865 left a comment

Choose a reason for hiding this comment

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

How about leaving a comment there? I feel like this condition will not be obvious, even to people familiar with Cygwin.

@jieyouxu
Copy link
Member

Yeah, please leave a comment. When I reviewed the original PR the change looked correct.

@jieyouxu
Copy link
Member

@rustbot author

@rustbot rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 22, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 22, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label May 22, 2025
…hosts

On Cygwin, it needs to not append .exe, because /proc/self/exe (and
therefore std::env::current_exe) does not include the .exe extension,
breaking bootstrap's rustc wrapper.  On hosts other than Cygwin, it
*does* need to append .exe because the file really does have a .exe
extension, and non-Cygwin hosts won't be doing the same filename
rewriting that Cygwin does when looking for a file X but finding only
X.exe in its place.
@jeremyd2019
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 22, 2025
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-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants