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

Build Python wheels with tag manylinux2010 #20282

Merged
merged 5 commits into from Sep 19, 2019
Merged

Build Python wheels with tag manylinux2010 #20282

merged 5 commits into from Sep 19, 2019

Conversation

lidizheng
Copy link
Contributor

Let's see if the tests are happy.

@lidizheng lidizheng added lang/Python release notes: yes Indicates if PR needs to be in release notes labels Sep 17, 2019
gnossen
gnossen approved these changes Sep 17, 2019
View reviewed changes
Copy link
Contributor

@gnossen gnossen left a comment

Choose a reason for hiding this comment

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

👍

tools/dockerfile/grpc_artifact_python_manylinux2010_x64/Dockerfile
RUN /opt/python/cp37-cp37m/bin/pip install cython

####################################################
# Install auditwheel with fix for namespace packages
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For infrastructure code, I think pin the version of auditwheel is better than always use the latest version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. The question is whether the install from a git repository is still necessary or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will investigate and see if I can remove all of the hack, or part of the hack 👌

@lidizheng lidizheng mentioned this pull request Sep 17, 2019
Closed
@jtattermusch
Copy link
Contributor

jtattermusch commented Sep 18, 2019 •
edited

Would this by any chance help with the problem described here: #20100 (comment) (it's preventing us from upgrading third_party/boringssl)?

Update: it seems this would help because manylinux2010 has gcc8.3.1

$ docker run --rm=true -it quay.io/pypa/manylinux2010_x86_64 bash
[root@b00620af072f /]# gcc --version
gcc (GCC) 8.3.1 20190311 (Red Hat 8.3.1-3)

@lidizheng lidizheng mentioned this pull request Sep 18, 2019
Merged
@lidizheng
Copy link
Contributor Author

@jtattermusch As commented in #20100, this will be helpful eventually. But for now, we won't be able to remove manylinux1.

@lidizheng
Copy link
Contributor Author

@jtattermusch PTALA. All failures are not-related to this PR.

gnossen
gnossen reviewed Sep 18, 2019
View reviewed changes
tools/run_tests/artifacts/build_artifact_python.sh
@@ -79,12 +79,12 @@ ${SETARCH_CMD} "${PYTHON}" tools/distrib/python/grpcio_tools/setup.py bdist_whee
if [ "$GRPC_BUILD_MANYLINUX_WHEEL" != "" ]
then
for wheel in dist/*.whl; do
"${AUDITWHEEL}" show "$wheel" | tee /dev/stderr | grep \"manylinux1
"${AUDITWHEEL}" show "$wheel" | tee /dev/stderr | grep -E -w 'manylinux(1|2010)_(x86_64|i686)'
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice change!

jtattermusch
jtattermusch approved these changes Sep 19, 2019
View reviewed changes
Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

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

LGTM.

@lidizheng
Copy link
Contributor Author

Known failures: #20306 #20307 #20308 #20309 #20310

@lidizheng lidizheng merged commit 74f8da0 into grpc:master Sep 19, 2019
@veblush veblush mentioned this pull request Oct 10, 2019
Merged
@lock lock bot locked as resolved and limited conversation to collaborators Dec 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gnossen gnossen gnossen approved these changes

@jtattermusch jtattermusch jtattermusch approved these changes

@apolcyn apolcyn Awaiting requested review from apolcyn apolcyn is a code owner

@nicolasnoble nicolasnoble Awaiting requested review from nicolasnoble

Assignees

@gnossen gnossen

@jtattermusch jtattermusch

Labels
area/infra lang/Python release notes: yes Indicates if PR needs to be in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lidizheng @jtattermusch @gnossen @grpc-kokoro