Added cxx_standard package for defining cxxopts

[UebelAndre]

This change allows users to specify default -std=c++ (or /std:c++ with MSVC) for targets that can still be overridden via the command line. This accounts for the scenario where libraries which require a min version of std set via .bazelrc flags become unbuildable in external repositories without also specifying this global flag or authoring a complicated transition. Instead, projects like these can specify a default value using the cxxopts macro but still benefit from a command line flag that affects all targets.

closes #406

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[UebelAndre]

CI appears to be failing for an issue unrelated to this change.

[fmeum]

This is just a drive-by review by someone interested in this topic in response to your request on Slack.

Could you describe why you went with select instead of well-known toolchain features?

I like that this introduces a way to override the standard from the command line. How should this interact with target-level settings though? If my target requires c++14 and I enable c++17 globally, c++17 should be picked and everything is fine. But if my target requires c++17 and I only enable c++14 globally, wouldn't the ideal experience be an analysis failure when that particular target is requested?

[fmeum]

Could this be an exact match?

[UebelAndre]

Yeah, no that also works. I'm no expert in the MSVC ecosystem, I just wanted to make sure the "windows-y" flags made it to the windows compilers so opted for the prefix.

[UebelAndre]

Is this what you're thinking?

Suggested change

	if compiler.startswith("msvc"):
	if compiler == "msvc-cl":

[fmeum]

What's the backwards compatibility story here? Could we ever delete this constant once we have added it?

[UebelAndre]

Since the rules are semantically versioned I figured a value could be dropped from release to release but I also think keeping a large list here is low cost.

[fmeum]

What's the use case for not specifying this parameter vs. not using cxxopts at all?

[UebelAndre]

you could still use the --@rules_cc//cc/cxx_standard flag to globally affect just this value. This is a slight convenience over explicitly specifying --copt or something as it would only affect targets that used the macro (as not all targets may be compatible with that version of C++). Though I would not be opposed to making default mandatory as it'd cleanup the awkward none value.

[fmeum]

This won't apply transitively. Could that become a usability issue when dealing with ABI-breaking standards changes (11 vs. 14)

[UebelAndre]

This wasn't intended to apply transitively. Yes it does mean you might specify this a lot, but in all the projects I've worked on when we get to a critical mass of cc targets folks have written macros to wrap this kind of boilerplate. But the intent was for this to be applied granularly.

[UebelAndre]

@fmeum Thanks for the review!

Could you describe why you went with select instead of well-known toolchain features?

I don't trust toolchain authors to implement a common feature for cxx standard. Unless there's some guidance somewhere I don't know about, in which case that would also be good. I opted to go for a select model since what I ultimately want is targets that require a cxx version to be able to specify that. Take llvm-project for example, I think at some point C++17 is required but you won't actually be able to build it and use it in your project unless you specify the flags via the command line or in your .bazelrc. Sure the maintainers there may also have a toolchain that adds it but I couldn't build it unless I added some kind of transition:

https://github.com/bazelbuild/rules_rust/blob/7e9252fb43259f4e3eb379b5e184ca10ea75b5b8/extensions/bindgen/BUILD.bazel#L29-L37

Instead of transitioning like this at the top level, if the target is genuinely un-buildable without these cxxopts then I would assert the target should have them but I also don't want that to come at the cost of easily toggling the version to try upgrading to new versions or implementing a similar transition to toggle the desired version (in more nuanced situations).

Can you expand on what you mean by "well-known toolchain features"? Is there something I'm missing?

[UebelAndre]

I like that this introduces a way to override the standard from the command line. How should this interact with target-level settings though? If my target requires c++14 and I enable c++17 globally, c++17 should be picked and everything is fine. But if my target requires c++17 and I only enable c++14 globally, wouldn't the ideal experience be an analysis failure when that particular target is requested?

Maybe there should be a way to specify a version independent of the global toggle, that's something I can definitely add. But I only envisioned the global flag to be used for development when you're transitioning a specific library, then you might update the default and move. This workflow had less thought than just providing a way to specify a default without breaking use-cases I see in projects where folks rely on --copt to set these flags.

[UebelAndre]

@fmeum friendly ping here 😄

[UebelAndre]

@fmeum hey, I have some renewed interest in this. Is there any movement toward another solution?

[fmeum]

@UebelAndre Sorry for the silence on this. I haven't gotten to think about this again. You are definitely more qualified to decide which approach works best in practice. How can I help?

[UebelAndre]

@UebelAndre Sorry for the silence on this. I haven't gotten to think about this again. You are definitely more qualified to decide which approach works best in practice. How can I help?

@fmeum I still think this approach is good. There will always be tradeoffs for solutions here but I think this has an easy means to disable the use of the cxxopts if users run into a tradeoff they don't like.

