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

[6.3.0] Adjust --top_level_targets_for_symlinks #18906

Closed
wants to merge 1 commit into from

Conversation

iancha1992
Copy link
Member

Adjust --top_level_targets_for_symlinks.

  • If all targets in $ bazel build //... have the top-level config, bazel-bin, etc. point to that config

  • If all targets in $ bazel build //... have the same transitioned config, bazel-bin, etc. point to that config

  • If targets in $ bazel build //... have mixed configs, bazel-bin, etc. are deleted

  • If all targets in $ bazel build //... have the top-level config, bazel-bin, etc. point to that config

  • If all targets in $ bazel build //... have the same transitioned config, bazel-bin, etc. point to that config

  • If targets in $ bazel build //... have mixed configs and at least one of them is the top-level config, bazel-bin, etc. point to the top-level config

  • If targets in $ bazel build //... have mixed configs and none are the top-level config, bazel-bin, etc. are deleted

Fixes #17081.

Commit ceb9955

Closes #18854.

PiperOrigin-RevId: 546938509
Change-Id: I75adf0b8c2094522125c5e65d8c450eb2436d392

Adjust `--top_level_targets_for_symlinks`.

- If all targets in `$ bazel build //...` have the top-level config, `bazel-bin`, etc. point to that config
- If all targets in `$ bazel build //...` have the same transitioned config, `bazel-bin`, etc. point to that config
- If targets in `$ bazel build //...` have mixed configs, `bazel-bin`, etc. are deleted

- If all targets in `$ bazel build //...` have the top-level config, `bazel-bin`, etc. point to that config
- If all targets in `$ bazel build //...` have the same transitioned config, `bazel-bin`, etc. point to that config
- If targets in `$ bazel build //...` have mixed configs and at least one of them is the top-level config, `bazel-bin`, etc. point to the top-level config
- If targets in `$ bazel build //...` have mixed configs and none are the top-level config, `bazel-bin`, etc. are deleted

Fixes bazelbuild#17081.

Closes bazelbuild#18854.

PiperOrigin-RevId: 546938509
Change-Id: I75adf0b8c2094522125c5e65d8c450eb2436d392
@iancha1992 iancha1992 added team-Configurability platforms, toolchains, cquery, select(), config transitions awaiting-review PR is awaiting review from an assigned reviewer labels Jul 11, 2023
@iancha1992 iancha1992 linked an issue Jul 11, 2023 that may be closed by this pull request
@iancha1992 iancha1992 enabled auto-merge (squash) July 11, 2023 20:27
@iancha1992
Copy link
Member Author

@fmeum @gregestren Looks like the tests are failing. Do you know why?

cc: @bazelbuild/triage

@gregestren
Copy link
Contributor

@fmeum @gregestren Looks like the tests are failing. Do you know why?

cc: @bazelbuild/triage

1cd3588 changed the affected code's method signature. I'd tentatively suggest merging that first but it looks like @joeleba has a bunch of commits on that file: https://github.com/bazelbuild/bazel/commits/26926ee068ce7c18ec9a8afee49e078fa7ab6a1c/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java I'd check with him to see what's safe.

If necessary we can probably merge this independently but we'd have to refactor some amount of code.

@gregestren
Copy link
Contributor

If necessary we can probably merge this independently but we'd have to refactor some amount of code.

I really just glanced at this but I suspect a tiny adjustment to this PR would also suffice, without having to worry about all those other commits. I'd still like to get @joeleba 's input first.

@joeleba
Copy link
Member

joeleba commented Jul 12, 2023

Sorry, I'm not familiar with this process. Are we trying to cherrypick your commit ceb9955 on top of a baseline foo, where foo comes before my commit 1cd3588?

@joeleba has a bunch of commits on that file: https://github.com/bazelbuild/bazel/commits/26926ee068ce7c18ec9a8afee49e078fa7ab6a1c/src/main/java/com/google/devtools/build/lib/buildtool/ExecutionTool.java I'd check with him to see what's safe.

It should be safe to merge 1cd3588. The other commits in the chain are independent of this.

@gregestren
Copy link
Contributor

Sounds good. @iancha1992 can we merge 1cd3588 first? Then this one should resolve.

@gregestren
Copy link
Contributor

Sorry, I'm not familiar with this process. Are we trying to cherrypick your commit ceb9955 on top of a baseline foo, where foo comes before my commit 1cd3588?

Yes.

@iancha1992
Copy link
Member Author

Sounds good. @iancha1992 can we merge 1cd3588 first? Then this one should resolve.

Looks like there's a conflicts with

  1. src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
    function "buildTargetsWithMergedAnalysisExecution", currently has the try block, and is quite different from the master's. For example, "() -> executionTool.prepareForExecution(request.getId()));", instead of () -> executionTool.prepareForExecution(request.getId(), executionTimer). And also "if (analysisAndExecutionResult == null)" does not exist currently in the same try block form the file. We may need another commit that address this change from the "buildTargetsWithMergedAnalysisExecution's try block".

@gregestren can you please take a look?

@gregestren
Copy link
Contributor

I'm going to manually resolve and move this over to #18916.

@iancha1992
Copy link
Member Author

Closing this now because new cherry-pick now pass the tests in: #18916

@iancha1992 iancha1992 closed this Jul 12, 2023
auto-merge was automatically disabled July 12, 2023 18:23

Pull request was closed

@iancha1992 iancha1992 removed team-Configurability platforms, toolchains, cquery, select(), config transitions awaiting-review PR is awaiting review from an assigned reviewer labels Jul 12, 2023
@iancha1992 iancha1992 removed the request for review from aranguyen July 12, 2023 18:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[6.3.0] Adjust --top_level_targets_for_symlinks
3 participants