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.

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

Patch boringssl for ppc64le #20791

Merged

Conversation

wdirons
Copy link
Contributor

@wdirons wdirons commented Jul 13, 2018

This commit adds a patch file for the bazel BUILD file contained in the
boringssl.tar.gz file. It set the necessary compile options to build boringssl
on pp64le. (Basically adds linux_ppc64le everywhere linux_x86_64 is)

Fixes #20677

This commit adds a patch file for the bazel BUILD file contained in the
boringssl.tar.gz file. It set the necessary compile options to build boringssl
on pp64le. (Basically adds linux_ppc64le everywhere linux_x86_64 is)

Fixes tensorflow#20677
@wdirons
Copy link
Contributor Author

wdirons commented Jul 13, 2018

Related question , if I wanted to upstream this fix so the patch wouldn't be required for Tensorflow where would I do it? The tar.gz file that is downloaded for boringssl includes more files then what I get when cloning https://boringssl.googlesource.com/boringssl/

@wdirons
Copy link
Contributor Author

wdirons commented Jul 13, 2018

I found that the boringssl repo has a master-with-bazel branch that I need to work from.

@qlzh727 qlzh727 requested a review from gunan July 16, 2018 17:23
@qlzh727 qlzh727 added the awaiting review Pull request awaiting review label Jul 16, 2018
"crypto_sources",
"crypto_sources_linux_x86_64",
"crypto_sources_mac_x86_64",
+ "crypto_sources_linux_ppc64le",
Copy link
Contributor

Choose a reason for hiding this comment

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

Previously when we asked boringssl about such patches, they told us some of these may not work at all.
Can we work with them to make sure this works, and then upstream it into boringssl?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gunan , I got the patch merged upstream into boringssl today. I'm in the process of testing the new tar.gz update in workspace.bzl, then I'll add a commit to change this PR to just use the new boringssl version.

In booringssl PR:
https://boringssl-review.googlesource.com/c/boringssl/+/29784

The bazel build file was updated to build ppc64le with the same
compile options as linux_x86_64. This commit updates boringssl
to reference that commit.
@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Jul 17, 2018
@jayfurmanek
Copy link
Contributor

@gunan Can we get a quick re-review? Looks like @wdirons had to make one last change to resolve #20677

Also is it possible to pick this over to the 1.10 branch?

Thx!

@kokoro-team kokoro-team removed kokoro:run kokoro:force-run Tests on submitted change labels Jul 25, 2018
@qlzh727 qlzh727 added the kokoro:force-run Tests on submitted change label Jul 27, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jul 27, 2018
@qlzh727 qlzh727 added the ready to pull PR ready for merge process label Jul 27, 2018
@tensorflow-copybara tensorflow-copybara merged commit 578da82 into tensorflow:master Aug 7, 2018
tensorflow-copybara pushed a commit that referenced this pull request Aug 7, 2018
@wdirons wdirons deleted the patch_boringssl_for_ppc64le branch September 26, 2018 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR ready for merge process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants