-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Trying to fix bazel on macOS and Xcode 14.3 #8304
base: master
Are you sure you want to change the base?
Conversation
b6cc432
to
325fa94
Compare
7e89a8f
to
f99a467
Compare
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.
Can you add a comment indicating why we need this/what it is doing? Or if it is borrowed from some other project, where it comes from.
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.
@jkuszmaul
This patch was kinda bored from tensorflow however its built on my own personal device, and it was done by simply fetching boringssl and trying adding it mainly to make sure it aligns with the patch that we already had for boringssl.
What it does specifically it removes the unused variable that was patched here commit
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.
Should I also update the commit body?
Removes references of swift from the windows pipeline for bazel,
and starts fetching boringssl directly within the repository to apply
the patch that fixes Bazel from throwing a unused var warning.
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.
My main goal here is make it as easy as possible for the next person who has to upgrade/alter dependencies to figure out what to do with this (whether to go back and look elsewhere for help, just delete it, etc.)---also, if it were copied 100% from somewhere else, then appropriate attribution would be necessary. Noting this clearly in the PR description so that it ends up in the commit message would be great; I personally like to throw comments in either the diff or where the patch is referenced in the BUILD file.
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.
Okay perfect! I'll update the PR to also include it
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.
Done, added some description to the WORKSPACE file and at the same time a summary within the commit
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.
I wonder how hard it would be to add Windows to this.
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.
I have no clue unfortunately
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.
|
||
#### Building boring ssl | ||
# Fetching boringssl within the flatbuffers repository, to patch the issue | ||
# of not being able to upgrade to Xcode 14.3 due to buildkite throwing errors | ||
# which was patched in the following below. | ||
# https://github.com/google/flatbuffers/commit/67eb95de9281087ccbba9aafd6e8ab1958d12045 | ||
# The patch was copied from the following comment on the same issue within tensorflow | ||
# and fixed to adapt the already existing patch for boringssl. | ||
# https://github.com/tensorflow/tensorflow/issues/60191#issuecomment-1496073147 |
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.
Adds a description to what the issue was, and where the issue was addressed before and where is the actual patch coming from
Ah linux is now broken :/ |
The following fixes the issue we are facing with
buildkite
and Xcode 14.3 by fetchingboringssl
within the flatbuffers repository and applying the patch directly. And also stops fetching swift for bazel on windowsdepends on #8307
Closes #8301