[fmeum]

I generally like the macro approach, but I wonder whether this composes safely. If one dep specifies C++14 for a target and another dep specifies C++17 for another target, couldn't that result in ABI breakages if the user doesn't explicitly set a default version (at least C++ 17)?

It seems like this macro approach would work well if the default version was the maximum encountered across all deps. Otherwise it looks like users would pretty much always have to set a standard explicitly to avoid breakages.

[armandomontanez]

Yes, that's absolutely a potential problem. I remember a while back I was working in a project where some libraries did exactly this (specify an explicit standard version, and let the rest of the system use something different), and things did NOT work harmoniously. Aside from the fact that headers may have #if directives that change behaviors in different translation units, the ABI problem is real.

It's only safe to mix C++ compile units across C API boundaries. I do not believe this approach is safe.

[UebelAndre]

I don't see how this is any different than linking a prebuilt binary into your library (e.g. boost). Yes you can create a scenario where you have a bad library interface and then compile with different stdcxx and break your builds. But I don't see how that's anything other than user error. Again, my core desire for this feature is to use it in external dependencies so I don't have to author transitions or beg consumers to set --copts in their .bazelrc.

[UebelAndre]

I've added additional testing to demonstrate that any use of this macro can be disabled by setting --@rules_cc//cc/cxx_standard:cxx_standard=none if there is a concern about these flags being used at all. I still see this as higher value than having folks explicitly add -std=c++* directly to a targets cxxopts which produces the exact failure mode being mentioned but is not something that can be disabled without patching or making a change to that target.

[UebelAndre]

ping @fmeum @armandomontanez

[fmeum]

I still see this as higher value than having folks explicitly add -std=c++* directly to a targets cxxopts which produces the exact failure mode being mentioned but is not something that can be disabled without patching or making a change to that target.

I fully agree with this. I would like to understand why explicit -std=c++... args are used in projects meant to be reused in the first place. Wouldn't it be strictly better to move such flags into the project's .bazelrc, ideally even as --features?

If that's done the project would still build in its own repo and would pick up any explicit standard set by the root project, either via the toolchain or similar .bazelrc flags.

The missing part would then be to ensure that the root module builds everything with a consistent standard even if it doesn't declare one of its own. I think that this necessarily requires a module extension that collects this information from all Bazel modules and exposes this minimum standard as a constant. It could then be embedded by toolchains or used as the default value of a flag like the one introduced by your PR.

[UebelAndre]

I fully agree with this. I would like to understand why explicit -std=c++... args are used in projects meant to be reused in the first place. Wouldn't it be strictly better to move such flags into the project's .bazelrc, ideally even as --features?

No, using --cxxopt (in your .bazelrc) causes this to apply globally which can break other parts of the repo. One could maybe use --per_file_coptand --host_per_file_copt but it's not a widely known pattern and can be incredibly unruly if the team uses this as the primary means of setting flags. It forces all consumers of an external dependency to need to manually update these flags should something about the project change.

Ultimately the goal is to have the necessary configuration needed to build a target on the target itself. Unfortunately with features, there's no way to guarantee all toolchains have some subset of common features so the likelihood that they're useful at all is fairly diminished. So far, most teams I've worked with have simply added cxxopts directly which has been the most ergonomic experience for external consumers. "You want to use some third party library? Just add it as a bazel_dep and start referring to it" is the desired experience.

My proposal here gives for the most common issue that requires edits in .bazelrc while also adding a kill-switch for any case where an external repo used it and the consumer didn't want it.

The missing part would then be to ensure that the root module builds everything with a consistent standard even if it doesn't declare one of its own. I think that this necessarily requires a module extension that collects this information from all Bazel modules and exposes this minimum standard as a constant. It could then be embedded by toolchains or used as the default value of a flag like the one introduced by your PR.

This change is largely an ergonomics improvement for folks who don't want to spend tons of time configuring the their build system. Having come from CMake, I rarely had to think about flags needed for any external project for a number of reasons:

1. There's usually a find_* module that can expose the dependency without much thought into it
2. Dependencies aren't usually built from source (something I think is not ideal but in practice is worth the cost for ergonomics)
3. Dependencies have well defined interfaces that don't subject themselves to potential ABI differences.

Correct me if I'm wrong but it seems 3 is the primary issue here and if so I'd love to know how common of an issue this is for external dependencies to your org. In my limited C++ experience, this issue would only occur in libraries authored and maintained internally which is a scenario that doesn't really have anything to do with this proposal as those same users could (and I've seen this) just hard-code -std=c++XX while using various parts of std in their interface.

[UebelAndre]

ping @fmeum @armandomontanez