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

Fail to compile TF 2.12.0 with XCode 14.3 due to Compiler flag in boringssl/src/crypto/x509 #60191

Closed
feranick opened this issue Mar 31, 2023 · 18 comments
Assignees
Labels
awaiting review Pull request awaiting review subtype:macOS macOS Build/Installation issues TF 2.12 For issues related to Tensorflow 2.12 type:bug Bug type:build/install Build and install issues

Comments

@feranick
Copy link
Contributor

feranick commented Mar 31, 2023

Click to expand!

Issue Type

Bug

Have you reproduced the bug with TF nightly?

No

Source

source

Tensorflow Version

2.12.0

Custom Code

No

OS Platform and Distribution

macOS 13.3

Mobile device

No response

Python version

3.10

Bazel version

5.3.0

GCC/Compiler version

XCode 14.3

CUDA/cuDNN version

No response

GPU model and memory

No response

Current Behaviour?

Using standard compiling procedure (no special flags), compilation of the external library: boringssl/src/crypto/x509 fails. Log attached below.

Standalone code to reproduce the issue

Compile TF 2.12.0 using MacOS 13.x and XCode 14.3 (not earlier).

Relevant log output

: /private/var/tmp/_bazel_alex/dc1a9368c8e4ba5b96348c2850b37ab0/external/boringssl/BUILD:161:11: Compiling src/crypto/x509/t_x509.c [for host] failed: (Exit 1): cc_wrapper.sh failed: error executing command external/local_config_cc/cc_wrapper.sh -U_FORTIFY_SOURCE -fstack-protector -Wall -Wthread-safety -Wself-assign -Wunused-but-set-parameter -Wno-free-nonheap-object -fcolor-diagnostics ... (remaining 44 arguments skipped)
external/boringssl/src/crypto/x509/t_x509.c:321:18: error: variable 'l' set but not used [-Werror,-Wunused-but-set-variable]
    int ret = 0, l, i;
                 ^
1 error generated.
Target //tensorflow/tools/pip_package:build_pip_package failed to build
@feranick
Copy link
Contributor Author

feranick commented Apr 1, 2023

The issue is related to an unused variable in external/boringssl/src/crypto/x509/t_x509.c:321:18. Removing that variable fixes the issue. See attached.

Patch: issue_60191_patch.txt

@tiruk007 tiruk007 added type:build/install Build and install issues subtype:macOS macOS Build/Installation issues TF 2.12 For issues related to Tensorflow 2.12 labels Apr 3, 2023
@tiruk007
Copy link
Contributor

tiruk007 commented Apr 3, 2023

@feranick
Could you please elaborate more and provide detailed steps to replicate the issue reported here ?

Thank you!

@tiruk007 tiruk007 added the stat:awaiting response Status - Awaiting response from author label Apr 3, 2023
@feranick
Copy link
Contributor Author

feranick commented Apr 4, 2023

