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

incompatible_disable_nocopts: Disallow 'nocopts' attribute from cc_* rules #8706

Closed
scentini opened this issue Jun 24, 2019 · 19 comments
Closed
Assignees
Labels
incompatible-change Incompatible/breaking change P1 I'll work on this now. (Assignee required) team-Rules-CPP Issues for C++ rules

Comments

@scentini
Copy link
Contributor

Flag: --incompatible_disable_nocopts
Available since: 0.28
Will be flipped: 1.0

The nocopts attribute in cc_* rules is used to specify a pattern for filtering out flags from C++ compilation command line.
We are deprecating it because:

  • it prevents migration of C++ rules to Starlark: we don’t have regex in Starlark currently
  • we don’t need to do it - we can disable features to have the same effect

Migration

If you use the nocopts attributes to filter out a flag please comment on this issue, with the offending flag/pattern that you're filtering out.

As we have no way of knowing which flags users filter out, we will rely on them reaching out to us in case the flipping of the --incompatible_disable_nocopts flag breaks their project. We will keep on support for filtering out the flags through --noincompatible_disable_nocopts for 1-2 extra Bazel releases, until we are able to provide the support for disabling the reported flags through features.

If a user is filtering out a flag provided by bazel's own C++ toolchain configuration, we will wrap the flag into a feature, so the user can disable the feature instead of filter out the flag.
E.g a

cc_library(
    name = "lib",
    srcs = ["lib.cc", "lib.h"],
    nocopts = "flagA",
)

would be migrated to

cc_library(
    name = "lib",
    srcs = ["lib.cc", "lib.h"],
    features = ["-flag_a_feature"],
)

with flag_a_feature being defined in bazel's own C++ toolchain configuration.

@scentini scentini added P2 We'll consider working on this in future. (Assignee optional) team-Rules-CPP Issues for C++ rules labels Jun 24, 2019
@scentini scentini self-assigned this Jun 24, 2019
bazel-io pushed a commit that referenced this issue Jun 24, 2019
This flag prohibits the use of 'nocopts' attribute in cc_* rules.

Issue #8706

RELNOTES: --incompatible_disable_nocopts flag has been added. See #8706 for details.
PiperOrigin-RevId: 254781400
@scentini scentini added incompatible-change Incompatible/breaking change migration-ready labels Jun 24, 2019
siberex pushed a commit to siberex/bazel that referenced this issue Jul 4, 2019
This flag prohibits the use of 'nocopts' attribute in cc_* rules.

Issue bazelbuild#8706

RELNOTES: --incompatible_disable_nocopts flag has been added. See bazelbuild#8706 for details.
PiperOrigin-RevId: 254781400
bazel-io pushed a commit that referenced this issue Jul 10, 2019
Baseline: 2e374a9

Cherry picks:

   + 6d0b14b:
     rule_test: apply "tags" to all rules in the macro

Incompatible changes:

  - Add --incompatible_enable_profile_by_default to enable the JSON
    profile by default.
  - The --incompatible_windows_style_arg_escaping flag is flipped to
    "true", and the "false" case unsupported. Bazel no longer accepts
    this flag.

