Skip to content

Fix x finding Python on Windows #104350

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Nov 15, 2022
Merged

Fix x finding Python on Windows #104350

merged 1 commit into from
Nov 15, 2022

Conversation

SparkyPotato
Copy link
Contributor

x searches through the path for {dir}/python{2|3}?, but this fails on Windows because the appropriate path is {dir}/python.exe.

This PR adds the expected .exe extension on Windows while searching.

@rustbot
Copy link
Collaborator

rustbot commented Nov 13, 2022

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 13, 2022
@tschuett
Copy link

How about std::env::consts::EXE_EXTENSION?

@SparkyPotato
Copy link
Contributor Author

Great idea, I used that instead.

@albertlarsan68
Copy link
Member

@rustbot label +A-bootstrap

@albertlarsan68
Copy link
Member

@rustbot ping

@rustbot
Copy link
Collaborator

rustbot commented Nov 13, 2022

Error: Parsing ping command in comment failed: ...' ping' | error: no team specified at >| ''...

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@Dylan-DPC Dylan-DPC added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Nov 13, 2022
@albertlarsan68 albertlarsan68 mentioned this pull request Nov 13, 2022
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 13, 2022
@matthiaskrgr
Rollup merge of rust-lang#104365 - albertlarsan68:add-x-tool-to-triag…
39ff9d2 
…ebot, r=Mark-Simulacrum

Add x tool to triagebot

Assign the A-bootstrap label when a pr modifies the x tool.

Happened in rust-lang#104350.
@Mark-Simulacrum
Copy link
Member

r? @jyn514 -- I think this seems okay but you might have the python discovery mess more in cache than I.

@rustbot rustbot assigned jyn514 and unassigned Mark-Simulacrum Nov 14, 2022
jyn514
jyn514 approved these changes Nov 14, 2022
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

This looks good, thanks.

FWIW I am not enthused about maintaining 4 separate entry points. I think at some point we may want to make this an even smaller shim that just invokes x.ps1 or ./x. But that doesn't need to block this change.

@jyn514
Copy link
Member

jyn514 commented Nov 14, 2022

@bors r+ rollup

Oh, the other reason I'm not a fan of this shim is it has no upgrade path, the changes don't have an effect unless people rerun cargo install.

@bors
Copy link
Collaborator

bors commented Nov 14, 2022

📌 Commit 56dfb70 has been approved by jyn514

It is now in the queue for this repository.

@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 Nov 14, 2022
@messense
Copy link
Contributor

messense commented Nov 14, 2022 •
edited
Loading

IMO, on Windows it's better to also try the py.exe launcher: https://docs.python.org/3/using/windows.html#launcher.

The default installation options from python.org don't include installing Python on the PATH, but it does include installing the py launcher on the PATH.

@jyn514
Copy link
Member

jyn514 commented Nov 14, 2022

@messense I am not willing to take large changes to the x tool at this time, unless they're to use the x.ps1 or ./x scripts.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 14, 2022
@matthiaskrgr
Rollup merge of rust-lang#104350 - SparkyPotato:fix-x-wrapper, r=jyn514
5e2785f 
Fix x finding Python on Windows

`x` searches through the path for `{dir}/python{2|3}?`, but this fails on Windows because the appropriate path is `{dir}/python.exe`.

This PR adds the expected `.exe` extension on Windows while searching.
@matthiaskrgr matthiaskrgr mentioned this pull request Nov 14, 2022
Closed
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 14, 2022
@matthiaskrgr
Rollup merge of rust-lang#104350 - SparkyPotato:fix-x-wrapper, r=jyn514
8ab46a1 
Fix x finding Python on Windows

`x` searches through the path for `{dir}/python{2|3}?`, but this fails on Windows because the appropriate path is `{dir}/python.exe`.

This PR adds the expected `.exe` extension on Windows while searching.
@matthiaskrgr matthiaskrgr mentioned this pull request Nov 14, 2022
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Nov 14, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 15, 2022
@bors
Auto merge of rust-lang#104418 - matthiaskrgr:rollup-y4i6xjc, r=matth…
101e182 
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#101967 (Move `unix_socket_abstract` feature API to `SocketAddrExt`.)
 - rust-lang#102470 (Stabilize const char convert)
 - rust-lang#104223 (Recover from function pointer types with generic parameter list)
 - rust-lang#104229 (Don't print full paths in overlap errors)
 - rust-lang#104294 (Don't ICE with inline const errors during MIR build)
 - rust-lang#104332 (Fixed some `_i32` notation in `maybe_uninit`’s doc)
 - rust-lang#104349 (fix some typos in comments)
 - rust-lang#104350 (Fix x finding Python on Windows)
 - rust-lang#104356 (interpret: make check_mplace public)
 - rust-lang#104364 (rustdoc: Resolve doc links in external traits having local impls)
 - rust-lang#104378 (Bump chalk to v0.87)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit aa29a8b into rust-lang:master Nov 15, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 15, 2022
@jyn514 jyn514 mentioned this pull request Dec 17, 2022
Closed
@SparkyPotato SparkyPotato deleted the fix-x-wrapper branch January 1, 2023 08:55
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
@matthiaskrgr
Rollup merge of rust-lang#104365 - albertlarsan68:add-x-tool-to-triag…
3991db0 
…ebot, r=Mark-Simulacrum

Add x tool to triagebot

Assign the A-bootstrap label when a pr modifies the x tool.

Happened in rust-lang#104350.
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@jyn514 jyn514 jyn514 approved these changes

Assignees

@jyn514 jyn514

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
1.67.0
Development

Successfully merging this pull request may close these issues.
9 participants
@SparkyPotato @rustbot @tschuett @albertlarsan68 @Mark-Simulacrum @jyn514 @bors @messense @Dylan-DPC