Could you please elaborate more and provide detailed steps to replicate the issue reported here ?

  1. Make sure you have XCode 14.3 installed (earlier versions won't compile TF as per issue: TF 2.11.0/2.12 fails to build in MacOS 13 - XCode 14.1 - issue with ld linker #58368 )
  2. git clone TF and checkout version 2.12.0 (or 2.11.1)
  3. cd in the folder tensorflow run ./configure with all default options.
  4. run compilation: bazel build --config=opt //tensorflow/tools/pip_package:build_pip_package --verbose_failures

At some point compilation will stop with the error in this issue.

To fix it:

  1. run your text editor (I use nano) into the external folder with the problematic library boringssl: nano /private/var/tmp/_bazel_YOU-AS-USER/SOME_ALPHANUMERIC/external/boringssl/src/crypto/x509/t_x509.c
  2. modify the code according to the patch attached (essentially remove all references to the unused variable l)
  3. restart compilation: bazel build --config=opt //tensorflow/tools/pip_package:build_pip_package --verbose_failures

Patch:
issue_60191_patch.txt

@google-ml-butler google-ml-butler bot removed the stat:awaiting response Status - Awaiting response from author label Apr 4, 2023
@feranick
Copy link
Contributor Author

feranick commented Apr 4, 2023

There might be ways to disable the -Wunused-but-set-variable flag, but I prefer to actually fix the code by removing the useless variable in first place.

Removing the variable should be applied in the ustream version as well (or make it do something useful, if that was the intent)

@feranick
Copy link
Contributor Author

feranick commented Apr 4, 2023

Note: the issue is not present in the master git for boringssl:
https://boringssl.googlesource.com/boringssl/

Th unused variable is simply removed as per my patch above. Therefore TF either needs to resync boringssl for a newer release or apply my patch (attached).
Patch:
issue_60191_patch.txt

@tiruk007 tiruk007 assigned pjpratik and unassigned tiruk007 Apr 4, 2023
@pjpratik pjpratik assigned SuryanarayanaY and unassigned pjpratik Apr 6, 2023
@SuryanarayanaY
Copy link
Collaborator

@feranick ,

Thanks for bringing this with the solution. If you are willing to contribute please feel free to raise a PR.

Thanks!

@feranick
Copy link
Contributor Author

feranick commented Apr 6, 2023

I would... Unfortunately the library is not included in the main TF tree, as it is pulled from private google servers. It needs to be fixed internally. BTW, TF pulls a specific version (can't tell you which one), but the bug is no longer present in the current master for boringssl (basically it has my patch applied). So bazel or whatever software pulls boringssl from the server needs to be updated to pull a more recent version, something only people with access to Google boringssl private repo can do.
https://boringssl.googlesource.com/boringssl/
It is also fixed in the github repo:
https://github.com/google/boringssl
So all this really needs is to pull a more recent version of boringssl.

@SuryanarayanaY
Copy link
Collaborator

Also, correct me if I am wrong.The file you mentioned for correction seems to be a temp file generated during bazel build.Not sure we can fix this from TF source tree. May be its related to Bazel.

I have gone through the bazel build docs in TF repo and found this one have some context for boring SSL.

tf_http_archive(
name = "boringssl",
sha256 = "9dc53f851107eaf87b391136d13b815df97ec8f76dadb487b58b2fc45e624d2c",
strip_prefix = "boringssl-c00d7ca810e93780bd0c8ee4eea28f4f2ea4bcdc",
system_build_file = "//third_party/systemlibs:boringssl.BUILD",
urls = tf_mirror_urls("https://github.com/google/boringssl/archive/c00d7ca810e93780bd0c8ee4eea28f4f2ea4bcdc.tar.gz"),
)

Whether we can do something here by changing URL or any thing to rectify this problem ?

@feranick
Copy link
Contributor Author

feranick commented Apr 6, 2023

Whether we can do something here by changing URL or any thing to rectify this problem ?

Yes, you are correct. Bazel builds it within a temporary folder.

And yes, I would think changing the URL might do it. However, I am not sure what URL/file to use from git as it probably uses an internal branch that is tar zipped. So the question is whether that package is there exclusively for TF... Maybe one can create a new package branched from main and placed in the same folder and then correct the reference URL in bazel....

@feranick
Copy link
Contributor Author

feranick commented Apr 6, 2023

OK, on a deeper inspection, it seems that the link has been already fixed in TF master. When looking at tensorflow/tensorflow/workspace2.bzl

TF Master:

    tf_http_archive(
        name = "boringssl",
        sha256 = "9dc53f851107eaf87b391136d13b815df97ec8f76dadb487b58b2fc45e624d2c",
        strip_prefix = "boringssl-c00d7ca810e93780bd0c8ee4eea28f4f2ea4bcdc",
        system_build_file = "//third_party/systemlibs:boringssl.BUILD",
        urls = tf_mirror_urls("https://github.com/google/boringssl/archive/c00d7ca810e93780bd0c8ee4eea28f4f2ea4bcdc.tar.gz"),
    )

while for TF 2.12.0:

tf_http_archive(
        name = "boringssl",
        sha256 = "534fa658bd845fd974b50b10f444d392dfd0d93768c4a51b61263fd37d851c40",
        strip_prefix = "boringssl-b9232f9e27e5668bc0414879dcdedb2a59ea75f2",
        system_build_file = "//third_party/systemlibs:boringssl.BUILD",
        urls = tf_mirror_urls("https://github.com/google/boringssl/archive/b9232f9e27e5668bc0414879dcdedb2a59ea75f2.tar.gz"),
    )

So, in principle, one would only need to replace the reference links in workspace2.bzlto the newer version now in master...

@feranick
Copy link
Contributor Author

feranick commented Apr 6, 2023

I am doing a test build where I replaced the strings above from main. Will report shortly.

@feranick
Copy link
Contributor Author

feranick commented Apr 6, 2023

So far compilation is proceeding normally, beyond the point where it would crash because of this issue. It seems like the proposed solution (swapping the tf_http_archive from master) will fix the issue, possibly also for the 2.12.0 branch.

@feranick
Copy link
Contributor Author

feranick commented Apr 6, 2023

I can confirm that compilation proceeds correctly on any platform I tried (MacOSX, linux).

@feranick
Copy link
Contributor Author

feranick commented Apr 6, 2023

Thanks for bringing this with the solution. If you are willing to contribute please feel free to raise a PR.

Pull request is in #60259

@SuryanarayanaY
Copy link
Collaborator

@feranick ,

Thanks for all your effort and time in resolving and raising the PR. Our Team will review and update.

Thanks!

@SuryanarayanaY
Copy link
Collaborator

Hi @feranick ,

I can see nightly build was updated as required.

tf_http_archive(
name = "boringssl",
sha256 = "9dc53f851107eaf87b391136d13b815df97ec8f76dadb487b58b2fc45e624d2c",
strip_prefix = "boringssl-c00d7ca810e93780bd0c8ee4eea28f4f2ea4bcdc",
system_build_file = "//third_party/systemlibs:boringssl.BUILD",
urls = tf_mirror_urls("https://github.com/google/boringssl/archive/c00d7ca810e93780bd0c8ee4eea28f4f2ea4bcdc.tar.gz"),
)

Can we mark this as resolved. Please spare some time to verify and close the issue.

Thanks!

@feranick
Copy link
Contributor Author

It works now. Thanks for pushing it. Closing.

@google-ml-butler
Copy link

Are you satisfied with the resolution of your issue?
Yes
No

mustiikhalil added a commit to mustiikhalil/flatbuffers that referenced this issue May 17, 2024
Fetching boringssl within the flatbuffers repository, to patch the issues
of not being able to upgrade to Xcode 14.3 due to buildkite throwing
errors. The patch was inspired by the tenserflow patch
tensorflow/tensorflow#60191 (comment)

Removes references of swift from the windows pipeline for bazel

Sets github actions to use xcode 14.3 for kotlin and sets the macOS
build for intel cpus.
dbaileychess added a commit to google/flatbuffers that referenced this issue May 29, 2024
* Fixes Bazel issues for windows and ci

Fetching boringssl within the flatbuffers repository, to patch the issues
of not being able to upgrade to Xcode 14.3 due to buildkite throwing
errors. The patch was inspired by the tenserflow patch
tensorflow/tensorflow#60191 (comment)

Removes references of swift from the windows pipeline for bazel

Sets github actions to use xcode 14.3 for kotlin and sets the macOS
build for intel cpus.

* Update build.yml

Remove comment that is not relevant any longer.

---------

Co-authored-by: Derek Bailey <derekbailey@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review Pull request awaiting review subtype:macOS macOS Build/Installation issues TF 2.12 For issues related to Tensorflow 2.12 type:bug Bug type:build/install Build and install issues
Projects
None yet
Development

No branches or pull requests

4 participants