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_allow_tags_propagation #8830

Closed
2 of 4 tasks
ishikhman opened this issue Jul 9, 2019 · 20 comments
Closed
2 of 4 tasks

incompatible_allow_tags_propagation #8830

ishikhman opened this issue Jul 9, 2019 · 20 comments
Assignees
Labels
incompatible-change Incompatible/breaking change migration-ready Incompatible flag is ready for migration with Bazel rolling releases or Bazel@last_green P2 We'll consider working on this in future. (Assignee optional) team-Remote-Exec Issues and PRs for the Execution (Remote) team type: feature request

Comments

@ishikhman
Copy link
Contributor

ishikhman commented Jul 9, 2019

Tags are not currently being propagated from a bazel's target to the actions' execution requirements. which would be very useful in order to be able to mar certain targets as 'no-remote' or 'no-cache'. At the moment, this is possible only on action/rule level or via adjustments to the rules.
Detailed description of the issue is in #7766.
Design is described in the proposal.

In order to make the transition smoother, a flag --incompatible_allow_tags_propagation has been introduced.

What does it change?

  • If the flag --incompatible_allow_tags_propagation set to true, bazel will take tags specified on target level, filter them based on prefix and propagate to the actions, that are created for the target.
    Propagated prefixes: "block-", "requires-", "no-", "supports-", "disable-", "local", "cpu:"

  • If set to false tags are not propagated.

Migration notes

Check the tags with the prefixes: "block-", "requires-", "no-", "supports-", "disable-", "local", "cpu:". Most likely they were not propagated and with the flag --incompatible_allow_tags_propagation they will be, therefore the build behavior might change.

Plan

By Bazel 1.0: Make functionality usable

  • Starlark rules are able to propagate tags under --incompatible_allow_tags_propagation flag
  • Native rules (C++, for example) are able to propagate tags under --incompatible_allow_tags_propagation flag

After Bazel 1.0:: Stabilize

  • fix everything that is failing/unstable incrementally
  • move 'tags propagation' from experimental to supported feature
@ishikhman ishikhman added incompatible-change Incompatible/breaking change team-Remote-Exec Issues and PRs for the Execution (Remote) team labels Jul 9, 2019
@ishikhman ishikhman self-assigned this Jul 9, 2019
bazel-io pushed a commit that referenced this issue Jul 17, 2019
reverted rollback of tags propagation: 29eecb5

Tags declared on targets are not propagated to actions and therefore are not taken into consideration by bazel. This causes some issues, for instance, target marked with a tag 'no-remote' will still be executed remotely.
As it was agreed in the design doc (see #7766 for a link), set of tags to be propagated to actions as a first iteration.
This change is responsible for that first step for the Starlark Rules.

RELNOTES: tags 'no-remote', 'no-cache', 'no-remote-cache', 'no-remote-exec', 'no-sandbox' are propagated now to the actions from targets when '--ncompatible_allow_tags_propagation' flag set to true. See #8830.

Closes #7766
Related to #8830

Closes #8829.

PiperOrigin-RevId: 258521443
@laurentlb
Copy link
Contributor

What's the status of this flag?
If it's ready, add the label migration-ready (or migration-0.x) and breaking-change-1.0.

@dslomov
Copy link
Contributor

dslomov commented Aug 5, 2019

What is the extent of the breakage? How hard it is to migrate? Do we have tooling available?

@ishikhman
Copy link
Contributor Author

ishikhman commented Aug 6, 2019

Before the change: tags were ignored for starlark rules
After the change: tags are propagated for starlark rules
Migration would be needed only for the cases where tags were specified for targets where they are not actually required.

What do you mean by 'tooling'?

Note: according to https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/196 there are no breakages for downstream projects

@dslomov
Copy link
Contributor

dslomov commented Aug 6, 2019

What do you mean by 'tooling'?

Migration tooling (of any kind) - but I do not think it is posisble :)
Breakage does seem non-existent

@ishikhman ishikhman added P1 I'll work on this now. (Assignee required) WIP and removed breaking-change-1.0 incompatible-change Incompatible/breaking change labels Aug 19, 2019
ishikhman pushed a commit to ishikhman/bazel that referenced this issue Aug 19, 2019
…tags_propagation'

