-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Project-wide platform-dependent compiler options #2358
Comments
Hi David, sorry for radio silence. Is this issue still relevant? |
I can reply on behalf of David. For the warning flags, we ended up doing option (2). See our tools/drake.bzl. This is probably the right choice for For other flags, we ended up doing option (3). See our checked-in CROSTOOL file, with file-top comments explaining how we generate it. I think these choices have worked well in practice, because our matrix of supported platforms and compilers is known a-priori and relatively small. YMMV. I would still be happy to hear any advice from the Bazel team about different suggestions, but if there is nothing more to add, we can consider this issue closed. |
Hi Jeremy, bazel/tools/osx/crosstool/CROSSTOOL.tpl Line 220 in 375f95b
You can enable the feature by default using
You can also specify them on package rule, and also as bazel option (--features=a,b,c). Feel free to continue discussion here, I'm closing the issue just so we know there are no action items for bazel (and will reopen if some show up). |
Excellent, thank you. Next time we are updating our CROSSTOOL we will take a look. I think considering this issue closed is appropriate. Thanks again! |
Description of the problem / feature request / question:
If asking a question or requesting a feature, also tell us about the underlying problem you're trying to solve.
At RobotLocomotion/drake, we're migrating from CMake to Bazel (yay!). We target gcc on Ubuntu, clang on Ubuntu, and clang on OS X.
There are some compiler flags that we need set differently from the Bazel defaults, and differently for gcc than for clang. For instance, the GCC spelling of a particular warning is
return-local-addr
, while the Clang spelling isreturn-stack-address
. Bazel doesn't promote that warning to an error by default incc_configure
, but we want to do so in Drake. We want it to take effect automatically on all build targets present and future, both on developer workstations (regardless of platform) and in CI.We considered a few approaches, all of which seem to work, and none of which are wholly satisfying. Do you have recommendations?
Add the platform specific flags to
tools/bazel.rc
using--cxxopts
, controlled by--config
switches namedgcc
andclang
. Possibly ship a wrapper script around Bazel that auto-selectsconfig
switches, or else just document it carefully and do the right thing in CI.Write a Skylark wrapper macro around
cc_library
that automatically populatescopts
with platform-appropriate flags, based on aselect
statement over aconfig_setting
that examinescpu
andcompiler
. Replace all uses ofcc_library
in ourBUILD
files with this macro, and add linter logic to bancc_library
going forward.Write our own hard-coded
CROSSTOOL
toolchain
stanzas for our supported platforms, by hand-editing the output ofcc_configure
. Check theCROSSTOOL
file into our own repo along with the supporting BUILD files and scripts from tools/cpp.Write a repository_rule that procedurally edits the output of
cc_configure
as in (3), using a lot ofgrep
andsed
. This is the approach we've tentatively chosen: Add compiler-specific warnings in Bazel. RobotLocomotion/drake#4717. It's by far the ugliest under the hood, but seems to put the least burden on our developers and users.Environment info
Operating System: Ubuntu Trusty/Xenial and OS X
Bazel version (output of
bazel info release
):Have you found anything relevant by searching the web? (e.g. GitHub issues, email threads in the [email protected] archive)
This thread recommends using
config_setting
andselect
in theBUILD
file to inject platform-specific options via thecopts
argument tocc_library
. Like alternative (2) above, but without the macro.Many thanks!
The text was updated successfully, but these errors were encountered: