-
Notifications
You must be signed in to change notification settings - Fork 259
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
Give ownership of the wheel files to the current user instead of root on Linux #66
Conversation
Nevermind bitrise (only @joerick has access there, but maybe it would be nice to temporarily disable?), but I've restarted the failed Travis build |
Furthermore:
(cfr. https://www.python.org/downloads/release/python-337/) So should we maybe just drop 3.3 support overall? |
Dropping 3.3 support overall is to be considered yes. However, this PR only aims at dropping support on linux because builds are failing right now. Windows Python 3.3 builds are not broken (yet). |
Wait, because these are actually three PRs in one, if I see that correctly: Python 3.3 for manylinux, the wheel ownership, and simplifying the configuration for Travis? I'm quite happy with the content, but is there a part that's urgent to merge? If not, I'll take out those changes. Otherwise, we can just wait for @joerick to review things as well. |
.travis.yml
Outdated
@@ -5,21 +5,20 @@ matrix: | |||
# Linux Python 2 | |||
- sudo: required | |||
language: python | |||
python: "2.7" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"2.7" -> 2.7?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well,
I copy/pasted this from travis-ci doc https://docs.travis-ci.com/user/languages/python/#Specifying-Python-versions
If it has no impact I'm happy to change it
.travis.yml
Outdated
|
||
# Linux Python 3 | ||
- sudo: required | ||
language: python | ||
python: "3.4" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 3.4, and not the latest version? Or just 3, then? (All in all, the different minor versions of Python will be tested inside the cibuildwheel
tests anyway)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to travis-ci docs, https://docs.travis-ci.com/user/languages/python/#Specifying-Python-versions, a major.minor shall be provided
This is the python version running cibuildwheel driver, not the python used to build wheels (so, cibuildwheel itself is not tested with every minor python versions). If support is dropped for 3.3 for building wheels, I found somewhat logical not to use it either to run cibuildwheel and thus, use the next supported version which is python 3.4
.travis.yml
Outdated
before_install: | ||
- sudo apt-get -qq update | ||
- sudo apt-get install -y python3-pip | ||
- "PYTHON=python" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If distinction between python2
and python3
is not needed, we can just get rid of this environment variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still needed for macOS which uses python2
and python3
(might not show in the diff)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, should've looked better!
069ec14
to
bd233cc
Compare
bd233cc
to
7036134
Compare
@YannickJadoul, PR rebased. |
46f51f5
to
ce59d58
Compare
cibuildwheel/linux.py
Outdated
@@ -95,6 +95,7 @@ def build(project_dir, package_name, output_dir, test_command, test_requires, be | |||
|
|||
# we're all done here; move it to output | |||
mv "$delocated_wheel" /output | |||
chown {uid}:{gid} /output/$(basename $delocated_wheel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since all variable expansions before are also quoted, can we still change this to "/output/$(basename "$delocated_wheel")"
?
ce59d58
to
d186b0b
Compare
@YannickJadoul, change requested has been committed |
Give ownership of the wheel files to the current user instead of root on Linux, this also allows to cleanup the
.travis.yml
file a bit.