RELNOTES: use 'experimental_allow_tags_propagation' to propagate tags to actions from target or rules. Tags that can be propagated should have one of the following prefixed: 'no-', 'requires-', 'supports-', 'block-', 'disable-', 'cpu:'. See bazelbuild#8830 for details
@ishikhman ishikhman changed the title incompatible_allow_tags_propagation experimental_allow_tags_propagation Aug 19, 2019
bazel-io pushed a commit that referenced this issue Aug 21, 2019
renamed `--incompatible_allow_tags_propagation` to `--experimental_allow_tags_propagation`

RELNOTES[NEW]: tags: use `--experimental_allow_tags_propagation` flag to propagate tags to the action's execution requirements from targets. Such tags should start with: `no-`, `requires-`, `supports-`, `block-`, `disable-`, `cpu:`. See #8830 for details.

Note: this is only 'starlark' rules part. Native rules change will come as a seperate change under the same flag.

Closes #9197.

PiperOrigin-RevId: 264573625
bazel-io pushed a commit that referenced this issue Aug 28, 2019
Baseline: 6c5ef53

Cherry picks:

   + 338829f:
     Fix retrying of SocketTimeoutExceptions in HttpConnector
   + 14651cd:
     Fallback to next urls if download fails in HttpDownloader
   + b7d300c:
     Fix incorrect stdout/stderr in remote action cache. Fixes #9072
   + 9602176:
     Automated rollback of commit
     0f0a0d5.
   + da557f9:
     Windows: fix "bazel run" argument quoting
   + ef8b6f6:
     Return JavaInfo from java proto aspects.
   + 209175f:
     Revert back to the old behavior of not creating a proto source
     root for generated .proto files.
   + 644060b:
     Fix PatchUtil for parsing special patch format
   + 067040d:
     Put the removal of the legacy repository-relative proto path
     behind the --incompatible_generated_protos_in_virtual_imports
     flag.
   + 76ed014:
     repository mapping lookup: convert to canonical name first

Important changes:

  - rule_test: fix Bazel 0.27 regression ("tags" attribute was
    ingored, #8723
  - Adds --incompatible_enable_execution_transition, which enables
    incremental migration of host attributes to exec attributes.
  - objc_proto_library rule has been deleted from Bazel.
  - repository_ctx.read is no longer restricted to files
        in the repository contructed.
  - tags 'no-remote', 'no-cache', 'no-remote-cache',
    'no-remote-exec', 'no-sandbox' are propagated now to the actions
    from targets when '--ncompatible_allow_tags_propagation' flag set
    to true. See #8830.
  - Adds flag
    --//tools/build_defs/pkg:incompatible_no_build_defs_pkg. This
    flag turns off the rules //tools/build_defs/pkg:{pkg_deb,
    pkg_rpm, pkg_tar}.
  - The Android NDK is now integrated with toolchains. To use them,
    pass the `--extra_toolchains=@androidndk//:all` flag or register
    them in your WORKSPACE with
    `register_toolchains("@androidndk//:all")`.
  - Stdout and stderr are checked to determine if output is going to a
    terminal. `--is_stderr_atty` is deprecated and `--isatty` is
    undeprecated.
  - --incompatible_load_proto_rules_from_bzl was added to forbid
    loading the native proto rules directly. See more on tracking
    issue #8922
  - Docker Sandbox now respects remote_default_platform_properties
  - pkg_deb, pkg_rpm & pkg_tar deprecation plan announced in the
    documentation.
  - The new java_tools release:
    * fixes #8614
    * exposes a new toolchain `@java_tools//:prebuilt_toolchain`
    which is using all the pre-built tools, including singlejar and
    ijar, even on remote execution. This toolchain should be used
    only when host and execution platform are the same, otherwise the
    binaries will not work on the execution platform.
  - java_common.compile supports specifying
    annotation_processor_additional_inputs and
    annotation_processor_additional_outputs for the Java compilation
    action for supporting annotation processors that consume or
    produce artifacts. Fixes #6415
  - There is now documentation on optimizing Android app build
    performance. Read it at
    https://docs.bazel.build/versions/0.29.0/android-build-performance
    .html
  - Execution log now respects --remote_default_platform_properties
  - Include a link to the relevant documenation on transitive Python
    version errors.
  - New incompatible flag
    --incompatible_disable_target_provider_fields removes the ability
    (in Starlark) to access a target's providers via the field syntax
    (for example, `ctx.attr.dep.my_provider`). The provider-key
    syntax should be used instead (for example,
    `ctx.attr.dep[MyProvider]`). See
    #9014 for details.
  - A new platform exec_properties is added to replace
    remote_execution_properties.
  - Added --incompatible_load_python_rules_from_bzl, which will be
    flipped in Bazel 1.0. See
    #9006.
  - add --break_build_on_parallel_dex2oat_failure to shortcut tests
    on dex2oat errors

