bazelbuild / rules_cc

C++ Rules for Bazel
https://bazel.build
Apache License 2.0
189 stars 94 forks source link

Toolchain config alternative to `--per_file_copt` #209

Open tpudlik opened 1 year ago

tpudlik commented 1 year ago

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 commented 9 months ago

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 commented 8 months ago

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 commented 8 months ago

@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 https://github.com/bazelbuild/bazel/issues/12009 instead; I'm not quite sure how such an extension would work?).

t-rad679 commented 8 months ago

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 commented 8 months ago

@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 commented 8 months ago

IIUC https://github.com/bazelbuild/bazel/issues/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 commented 8 months ago

Got it, thanks for the explanation.

t-rad679 commented 8 months ago

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 commented 8 months ago

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:

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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

@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 commented 8 months ago

@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 commented 8 months ago

@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 commented 8 months ago

@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 commented 7 months ago

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 commented 3 months ago

@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 commented 3 months ago

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