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

Add better python discovery #103007

Merged
merged 1 commit into from
Nov 1, 2022
Merged

Add better python discovery #103007

merged 1 commit into from
Nov 1, 2022

Conversation

albertlarsan68
Copy link
Member

The Microsoft Store version of Python installs itself as pythonM.m, with M being the major version and m the minor.

The x.ps1 script will now search for python executables whose command matches the regex python\d.
The \d at the end is to protect from using the pythonw versions, which do not work as standard python.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 13, 2022
@albertlarsan68
Copy link
Member Author

Somehow Rustbot didn't apply labels

@rustbot label +A-bootstrap

@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Oct 13, 2022
@Noratrieb Noratrieb mentioned this pull request Oct 13, 2022
Merged
jyn514
jyn514 reviewed Oct 13, 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.

I think we should have this same logic in the x bash script; the two should always be in sync.

x.ps1 Outdated Show resolved Hide resolved
x.ps1 Outdated Show resolved Hide resolved
@albertlarsan68
Copy link
Member Author

Thank you for reviewing.

I do not know how to do this in bash, being exclusively a Windows user.
The first check is still relevant: it matches things that are not matched by my additions.

jyn514
jyn514 reviewed Oct 13, 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.

Maybe you can do it with compgen? You can do version detection with BASH_VERSION to see if this is a bash shell or not. https://stackoverflow.com/questions/511683/bash-get-list-of-commands-starting-with-a-given-string

x.ps1 Outdated Show resolved Hide resolved
x.ps1 Outdated Show resolved Hide resolved
@albertlarsan68
Copy link
Member Author

albertlarsan68 commented Oct 13, 2022 •
edited
Loading

Hope these changes are correct.
I can not test the bash script, if someone could verify that this does the intended thing, it would be awesome.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 13, 2022
@matthiaskrgr
Rollup merge of rust-lang#103013 - Nilstrieb:patch-1, r=jyn514
300b8fb 
Add new bootstrap entrypoints to triagebot

They haven't been added yet, as seen in rust-lang#103007.

r? `@jyn514`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 13, 2022
@matthiaskrgr
Rollup merge of rust-lang#103013 - Nilstrieb:patch-1, r=jyn514
ea709c2 
Add new bootstrap entrypoints to triagebot

They haven't been added yet, as seen in rust-lang#103007.

r? ``@jyn514``
jyn514
jyn514 suggested changes Oct 27, 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 on the right track, thanks :)

x.ps1 Outdated Show resolved Hide resolved
x Outdated Show resolved Hide resolved
x Outdated Show resolved Hide resolved
x Outdated Show resolved Hide resolved
@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 27, 2022
@albertlarsan68
Copy link
Member Author

Applied suggestions, hope this still works, especially for the bash script.

@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 Oct 28, 2022
@jyn514
Copy link
Member

jyn514 commented Oct 30, 2022 •
edited
Loading

Ok, this looks good to me with albertlarsan68#1 merged :) If you could squash the commits afterwards that would also be nice: https://rustc-dev-guide.rust-lang.org/git.html

@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 30, 2022
@albertlarsan68
Add better python discovery
c83ddae 
`x.ps1` and `x` will now search for python executables like `python3.9`
and `python3.10.exe`
@albertlarsan68
Copy link
Member Author

Squashed and rebased.

@rustbot ready

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Oct 31, 2022
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 31, 2022
@albertlarsan68
Copy link
Member Author

Are those scripts used anywhere in CI?

@jyn514
Copy link
Member

jyn514 commented Oct 31, 2022

Are those scripts used anywhere in CI?

Yes, but because your new code comes last and bash is a dynamic language, it didn't fail on the invalid syntax because it found python3 first. Theoretically we could have a builder where only python3.9 is installed and test it that way, but it seems more trouble than it's worth, I think this is unlikely to break.

@jyn514
Copy link
Member

jyn514 commented Oct 31, 2022

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 31, 2022

📌 Commit c83ddae 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 Oct 31, 2022
@JohnTitor JohnTitor mentioned this pull request Nov 1, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 1, 2022
@bors
Auto merge of rust-lang#103829 - JohnTitor:rollup-o03nzr8, r=JohnTitor
dc05f60 
Rollup of 10 pull requests

Successful merges:

 - rust-lang#103007 (Add better python discovery)
 - rust-lang#103674 (Update note about unstable split-debuginfo flag.)
 - rust-lang#103692 (Add `walk_generic_arg`)
 - rust-lang#103749 (Reduce span of let else irrefutable_let_patterns warning)
 - rust-lang#103772 (better error for `rustc_strict_coherence` misuse)
 - rust-lang#103788 (Fix ICE in checking transmutability of NaughtyLenArray)
 - rust-lang#103793 (rustdoc: add margins to all impl-item toggles, not just methods)
 - rust-lang#103798 (interpret: move type_name implementation to an interpreter-independent helper file)
 - rust-lang#103799 (Remove generation of tuple struct fields in the search index)
 - rust-lang#103805 (Enable RUSTC_BOOTSTRAP for a few steps)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a6ac6b2 into rust-lang:master Nov 1, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 1, 2022
@albertlarsan68 albertlarsan68 deleted the better-python-discovery branch November 1, 2022 08:52
@albertlarsan68
Copy link
Member Author

Thanks for sticking along with me jyn514!

@jyn514
Copy link
Member

jyn514 commented Nov 1, 2022

Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 Awaiting requested review from jyn514

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.
6 participants
@albertlarsan68 @rust-highfive @jyn514 @bors @Mark-Simulacrum @rustbot