Skip to content

Checking python in PATH is the one cibuildwheel installed/expects#264

Merged
YannickJadoul merged 3 commits into
pypa:masterfrom
YannickJadoul:check-python-path
Feb 20, 2020
Merged

Checking python in PATH is the one cibuildwheel installed/expects#264
YannickJadoul merged 3 commits into
pypa:masterfrom
YannickJadoul:check-python-path

Conversation

@YannickJadoul

Copy link
Copy Markdown
Member

Fixes #261

@YannickJadoul

Copy link
Copy Markdown
Member Author

Once tests pass, two more things to consider:

  • Should we resolve the symlinks with os.path.realpath?
  • Should we throw an exception and/or print an error message? The main problem is that throwing a Python exception from the bash script in manylinux is not that easy...

@YannickJadoul YannickJadoul mentioned this pull request Feb 3, 2020
5 tasks
@YannickJadoul YannickJadoul force-pushed the check-python-path branch 4 times, most recently from ff42d50 to 11c1783 Compare February 3, 2020 15:20
@joerick

joerick commented Feb 3, 2020

Copy link
Copy Markdown
Contributor

This looks good to me. Probably worth adding an error message to save people raising issues. How about

cibuildwheel: python / pip available on PATH doesn't match our installed instance. If you have modified PATH, ensure that you don't overwrite cibuildwheel's entry or insert python/pip above it.

@YannickJadoul

YannickJadoul commented Feb 3, 2020

Copy link
Copy Markdown
Member Author

This looks good to me. Probably worth adding an error message to save people raising issues. How about

cibuildwheel: python / pip available on PATH doesn't match our installed instance. If you have modified PATH, ensure that you don't overwrite cibuildwheel's entry or insert python/pip above it.

Sure, sounds good. But just print it before test or assert, have it as the assert message, throw an exception, ...?

@joerick

joerick commented Feb 3, 2020

Copy link
Copy Markdown
Contributor

Whatever's convenient - assert message or just a simple print would be fine. As you mentioned, getting an exception out of the Linux script is hard, so echo error message; exit 1 might be the easiest. Or, I guess you could use a special return code from the bash script, if you wanted to get fancy! Up to you

@YannickJadoul

Copy link
Copy Markdown
Member Author

(currently waiting to do this until the build gets fixed; see #270)

@YannickJadoul YannickJadoul force-pushed the check-python-path branch 3 times, most recently from dfabdb8 to 72c9824 Compare February 19, 2020 17:35
@YannickJadoul

Copy link
Copy Markdown
Member Author

@joerick OK, ready for further review/merge, I think. Though it's probably best if this gets rebased onto #185, rather than the other way around.

@joerick joerick left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great. I'll let you decide the order you want to merge @YannickJadoul

@YannickJadoul

YannickJadoul commented Feb 19, 2020

Copy link
Copy Markdown
Member Author

@Czaki If you still want to have a quick look, I'll wait before merging this. (Not sure there is a lot to see, though. It's a pretty small PR. But still, if you spot something, do tell.)

@Czaki

Czaki commented Feb 19, 2020

Copy link
Copy Markdown
Contributor

Tomorrow.

@YannickJadoul

Copy link
Copy Markdown
Member Author

Tomorrow.

Great; no rush to get this merged!

@YannickJadoul YannickJadoul merged commit 65792bd into pypa:master Feb 20, 2020
@YannickJadoul

Copy link
Copy Markdown
Member Author

Thanks, @Czaki! :-)

@YannickJadoul YannickJadoul deleted the check-python-path branch February 20, 2020 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Changing PATH in CIBW_ENVIRONMENT can cause wrong Python to be used

3 participants