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

common bazel cxx toolchain for Linux and Mac #1

Merged
merged 52 commits into from
Aug 3, 2017
Merged

common bazel cxx toolchain for Linux and Mac #1

merged 52 commits into from
Aug 3, 2017

Conversation

sayrer
Copy link
Contributor

@sayrer sayrer commented Jul 24, 2017

https://travis-ci.com/vsco/bazel-toolchains/builds/50882186

Still some cleanup to do:

  • not sure I'm getting a clean link on linux (should be using the sysroot on the link line).
  • give scripts/generate_workspace.py an arg parser
  • go through the visibility rules under tools/ (they are all public rn)
  • doesn't use the Chromium copy of libc++ on mac, but I don't think Chromium does either.
    [edit: this is a lie. we need to use their tree's copy of libc++ too. https://docs.google.com/document/d/1cFVCLYqpVV0pn2aX09SALcE3BN351sFN_rTcPcHRg3U/edit#]
    only used on linux: https://cs.chromium.org/chromium/src/build/config/c%2B%2B/c%2B%2B.gni?type=cs&q=use_custom_libcxx&l=11

@promiseofcake
Copy link
Contributor

Thanks for the updates, I was able to get parts of cpp building as well using the above, just running into the linker issues mentioned above. Are they specific to OSX linking only?

@sayrer sayrer removed the request for review from vladlosev August 3, 2017 04:53
README.md Outdated
## Prequisites

Choose a reason for hiding this comment

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

I think that prequisite might not be a word

response = requests.get(url, stream=stream)
if response.status_code != 200:
print >> sys.stderr, "Unexpected status code", response.status_code
raise

Choose a reason for hiding this comment

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

do you want to raise a specific exception here? (Python might fail on no argument here)

Copy link

@melindalu melindalu left a comment

Choose a reason for hiding this comment

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

My Bazel-fu is not strong, but this looks LGTM — very nice 👏 🎈 sorry for taking 84 years to respond

@promiseofcake
Copy link
Contributor

After removing the health-check code from cpp and building via

bazel build --cxxopt="-std=c++11" -c opt --crosstool_top=@co_vsco_bazel_toolchains//tools/cpp:default-toolchain //prediction-api:vsco_prediction_server

Was able to successfully build prediction-api on OSX! 🎉 🎉 🎉 🎉

@sayrer sayrer removed the request for review from negator August 3, 2017 21:30
@sayrer sayrer merged commit f6224d5 into master Aug 3, 2017
@sayrer sayrer deleted the add_mac branch August 3, 2017 21:36
@vsco vsco deleted a comment from promiseofcake Aug 4, 2017
@vsco vsco deleted a comment from promiseofcake Aug 4, 2017
TEST(CompilerMeta, AppleBuild) {
#if defined(__apple_build_version__)
// __apple_build_version__ shouldn't be defined in the Chromium toolchain
ASSERT_TRUE(false);

Choose a reason for hiding this comment

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

BTW, gtest has a FAIL() macro just for such a case.

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.

4 participants