This release contains contributions from many people at Google, as well as Alexander Ilyin, Arek Sredzki, Artem Zinnatullin, Benjamin Peterson, Fan Wu, John Millikin, Loo Rong Jie, Marwan Tammam, Oscar Bonilla, Peter Mounce, Sergio Rodriguez Orellana, Takeo Sawada, and Yannic Bonenberger.
@aiuto
Copy link
Contributor

aiuto commented Feb 15, 2022

@katre @gregestren FYI.

@katre
Copy link
Member

katre commented Feb 15, 2022

The separation between exec properties and exec requirements has always been confusing to me. I recently made a change to that all exec properties become exec requirements, but not vice versa. Does it make sense to use exec requirements to populate PlatformProto?

alexeagle added a commit to bazelbuild/rules_docker that referenced this issue Mar 16, 2022
These were introduced to reduce load on a remote-cache instance to avoid network saturation.
A month later, a feature was added in one remote-cache implementatation which provides a different fix:
buchgr/bazel-remote#440 rejects large input files on upload.

In practice, while these action do often produce huge outputs, they are also slow to re-execute.
In many cases it's worth it to use a remote-cache for RunAndCommitLayer in particular to avoid a local rebuild
even though it's a large network fetch.
Currently users can't configure this because we've hardcoded the values. If they do want to keep the
no-remote-cache execution requirement, they can do this via a tag (provided they opt-in to
experimental_allow_tags_propagation, see bazelbuild/bazel#8830)

#1856 (comment) is an example of a user
asking for these to be removed.
alexeagle added a commit to bazelbuild/rules_docker that referenced this issue Mar 16, 2022
These were introduced to reduce load on a remote-cache instance to avoid network saturation.
A month later, a feature was added in one remote-cache implementatation which provides a different fix:
buchgr/bazel-remote#440 rejects large input files on upload.

In practice, while these action do often produce huge outputs, they are also slow to re-execute.
In many cases it's worth it to use a remote-cache for RunAndCommitLayer in particular to avoid a local rebuild
even though it's a large network fetch.
Currently users can't configure this because we've hardcoded the values. If they do want to keep the
no-remote-cache execution requirement, they can do this via a tag (provided they opt-in to
experimental_allow_tags_propagation, see bazelbuild/bazel#8830)

#1856 (comment) is an example of a user
asking for these to be removed.
gravypod pushed a commit to bazelbuild/rules_docker that referenced this issue Mar 16, 2022
These were introduced to reduce load on a remote-cache instance to avoid network saturation.
A month later, a feature was added in one remote-cache implementatation which provides a different fix:
buchgr/bazel-remote#440 rejects large input files on upload.

In practice, while these action do often produce huge outputs, they are also slow to re-execute.
In many cases it's worth it to use a remote-cache for RunAndCommitLayer in particular to avoid a local rebuild
even though it's a large network fetch.
Currently users can't configure this because we've hardcoded the values. If they do want to keep the
no-remote-cache execution requirement, they can do this via a tag (provided they opt-in to
experimental_allow_tags_propagation, see bazelbuild/bazel#8830)

#1856 (comment) is an example of a user
asking for these to be removed.
dfinity-bot pushed a commit to dfinity/ic that referenced this issue May 23, 2022
Propagate tags.

bazelbuild/bazel#8830

Closes IDX-2223 

Closes IDX-2223

See merge request dfinity-lab/public/ic!5158
rickystewart added a commit to rickystewart/cockroach that referenced this issue Jun 13, 2022
On my machine, building the `GoPath` takes a lot of time but it's almost
all in caching apparently. I turn off caching with the `no-remote-cache`
tag, but this has no effect unless we also set
`--experimental_allow_tags_propagation`. See the
[issue upstream](bazelbuild/bazel#8830).
I suspect that `--experimental_allow_tags_propagation` will be useful in
other places as well for our fine-tuning.

Release note: None
craig bot pushed a commit to cockroachdb/cockroach that referenced this issue Jun 14, 2022
82843: bazel: set `no-remote-cache` for `GoPath`, propagate tags r=rail a=rickystewart

On my machine, building the `GoPath` takes a lot of time but it's almost
all in caching apparently. I turn off caching with the `no-remote-cache`
tag, but this has no effect unless we also set
`--experimental_allow_tags_propagation`. See the
[issue upstream](bazelbuild/bazel#8830).
I suspect that `--experimental_allow_tags_propagation` will be useful in
other places as well for our fine-tuning.

Release note: None

Co-authored-by: Ricky Stewart <[email protected]>
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
    Tags are not propagated from targets to actions for Android rules. bazelbuild/bazel#8830

    This PR adds basic propagation of tags from the android_binary target it's actions.

    Testing with aquery with the Android repo:

    ```
    bazel aquery 'mnemonic(RClassGenerator, //apps/foo)'
    bazel aquery 'mnemonic(JavaDeployJar, //apps/foo)'
    bazel aquery 'mnemonic(ApkBuilder, //apps/foo)'
    ```

    Closes #13093.

    PiperOrigin-RevId: 360836954
@fmeum
Copy link
Collaborator

fmeum commented May 28, 2023

@coeuvre I would see a lot of value in flipping this. It may not even require much effort apart from moving it to the common flags. What do you think?

@coeuvre
Copy link
Member

coeuvre commented May 30, 2023

SGTM. I would love to see this is flipped.

@fmeum
Copy link
Collaborator

fmeum commented May 31, 2023

@meteorcloudy Could you rename the issue to incompatible_allow_tags_propagation? I have a test PR for the flip at #18544.

@meteorcloudy meteorcloudy changed the title experimental_allow_tags_propagation incompatible_allow_tags_propagation May 31, 2023
@meteorcloudy meteorcloudy added P2 We'll consider working on this in future. (Assignee optional) migration-ready Incompatible flag is ready for migration with Bazel rolling releases or Bazel@last_green incompatible-change Incompatible/breaking change and removed P3 We're not considering working on this, but happy to review a PR. (No assignee) labels May 31, 2023
@meteorcloudy
Copy link
Member

Done, also added labels so that it'll be tested by the incompatible flag pipeline.

@sgowroji sgowroji self-assigned this Jun 5, 2023
copybara-service bot pushed a commit that referenced this issue Jun 16, 2023
Work towards #8830

Closes #18544.

PiperOrigin-RevId: 540884654
Change-Id: Ia027f73b5857a36f4798a14a46a63befbcc673da
@sgowroji
Copy link
Member

All test cases are getting skipped and one fails //src/test/shell/bazel:srcs_test https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/1621#018a159f-e7de-4b84-b1dc-00533b11d231/2174

@fmeum
Copy link
Collaborator

fmeum commented Aug 25, 2023

@meteorcloudy Should we close this issue since the flag has been flipped? Or do we leave it open until the flip has been released?

@meteorcloudy
Copy link
Member

This should be closed once it's flipped, although it would be even nicer to have an issue to track the removal of this flag, there are too many unused incompatible flags in Bazel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
incompatible-change Incompatible/breaking change migration-ready Incompatible flag is ready for migration with Bazel rolling releases or Bazel@last_green P2 We'll consider working on this in future. (Assignee optional) team-Remote-Exec Issues and PRs for the Execution (Remote) team type: feature request
Projects
None yet
Development

No branches or pull requests