Toolchain config alternative to --per_file_copt

[tpudlik]

Description of the problem / feature request:

Provide a way to express --per_file_copt in the toolchain configuration, as an alternative to putting it in .bazelrc.

Feature requests: what underlying problem are you trying to solve with this feature?

--per_file_copt can be used to control warnings from third-party dependencies. For example, --per_file_copt=external/.*@-w will silence all such warnings (which is often appropriate, since projects often don't have control over their dependencies' third-party code).

Have you found anything relevant by searching the web?

Yes. This feature was originally requested here: https://groups.google.com/g/bazel-discuss/c/0zk1csyp4uU

It came up in https://bazelbuild.slack.com/archives/CGA9QFQ8H/p1698699534921939, too.

Any other information, logs, or outputs that you want to share?

This seems like an important piece of UX for configuring toolchains with strict warnings.

[tpudlik]

I bumped into this again. It really is a recurring problem.

Fundamentally, in a multirepo world where you don't own all the code you're pulling into your build, you want to be able to configure warnings on a per-repo basis. It's weird that this ends up in .bazelrc instead of in the toolchain configuration, as this makes the two hard to keep in sync. In practice you're reduced to disabling all warnings for external code, which is a hammer I'd rather not use.

[tpudlik]

Silencing external warnings through --per-file-copt in .bazelrc is particularly inconvenient in projects that build the same targets with more than one toolchain (e.g., arm and clang). We apply different warnings for different toolchains, and so need different warning silencing instructions.

[tpudlik]

@fmeum FYI in case you have comments on this (beyond the high-level suggestion in the Bazel Slack that maybe this should be handled by an extension of bazelbuild/bazel#12009 instead; I'm not quite sure how such an extension would work?).

[t-rad679]

Hey y'all,

I think I'm fairly close to a solution proposal for this, but I wanted to make sure that what I have in mind actually meets the needs of the users.

In particular, it seems fairly straightforward to add the possibility to expand on regex matches. Example code:

expand_if_regex_matches = variable_with_value(
    name = "source_file",
    value = "external.*",
)

In my intended scenario, the above code would be essentially equivalent to the example code in the first message of the thread in the first comment on this issue. In addition to being much easier to implement, this seems like a much more generic solution that could satisfy use cases (ie, checking variables other than source_file).

Do y'all have any use cases that this would not satisfy, or any other reason you feel this solution is not ideal? I could probably implement what is described in that thread, but it would take much longer and, as far as I can tell, it would only satisfy a small subset of the use cases the regex solution does. Also open to suggestions for other solutions.

Thanks!

[t-rad679]

@tpudlik Can you give me more detail about your last comment?

I took a look at the issue you linked, and I'm not sure how the functionality described there doesn't meet the needs described in this one? I see that users were posting saying that it didn't work for them, but I don't see the bug being reopened or anything like that. I do see how this solution is more generic, but I don't understand how the initial intention is different.

Also I really think someone should comment on that issue and help those users out who are trying to use the feature but can't.

[tpudlik]

IIUC bazelbuild/bazel#12009 specifically handles the case of warnings coming from included headers from external repositories. But it won't help you if your project has deps on (and attempts to build) cc_library targets defined in external repositories.

[t-rad679]

Got it, thanks for the explanation.

[t-rad679]

Back to the regex solution, it was brought to my attention that regex can be a performance concern. Are we worried about that in this case? If so, are there any ways that we can mitigate those concerns?

[fmeum]

In my opinion, this feature would be a layering violation: Toolchains should be reusable across projects and should thus not encode assumptions about the source file path layout of a given project. Furthermore, evaluating regexes for every compilation action could at least in theory become a performance concern.

Instead, how about the following:

• Add a disable_all_warnings or external_project feature to your toolchain that disables all warnings in a way appropriate for your toolchain's compiler. This is of course possible today.
• Request that feature for all external repositories. Today, this requires patching a REPO.bazel with repo(features = ["disable_all_warnings"]) into each repo, but this could be improved. For example, we could add an extension to rules_cc that can be used to configure features for all repos created by given modules from the MODULE.bazel file. This allows for far more expressive, regular starlark syntax instead of flags, e.g.:

cc.feature_config(feature = "disable_all_warnings", all_modules = True)
cc.feature_config(feature = "-disable_all_warnings", modules = ["my_root_module"])

The particular syntax is completely made up and not well thought out, but I'm sure we can get something more ergonomic and flexible this way compared to a flag or toolchain config.

Cc @Wyverald

[tpudlik]

Thank you for the feedback! But would this proposed solution only work if all your (transitive) dependencies have migrated to bzlmod? That's not practical, since so much of the ecosystem is still on WORKSPACE.

[fmeum]

We could build arbitrary logic into rules_cc to handle WORKSPACE repos, so yes, we could cover this.

Maybe @meteorcloudy also has some more insights into this interaction of C++ feature config and external deps.

[t-rad679]

@fmeum

Just to clarify, the solution you mentioned that currently exists involving REPO.bazel would require modifying the files inside the local copy of an external dependency? And there's no equivalent if you're using WORKSPACE correct?

I don't really have a good understanding of where this logic (i.e., cc.feature_config) would live in rules_cc or where it would be called from, though. I also don't know how we would retrieve/modify the configuration of the modules/repos. Did you have an idea of how these things would look?

[meteorcloudy]

@fmeum @Wyverald I'm also not sure how do we inject REPO.bazel file based on information collected by cc.feature_config. I guess we'll have to change Bazel itself?

[fmeum]

@meteorcloudy That sounds tricky indeed. But since features are essentially a C++/Objc-only mechanism, we could just have the relevant rules read the result of the module extension and add the features in their rule impls.

[tpudlik]

@comius what's the rules_cc owners' perspective on this?

I'm a little wary of a solution that integrates deeply with deps management, and requires separate bazelmod and WORKSPACE implementations.

I'd also like to support more granular external deps warning configurations (e.g., disabling only specified warnings, only when building with a particular compiler).

Toolchains should be reusable across projects and should thus not encode assumptions about the source file path layout of a given project.

I think this is true in some domains, but not in others. Projects targeting embedded systems generally want to control their own toolchain configuration. So for these users, the goal is making it possible to construct a toolchain's configuration from reusable components, rather than providing a full configuration that's reusable. Something like selecting which warnings to enable would definitely be project-specific.

[t-rad679]

So, just had a chat with @tpudlik, @comius, @Wyverald, @fmeum, and a few others...

We came to the conclusion that what we're after here can be achieved today by doing the following:

You can essentially add a feature disable_warnings that is enabled by default, then add the following to your REPO.bazel

repo(features = ["-disable_warnings"]

We discussed that this may not meet higher granularity needs, we determined that, since the needs motivating this specific bug are met, we're going to close it for now.

[cramertj]

@t-rad679 One downside of the REPO.bazel approach is that it disables warnings for downstream users as well as for third-party dependencies. Downstream users would also have to copy the REPO.bazel in order to enable warnings.

[keith]

Another case where REPO.bazel is unsuitable is if you're pulling in the repository as a submodule and adding it as a local_repository in order to make iterating on it for developers easier. In that case if you don't have a custom fork + branch that you're rebasing as you update, you can't change the REPO.bazel there