Skip to content

Make build environment manylinux2014 compatible #52

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

Closed
wants to merge 11 commits into from

Conversation

nitins17
Copy link
Member

@nitins17 nitins17 commented Dec 1, 2021

Modifies the build environment to be manylinux2014 compatible

Changes include:

  • Added devtoolset-8 to the Dockerfile
  • Created a config option for manylinux2014 in {cpu,gpu}.bazelrc
  • Added manylinux2014 compatible glibc and libstdc++ libraries in devtoolset-8 (/dt8)

@@ -37,6 +37,10 @@ build --action_env=GCC_HOST_COMPILER_PATH="/dt7/usr/bin/gcc"
build --action_env=LD_LIBRARY_PATH="/usr/local/cuda:/usr/local/cuda/lib64:/usr/local/cuda/extras/CUPTI/lib64:/usr/local/tensorrt/lib"
build [email protected]_manylinux2010-cuda11.2-cudnn8.1-tensorrt7.2_config_cuda//crosstool:toolchain

# Set up a manylinux2014 config option
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also suggest that you drop the option here and just make manylinux2014 the default while you're using this PR. It will be easier to test the WIP containers that way, because they'll be the exact same as the regular ones and you won't need to change any flags outside.

We can revisit the preferred method of integration once you've verified everything works and you're ready to merge these -- which I expect won't happen for a while, until you've done more testing.

Copy link
Member Author

@nitins17 nitins17 Dec 2, 2021

Choose a reason for hiding this comment

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

Oh sure, I'll push a commit to update this file. Thanks!

@@ -15,7 +15,7 @@
# ==============================================================================
#
# Builds a devtoolset cross-compiler targeting manylinux 2010 (glibc 2.12 /
# libstdc++ 4.4).
# libstdc++ 4.4) or manylinux2014 (glibc 2.17 / libstdc++ 4.8).
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 file pulled directly from tensorflow's master branch? If so, you can also make a separate PR to update it to match -- it should be kept up to date with TensorFlow's (eventually, it should only live here, but for now it should be mirrored).

Copy link
Member Author

Choose a reason for hiding this comment

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

The only difference this file has with tensorflow's master branch is e2d2d48 where I removed Python 3.6.

Copy link
Member Author

@nitins17 nitins17 Dec 2, 2021

Choose a reason for hiding this comment

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

I'll push a commit to make it mirror the master branch's file.

@angerson
Copy link
Contributor

angerson commented Dec 1, 2021

You should be able to apply that label whenever you like, as a TF org member.

@nitins17 nitins17 added the build and push to gcr.io for staging Create a staging release on gcr.io label Dec 2, 2021
@angerson
Copy link
Contributor

angerson commented Dec 2, 2021

Huh. I guess the push didn't work because

Secrets are not passed to workflows that are triggered by a pull request from a fork.

So what I think you'll have to do instead is create a branch on this repository directly (you should have permissions to do this as a DevInfra team member) and then create a PR based on that branch, instead of a PR from your fork.

@angerson angerson removed the build and push to gcr.io for staging Create a staging release on gcr.io label Dec 2, 2021
@nitins17 nitins17 closed this Dec 2, 2021
@nitins17
Copy link
Member Author

nitins17 commented Dec 3, 2021

Closed this as #57 now instead tracks the manylinux2014 upgrade.

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.

2 participants