-
Notifications
You must be signed in to change notification settings - Fork 8
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
macOS Fixes / Larger Build Matrix #15
Conversation
@@ -1,27 +0,0 @@ | |||
package(default_visibility = ['//visibility:public']) |
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.
Hmm, how is libunwind
broken here?
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.
@vladlosev, it's no longer used by Chromium except for fuchsia: https://bugs.chromium.org/p/chromium/issues/detail?id=803679, so they have removed 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.
IIRC, libunwind
is used by exception handling. Chrome, like most Google code, does not use exceptions, hence no need for libunwind
. Is it possible some of our code (or dependencies) will need them?
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.
Is it possible some of our code (or dependencies) will need them?
I think it could be possible in the future, but since this repo is, at present, concerned with building a Bazel toolchain based strictly upon Chromium's LLVM toolchain, I think that we can follow conventions there until we have a good reason to break away.
Since we are not using libunwind presently in any consumers of this repo, I think it would be better to remove the broken code and then re-add a working version later (or via contributions) when necessary.
Reverting 3764b99 due to it's inability to be utilized by other workspaces. 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.
LGTM
daf9d6f
to
37ea570
Compare
@@ -42,7 +42,7 @@ for i in "$@"; do | |||
LIBS="${BASH_REMATCH[1]} $LIBS" | |||
elif [[ "$i" =~ ^-L(.*)$ ]]; then | |||
LIB_DIRS="${BASH_REMATCH[1]} $LIB_DIRS" | |||
elif [[ "$i" =~ ^-Wl,-rpath,\$ORIGIN/(.*)$ ]]; then |
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.
This change moves the min OSX working version from 0.9.0 to 0.12.0, I probably will pin a release for 0.9.0 to make this clear, I will also update the README to describe those changes.
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.
Actually this might not be necessary, formerly we were using Bazel@head only for OSX, so this just provides more insight into what versions are supported.
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.
In all the articles I've been reading about -rpath
, only ones about Linux versions were mentioning $ORIGIN
, so it may well be a Linux-only feature. Apple-related ones seem to recommend using @loader_path
or @executable_path
with it.
* github.com/bazelbuild/bazel/blob/f98a7a2fedb3e714cef1038dcb85f83731150246
37ea570
to
775d7af
Compare
0.9.0 - 0.13.0
/usr/bin
to theclang
search path to access the linker in Bazel 0.12+cc_inc_library
This will fail for OSX builds with Bazel <= 0.12.0, I am not sure that statically linking gtest is the correct approach here to because this issue has a greater scope in other projects.
Internal ref: ML-1330