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

macOS Fixes / Larger Build Matrix #15

Merged
merged 7 commits into from
May 16, 2018
Merged

macOS Fixes / Larger Build Matrix #15

merged 7 commits into from
May 16, 2018

Conversation

promiseofcake
Copy link
Contributor

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

@@ -1,27 +0,0 @@
package(default_visibility = ['//visibility:public'])

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?

Copy link
Contributor Author

@promiseofcake promiseofcake May 15, 2018

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.

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?

Copy link
Contributor Author

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.

@promiseofcake
Copy link
Contributor Author

promiseofcake commented May 15, 2018

3764b99 replaces the -B/usr/bin argument to clang and specifying a search path of tools/cpp/tool_wrappers/mac which has a symlink to /usr/bin/ld to reduce the unintended consequences of including the larger scoped path.

Reverting 3764b99 due to it's inability to be utilized by other workspaces. This ld workaround might be overkill and might swap back to including all of /usr/bin in the search path since prior to Bazel 0.13.0, the user's entire PATH was leveraged for our Darwin builds, so I don't see this as a huge regression.

Copy link

@vladlosev vladlosev left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -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
Copy link
Contributor Author

@promiseofcake promiseofcake May 15, 2018

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.

Copy link
Contributor Author

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.

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.

@promiseofcake promiseofcake merged commit 3b42f66 into master May 16, 2018
@promiseofcake promiseofcake deleted the ljk/bazel-new branch May 16, 2018 14:51
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.

libunwind upstream doesn't exist
3 participants