Important changes:

  - Bazel now supports hiding compiler warnings for targets that
    you're not explicitly building (see
    https://docs.bazel.build/versions/master/user-manual.html#flag--au
    to_output_filter).
  - Flag `--incompatible_restrict_escape_sequences` is added. See
    #8380
  - The "info" command now supports the "starlark-semantics"
    argument, which outputs a representation of the effective Starlark
    semantics option values.
  - The `outputs` parameter of the `rule()` function is deprecated
    and attached to flag `--incompatible_no_rule_outputs_param`.
    Migrate rules to use `OutputGroupInfo` or `attr.output` instead.
    See #7977 for more info.
  - When `--incompatible_strict_action_env` is enabled, the default
    `PATH` now includes `/usr/local/bin`.
  - Turn on --experimental_build_setting_api by default for starlark
    build settings (see
    https://docs.bazel.build/versions/master/skylark/config.html#user-
    defined-build-settings for more info)
  - `@bazel_tools//tools/jdk:toolchain_java10` and
    `@bazel_tools//tools/jdk:toolchain_java11` are now available to
    enable java 10, respectively java 11 language level support.
  - The `command` parameter of the `actions.run_shell()` function
    will be restricted to only accept strings (and not string
    sequences). This check is attached to flag
    `--incompatible_run_shell_command_string`. One may migrate by
    using the `arguments` parameter of `actions.run()` instead. See
    #5903 for more info.
  - Incompatible change
    `--incompatible_use_platforms_repo_for_constraints` has been
    added. See #8622 for
    details.
  - Incompatible change
    `--incompatible_use_platforms_repo_for_constraints` has been
    added. See #8622 f...
  - Bazel's C++ autoconfiguration now understands `BAZEL_LINKLIBS`
    environment variable to specify system libraries that should be
    appended to the link command line.
  - paths under the execution root starting with "." or "_" will be
    re-linked across builds
  - execution_log_json_file now allows actions without outputs.
  - Labels aapt as deprecated for aapt_version, and heavily endorses
    aapt2.
  - Update doc links still pointing to cc_binary.features to point to
    common features
  - Incompatible change
    `--incompatible_use_platforms_repo_for_constraints` has been
    added. See #8622 for
    details.
    RELNOTES:
  - --incompatible_disable_nocopts flag has been added. See
    #8706 for details.
  - Fixed treatment of <dist:module /> tags in AndroidManifest.xml
  - Fixed asset precedence for android_binary rules with aapt2.
  - Bazel now officially supports running on CentOS 7.
  - The runtime dynamic libraries are no longer in default output
    group of cc_binary.
  - set the FDOBuildType as CSFDO for binaries built with
    --cs_fdo_absolute_path.
  - Bazel can now be bootstrapped and built on arm64 platforms
    without requiring any flags or patches.
  - Fixed treatment of AndroidManifest.xml attributes which contained
    XML escaping
  - Retire experimental blaze flag
    experimental_link_compile_output_separately. The same behavior is
    available through the feature dynamic_link_test_srcs.
  - --incompatible_load_java_rules_from_bzl was added to forbid
    loading the native java rules directly. See more on tracking
    issue #8746
  - Turn on --experimental_build_setting_api by default for starlark
    build settings (see
    https://docs.bazel.build/versions/master/skylark/config.html#user-
    defined-build-settings for more info)
  - Attribute names are going to be restricted and must be
    syntactically valid identifiers.
    #6437
  - rule_test: fix Bazel 0.27 regression ("tags" attribute was
    ingored, #8723

This release contains contributions from many people at Google, as well as Ben Diuguid, Benjamin Peterson, Dave Lee, Loo Rong Jie, Mark Butcher, Marwan Tammam, Pedro Alvarez.
irengrig pushed a commit to irengrig/bazel that referenced this issue Jul 15, 2019
This flag prohibits the use of 'nocopts' attribute in cc_* rules.

Issue bazelbuild#8706

RELNOTES: --incompatible_disable_nocopts flag has been added. See bazelbuild#8706 for details.
PiperOrigin-RevId: 254781400
irengrig pushed a commit to irengrig/bazel that referenced this issue Jul 15, 2019
Baseline: 2e374a9

Cherry picks:

   + 6d0b14b:
     rule_test: apply "tags" to all rules in the macro

Incompatible changes:

  - Add --incompatible_enable_profile_by_default to enable the JSON
    profile by default.
  - The --incompatible_windows_style_arg_escaping flag is flipped to
    "true", and the "false" case unsupported. Bazel no longer accepts
    this flag.

Important changes:

  - Bazel now supports hiding compiler warnings for targets that
    you're not explicitly building (see
    https://docs.bazel.build/versions/master/user-manual.html#flag--au
    to_output_filter).
  - Flag `--incompatible_restrict_escape_sequences` is added. See
    bazelbuild#8380
  - The "info" command now supports the "starlark-semantics"
    argument, which outputs a representation of the effective Starlark
    semantics option values.
  - The `outputs` parameter of the `rule()` function is deprecated
    and attached to flag `--incompatible_no_rule_outputs_param`.
    Migrate rules to use `OutputGroupInfo` or `attr.output` instead.
    See bazelbuild#7977 for more info.
  - When `--incompatible_strict_action_env` is enabled, the default
    `PATH` now includes `/usr/local/bin`.
  - Turn on --experimental_build_setting_api by default for starlark
    build settings (see
    https://docs.bazel.build/versions/master/skylark/config.html#user-
    defined-build-settings for more info)
  - `@bazel_tools//tools/jdk:toolchain_java10` and
    `@bazel_tools//tools/jdk:toolchain_java11` are now available to
    enable java 10, respectively java 11 language level support.
  - The `command` parameter of the `actions.run_shell()` function
    will be restricted to only accept strings (and not string
    sequences). This check is attached to flag
    `--incompatible_run_shell_command_string`. One may migrate by
    using the `arguments` parameter of `actions.run()` instead. See
    bazelbuild#5903 for more info.
  - Incompatible change
    `--incompatible_use_platforms_repo_for_constraints` has been
    added. See bazelbuild#8622 for
    details.
  - Incompatible change
    `--incompatible_use_platforms_repo_for_constraints` has been
    added. See bazelbuild#8622 f...
  - Bazel's C++ autoconfiguration now understands `BAZEL_LINKLIBS`
    environment variable to specify system libraries that should be
    appended to the link command line.
  - paths under the execution root starting with "." or "_" will be
    re-linked across builds
  - execution_log_json_file now allows actions without outputs.
  - Labels aapt as deprecated for aapt_version, and heavily endorses
    aapt2.
  - Update doc links still pointing to cc_binary.features to point to
    common features
  - Incompatible change
    `--incompatible_use_platforms_repo_for_constraints` has been
    added. See bazelbuild#8622 for
    details.
    RELNOTES:
  - --incompatible_disable_nocopts flag has been added. See
    bazelbuild#8706 for details.
  - Fixed treatment of <dist:module /> tags in AndroidManifest.xml
  - Fixed asset precedence for android_binary rules with aapt2.
  - Bazel now officially supports running on CentOS 7.
  - The runtime dynamic libraries are no longer in default output
    group of cc_binary.
  - set the FDOBuildType as CSFDO for binaries built with
    --cs_fdo_absolute_path.
  - Bazel can now be bootstrapped and built on arm64 platforms
    without requiring any flags or patches.
  - Fixed treatment of AndroidManifest.xml attributes which contained
    XML escaping
  - Retire experimental blaze flag
    experimental_link_compile_output_separately. The same behavior is
    available through the feature dynamic_link_test_srcs.
  - --incompatible_load_java_rules_from_bzl was added to forbid
    loading the native java rules directly. See more on tracking
    issue bazelbuild#8746
  - Turn on --experimental_build_setting_api by default for starlark
    build settings (see
    https://docs.bazel.build/versions/master/skylark/config.html#user-
    defined-build-settings for more info)
  - Attribute names are going to be restricted and must be
    syntactically valid identifiers.
    bazelbuild#6437
  - rule_test: fix Bazel 0.27 regression ("tags" attribute was
    ingored, bazelbuild#8723

This release contains contributions from many people at Google, as well as Ben Diuguid, Benjamin Peterson, Dave Lee, Loo Rong Jie, Mark Butcher, Marwan Tammam, Pedro Alvarez.
laurentlb pushed a commit that referenced this issue Jul 16, 2019
Baseline: 2e374a9

Cherry picks:

   + 6d0b14b:
     rule_test: apply "tags" to all rules in the macro

Incompatible changes:

  - Add --incompatible_enable_profile_by_default to enable the JSON
    profile by default.
  - The --incompatible_windows_style_arg_escaping flag is flipped to
    "true", and the "false" case unsupported. Bazel no longer accepts
    this flag.

Important changes:

  - Bazel now supports hiding compiler warnings for targets that
    you're not explicitly building (see
    https://docs.bazel.build/versions/master/user-manual.html#flag--au
    to_output_filter).
  - Flag `--incompatible_restrict_escape_sequences` is added. See
    #8380
  - The "info" command now supports the "starlark-semantics"
    argument, which outputs a representation of the effective Starlark
    semantics option values.
  - The `outputs` parameter of the `rule()` function is deprecated
    and attached to flag `--incompatible_no_rule_outputs_param`.
    Migrate rules to use `OutputGroupInfo` or `attr.output` instead.
    See #7977 for more info.
  - When `--incompatible_strict_action_env` is enabled, the default
    `PATH` now includes `/usr/local/bin`.
  - Turn on --experimental_build_setting_api by default for starlark
    build settings (see
    https://docs.bazel.build/versions/master/skylark/config.html#user-
    defined-build-settings for more info)
  - `@bazel_tools//tools/jdk:toolchain_java10` and
    `@bazel_tools//tools/jdk:toolchain_java11` are now available to
    enable java 10, respectively java 11 language level support.
  - The `command` parameter of the `actions.run_shell()` function
    will be restricted to only accept strings (and not string
    sequences). This check is attached to flag
    `--incompatible_run_shell_command_string`. One may migrate by
    using the `arguments` parameter of `actions.run()` instead. See
    #5903 for more info.
  - Incompatible change
    `--incompatible_use_platforms_repo_for_constraints` has been
    added. See #8622 for
    details.
  - Incompatible change
    `--incompatible_use_platforms_repo_for_constraints` has been
    added. See #8622 f...
  - Bazel's C++ autoconfiguration now understands `BAZEL_LINKLIBS`
    environment variable to specify system libraries that should be
    appended to the link command line.
  - paths under the execution root starting with "." or "_" will be
    re-linked across builds
  - execution_log_json_file now allows actions without outputs.
  - Labels aapt as deprecated for aapt_version, and heavily endorses
    aapt2.
  - Update doc links still pointing to cc_binary.features to point to
    common features
  - Incompatible change
    `--incompatible_use_platforms_repo_for_constraints` has been
    added. See #8622 for
    details.
    RELNOTES:
  - --incompatible_disable_nocopts flag has been added. See
    #8706 for details.
  - Fixed treatment of <dist:module /> tags in AndroidManifest.xml
  - Fixed asset precedence for android_binary rules with aapt2.
  - Bazel now officially supports running on CentOS 7.
  - The runtime dynamic libraries are no longer in default output
    group of cc_binary.
  - set the FDOBuildType as CSFDO for binaries built with
    --cs_fdo_absolute_path.
  - Bazel can now be bootstrapped and built on arm64 platforms
    without requiring any flags or patches.
  - Fixed treatment of AndroidManifest.xml attributes which contained
    XML escaping
  - Retire experimental blaze flag
    experimental_link_compile_output_separately. The same behavior is
    available through the feature dynamic_link_test_srcs.
  - --incompatible_load_java_rules_from_bzl was added to forbid
    loading the native java rules directly. See more on tracking
    issue #8746
  - Turn on --experimental_build_setting_api by default for starlark
    build settings (see
    https://docs.bazel.build/versions/master/skylark/config.html#user-
    defined-build-settings for more info)
  - Attribute names are going to be restricted and must be
    syntactically valid identifiers.
    #6437
  - rule_test: fix Bazel 0.27 regression ("tags" attribute was
    ingored, #8723

This release contains contributions from many people at Google, as well as Ben Diuguid, Benjamin Peterson, Dave Lee, Loo Rong Jie, Mark Butcher, Marwan Tammam, Pedro Alvarez.
@scentini scentini added P1 I'll work on this now. (Assignee required) and removed P2 We'll consider working on this in future. (Assignee optional) labels Jul 22, 2019
@dslomov
Copy link
Contributor

dslomov commented Aug 5, 2019

What is the flag flip risk in 1.0?
https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/195 shows that it "only" breaks TensorFlow

@AustinSchuh
Copy link
Contributor

I grepped through our code base and we have 1 call site. One library really doesn't work well when built with coverage support (which is enabled by the toolchain/crosstool). We remove the --coverage flag with nocopts.

@scentini
Copy link
Contributor Author

scentini commented Aug 7, 2019

Thanks for reaching out!
Our C++ toolchain configuration rule provides the --coverage flag through a coverage feature.
Can you try replacing nocopts = '--coverage' with features = ["-coverage"] (note the minus , denoting "disable this feature")?

@AustinSchuh
Copy link
Contributor

That looks like it worked. That's a much better solution than nocopts. Thanks!

tensorflow-copybara pushed a commit to tensorflow/tensorflow that referenced this issue Aug 13, 2019
This attribute will be removed soon: bazelbuild/bazel#8706
The only nocopts values passed are "-fno-exceptions" and "-[W]error". Bazel's crosstool doesn't provide either of them.

PiperOrigin-RevId: 263082078
@scentini
Copy link
Contributor Author

This flag was flipped in 8b840e1.

@cocasema
Copy link

cocasema commented Nov 6, 2019

There's one more use-case for nocopts - when we build third-party code and have to disable some warnings (-Wno-). If we have both .c and .cpp files in a single cc_library and need to turn off some c-only warnings (like -Wno-declaration-after-statement), we also have to add then to nocopts to make g++ happy.

@scentini
Copy link
Contributor Author

scentini commented Nov 7, 2019

Do you use your own toolchain configuration? If so, you'll need to create a feature that will pass the -Wno-declaration-after-statement to the command line, and use
features = ["-declaration_after_statement"] in the targets that you'd like to not apply the flag to.

@wanguanglu
Copy link

if I want to filter "-Werror" with nocopts="-Werror"
but features = ["-error"] seems not work.

@asraa
Copy link

asraa commented Aug 24, 2020

Hi, I have a use-case for nocopts, and wanted to know what the appropriate migration is. I have a third-party library that I'm building through an http_archive, and wanted to build it without the preprocessor macro FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION defined, even when the CLI defines it. Some users pass in --copt=-DFUZZING_BUILD_... and a build configuraiton we define also uses this copt.

Originally I was going to use nocopts and patch the BUILD file for the dependency, but instead it seems I should be using a feature. Does this mean I need to create a feature for FUZZING_BUILD_MODE... and pass it in to every target but the RE2 library during a specific build config?

@mst9009
Copy link

mst9009 commented Oct 28, 2020

If I want to filter out the “-g” option, how should I set the “features” attribute?Thanks!

@wrowe
Copy link
Contributor

wrowe commented May 7, 2021

If I want to filter out the “-g” option, how should I set the “features” attribute?Thanks!

@mst9009 sorry this is quite late, but adding copts "-g0" should accomplish what you were looking for?

@changlan
Copy link

Hi. How should I filter out the -fstack-protector flag with the “features” attribute? Tried default_compile_flags but it removed all compile flags.

@mason-bially
Copy link

mason-bially commented Dec 3, 2021

This may be because of a whole separate issue (and I realize years after the original change), but one of the use cases for nocopts that I was using was removing /I. from some windows builds where it was being applied incorrectly (it's inclusion causes MSVC to behave differently than gcc/clang given the same file structure; this would be the separate and much more complex issue to fix that I am fine working around, even though it's pretty weird (the first depending cc_library's BUILD file location effects the build of the dependent cc_library for all the other depending rules)). So a feature flag setting of -include_path_local (minus to remove) would be nice (unless there is a syntax for removing a specific instance of the include_paths flag group? Currently my workaround is this:

cc_library(
    ...
    features = select({
        "@bazel_tools//src/conditions:windows": ["-include_paths"],
        "//conditions:default": [],
    }),
    copts = select({
        "@bazel_tools//src/conditions:windows": ["/Imanual", "/Iinclude", "/Ilist"],
        "//conditions:default": [],
    }),
)

Which is brittle, ugly, and not great (and is notably significantly worse than the original nocopts method of fixing it up)

@wondfor
Copy link

wondfor commented Jan 13, 2022

maybe you should update https://docs.bazel.build/versions/main/be/c-cpp.html#cc_binary.nocopts

@JohnGHS
Copy link

JohnGHS commented Mar 28, 2022

I have to admit that I'm only starting with Bazel, so please take the following with a grain of salt.
I'm trying to integrate a non-gcc C++ compiler into Bazel, and it is complaining about a lot of gcc-specific options that are being injected into the build. I can easily believe that this is because I'm following a tutorial that assumes a gcc builder, hence a single load() could be adding unnecessary flags. However, the most egregious of these (as I see it) are:

  • "Unknown option '-frandom-seed=...'"
  • "Unknown option '-iquote' passed to linker"
  • "Unrecognized option: -S"

I know what each of these options do with gcc (although I'm surprised to see '-iquote' passed to the linker), but want to turn them off.

@scentini
Copy link
Contributor Author

cc @oquenchil

@dmadisetti
Copy link

dmadisetti commented May 13, 2022

3 years later and docs have no been updated. Also dead references in comments and code: https://github.com/bazelbuild/bazel/search?p=1&q=nocopts&type=code (edit: github code search is 😬, see: https://cs.opensource.google/search?q=nocopts&sq=&ss=bazel%2Fbazel )

Is the logic with UNFILTERED_COMPILE_FLAGS_FEATURE_NAME still relevant?

if (pair.getFirst().equals(CppRuleClasses.UNFILTERED_COMPILE_FLAGS_FEATURE_NAME)) {

The provided toolchain link is also dead, has it been moved to https://cs.opensource.google/bazel/bazel/+/master:tools/cpp/cc_toolchain_config_lib.bzl ?

Which is then consumed by the various tool chain definitions? e.g. https://cs.opensource.google/bazel/bazel/+/master:tools/cpp/unix_cc_toolchain_config.bzl

I think these features need to be a bit better documented, are they consistent between toolchains? I think I found something for my usecase, but not with the granularity I would like (just looking to turn off cxx_flags).

luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
    This flag prohibits the use of 'nocopts' attribute in cc_* rules.

    Issue #8706

    RELNOTES: --incompatible_disable_nocopts flag has been added. See bazelbuild/bazel#8706 for details.
    PiperOrigin-RevId: 254781400
copybara-service bot pushed a commit that referenced this issue Jan 19, 2023
Using this attribute was disabled, see #8706. It was added to the Starlark rule definition although it wasn't working. It is now moved to semantics while we do the necessary internal migration to remove it completely.

output_filter_test doesn't make sense in Bazel since nocopts is not available. The file must remain while we migrate internally away from nocopts though.

RELNOTES:none
PiperOrigin-RevId: 503155992
Change-Id: I43184941c9cb9ee97d5229cf37062b346643a48a
hvadehra pushed a commit that referenced this issue Feb 14, 2023
Using this attribute was disabled, see #8706. It was added to the Starlark rule definition although it wasn't working. It is now moved to semantics while we do the necessary internal migration to remove it completely.

output_filter_test doesn't make sense in Bazel since nocopts is not available. The file must remain while we migrate internally away from nocopts though.

RELNOTES:none
PiperOrigin-RevId: 503155992
Change-Id: I43184941c9cb9ee97d5229cf37062b346643a48a
armandomontanez added a commit to armandomontanez/nanopb that referenced this issue Nov 8, 2023
With Bazel 7.0.0, nocopts has been removed and is no longer supported.
The correct way to handle this is to subtract features using the
`features` attribute (see bazelbuild/bazel#8706).
PetteriAimonen pushed a commit to nanopb/nanopb that referenced this issue Nov 9, 2023
With Bazel 7.0.0, nocopts has been removed and is no longer supported.
The correct way to handle this is to subtract features using the
`features` attribute (see bazelbuild/bazel#8706).
@keith
Copy link
Member

keith commented Jun 27, 2024

FWIW this attribute is still in the docs which might confuse some folks

@hvadehra
Copy link
Member

hvadehra commented Jul 1, 2024

cc @pzembrod

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
incompatible-change Incompatible/breaking change P1 I'll work on this now. (Assignee required) team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

No branches or pull requests