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

Choose the version of python at runtime (portable version) #80625

Merged
merged 1 commit into from
Jan 15, 2021
Merged

Choose the version of python at runtime (portable version) #80625

merged 1 commit into from
Jan 15, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Jan 2, 2021 •
edited
Loading

r? @Mark-Simulacrum

Fixed version of #80585. The goal is to avoid giving 'error: python3 required' when downloading LLVM from CI and instead default to python3 where possible.

This has some minor overhead when you have python as python2, but almost nothing compared to actually running the build.

@jyn514 jyn514 added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself labels Jan 2, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 2, 2021
@jyn514 jyn514 force-pushed the python-what-python branch from b31222c to 9725665 Compare January 2, 2021 18:48
@jyn514
Copy link
Member Author

jyn514 commented Jan 2, 2021

Err actually this doesn't close #71818 because it will still give an error if python doesn't exist (i.e. you only have python3). It just avoids #80581 a little more frequently.

@CraftSpider
Copy link
Contributor

CraftSpider commented Jan 2, 2021 •
edited
Loading

On Windows, the conventional way to run different python versions is with the py runner (Relevant Docs), and no python3 exe is added to the PATH. Not a problem, just a note on windows compat.

Edit: In case anyone wants, here's how pytype finds an executable for a platform: link

@jyn514
Copy link
Member Author

jyn514 commented Jan 2, 2021

Ok, this now tries to be smart about the hashbang again. @nagisa I tested in fish this time and it works, but always happy to get more testing :) This shouldn't affect windows at all since people seem to be using python x.py there instead of running it directly. I would expect running it directly to work because .py is associated with the python interpreter, but I don't have a windows machine to test.

@CraftSpider
Copy link
Contributor

Just tested locally in powershell and command, the shebang line is messing things up for me:
image
image

@jyn514
Copy link
Member Author

jyn514 commented Jan 2, 2021

@CraftSpider is there more to the error below? /bin/sh x.py looks like the right command line - I guess it breaks since there's no /bin on Windows?

@CraftSpider
Copy link
Contributor

That's the whole error in both cases. And I agree, I think the issue is just that the path doesn't exist. I can probably even test that if you want, by adding a program at C:\bin\bash

@jyn514 jyn514 force-pushed the python-what-python branch from 155dd5a to 9725665 Compare January 2, 2021 19:47
@jyn514
Copy link
Member Author

jyn514 commented Jan 2, 2021

So it turns out that py x.py will only work on Windows

  1. If the shebang is exactly /usr/bin/env python (or possibly python2/3)
  2. If there is no shebang

Doing anything else gives an error that /usr/bin/env doesn't exist. Removing the shebang breaks fish. So I reverted the second commit.

@jyn514 jyn514 closed this Jan 2, 2021
@jyn514 jyn514 reopened this Jan 2, 2021
@joshtriplett
Copy link
Member

Looks reasonable to me.

Mark-Simulacrum
Mark-Simulacrum reviewed Jan 4, 2021
View reviewed changes
x.py Outdated
# interpreter.
if sys.version_info.major < 3:
try:
os.execvp("python3", ["python3"] + sys.argv)
Copy link
Member

Choose a reason for hiding this comment

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

Can we try to use the alternate forms (e.g., I take it Windows would prefer something like py --version 3 ... perhaps?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know how useful that would be, since download-ci-llvm currently only works on linux. I can add it just in case #77084 ever gets fixed if you like.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, I'm not able to test on windows, so I'm a little hesitant to add code that I haven't tried.

@Mark-Simulacrum Mark-Simulacrum 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 Jan 4, 2021
frewsxcv
frewsxcv approved these changes Jan 4, 2021
View reviewed changes
Copy link
Member

@frewsxcv frewsxcv left a comment

Choose a reason for hiding this comment

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

Thanks!

@jyn514 jyn514 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 Jan 7, 2021
Mark-Simulacrum
Mark-Simulacrum reviewed Jan 9, 2021
View reviewed changes
@Mark-Simulacrum Mark-Simulacrum 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 Jan 9, 2021
@jyn514 jyn514 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 Jan 10, 2021
@cuviper cuviper mentioned this pull request Jan 15, 2021
Closed
@Mark-Simulacrum
Copy link
Member

r=me with commits squashed

@Mark-Simulacrum Mark-Simulacrum removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 15, 2021
@Mark-Simulacrum Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jan 15, 2021
@jyn514
Choose the version of python at runtime (portable version)
c8cac2a 
- Try `py -3` first for windows compatibility
- Fall back to `python3` if `py` doesn't work
@jyn514 jyn514 force-pushed the python-what-python branch from 146c841 to c8cac2a Compare January 15, 2021 02:01
@jyn514
Copy link
Member Author

jyn514 commented Jan 15, 2021

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Jan 15, 2021

📌 Commit c8cac2a has been approved by Mark-Simulacrum

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 15, 2021
@bors
Copy link
Contributor

bors commented Jan 15, 2021

⌛ Testing commit c8cac2a with merge 18ec4a9...

@bors
Copy link
Contributor

bors commented Jan 15, 2021

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 18ec4a9 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 15, 2021
@bors bors merged commit 18ec4a9 into rust-lang:master Jan 15, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 15, 2021
@jyn514 jyn514 deleted the python-what-python branch January 15, 2021 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

@frewsxcv frewsxcv frewsxcv approved these changes

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
A-contributor-roadblock Area: Makes things more difficult for new contributors to rust itself merged-by-bors This PR was explicitly merged by bors. 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.51.0
Development

Successfully merging this pull request may close these issues.
8 participants
@jyn514 @CraftSpider @joshtriplett @Mark-Simulacrum @bors @frewsxcv @rust-highfive @rustbot