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

Flip --incompatible_enable_cc_toolchain_resolution #19441

Closed
wants to merge 5 commits into from

Conversation

comius
Copy link
Contributor

@comius comius commented Sep 7, 2023

RELNOTES[INC]: Flip incompatible_enable_cc_toolchain_resolution (#7260)

@comius comius requested a review from lberki as a code owner September 7, 2023 10:38
@github-actions github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-Rules-CPP Issues for C++ rules labels Sep 7, 2023
@comius
Copy link
Contributor Author

comius commented Sep 7, 2023

Known downstream issues:

@lberki
Copy link
Contributor

lberki commented Sep 7, 2023

I love this! What's the plan for all the failing projects on BuildKite, though?

@comius
Copy link
Contributor Author

comius commented Sep 7, 2023

I love this! What's the plan for all the failing projects on BuildKite, though?

Read the comment above it explains all the details. TLDR big projects are fixed and work. There’s a problem: Apple and Swift, they only work without bzlmod.

@comius comius added the team-Configurability platforms, toolchains, cquery, select(), config transitions label Sep 7, 2023
@lberki
Copy link
Contributor

lberki commented Sep 7, 2023

Ah, I see. I assume the StarlarkCcCommonTest / JavaConfigurationTest breakages on non-Apple builds are just some sort of artifact? (I can't immediately connect them to Apple or Android rules...)

I assume #17133 fixes the bzlmod/C++ toolchain incompatibility, so as long as the above breakages are not a sign of anything malign, I'm good.

@comius
Copy link
Contributor Author

comius commented Sep 7, 2023

Ah, I see. I assume the StarlarkCcCommonTest / JavaConfigurationTest breakages on non-Apple builds are just some sort of artifact? (I can't immediately connect them to Apple or Android rules...)

Those are just unit tests that have small Starlark rules without declaring C++ toolchains. Easy fix and not a source for concern.

@lberki
Copy link
Contributor

lberki commented Sep 7, 2023

(then why not fix them before send out the change for review?)

In any case, looks good to me -- this is a great step forward :)

@comius comius requested a review from a team as a code owner September 7, 2023 14:47
@keith
Copy link
Member

keith commented Sep 7, 2023

Testing this locally I found this issue bazelbuild/rules_swift#1103, might just be a change we need to make though

@comius comius added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Sep 8, 2023
@iancha1992
Copy link
Member

@comius I was told to cancel the last CL. Do you want me to try and recreate a CL or are you creating on your own?

@comius
Copy link
Contributor Author

comius commented Sep 11, 2023

@comius I was told to cancel the last CL. Do you want me to try and recreate a CL or are you creating on your own?

I’m creating my own cl. Unexpectedly there are more internal fixes. Unit tests have slided.

Also I will most likely hold off submitting/merging the PR until we at leas investigate apple and swift rules problems.

@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Sep 15, 2023
@brentleyjones
Copy link
Contributor

This is breaking downstream (rules_xcodeproj example): https://app.buildbuddy.io/invocation/d7f1ffff-e881-4457-be98-7e302cb62a50

@comius
Copy link
Contributor Author

comius commented Sep 15, 2023

This is breaking downstream (rules_xcodeproj example): https://app.buildbuddy.io/invocation/d7f1ffff-e881-4457-be98-7e302cb62a50

Ah, I couldn’t have known this, because it’s not running on buildkite.

I can help with investigation, however I probably also can’t fix the world myself. There’s always an option to unflip the flag until we find a solution

@brentleyjones
Copy link
Contributor

No problem. I want to get rules_xcodeproj to be part of Bazel CI in some capacity, but haven't found a way yet. Regardless, our CI will catch things like this and I can report them.

I don't think we are doing anything special, and this will probably manifest in most Apple projects. So figuring it out for this case should unblock the rest.

@brentleyjones
Copy link
Contributor

@brentleyjones
Copy link
Contributor

clang: error: using sysroot for 'AppleTVSimulator' but targeting 'MacOSX' [-Werror,-Wincompatible-sysroot]

@brentleyjones
Copy link
Contributor

brentleyjones commented Sep 18, 2023

For the rules_xcodeproj case, it looks like an issue with Bzlmod. If you clone git@github.com:MobileNativeFoundation/rules_xcodeproj.git, cd examples/intergration, and then run:

USE_BAZEL_VERSION=last_green bazel build //CommandLine/CommandLineTool:tool.library

it fails with:

$ USE_BAZEL_VERSION=last_green bazel build //CommandLine/CommandLineTool:tool.library
WARNING: Build options --@rules_xcodeproj//xcodeproj:extra_generator_flags, --apple_crosstool_top, --crosstool_top, and 3 more have changed, discarding analysis cache (this can be expensive, see https://bazel.build/advanced/performance/iteration-speed).
ERROR: /Users/brentley/Developer/rules_xcodeproj/examples/integration/CommandLine/CommandLineToolLib/BUILD:3:13: in objc_library rule //CommandLine/CommandLineToolLib:lib_defines: 
Traceback (most recent call last):
        File "/virtual_builtins_bzl/common/objc/objc_library.bzl", line 61, column 52, in _objc_library_impl
        File "/virtual_builtins_bzl/common/objc/semantics.bzl", line 54, column 13, in _check_toolchain_supports_objc_compile
Error in fail: Compiling objc_library targets requires the Apple CC toolchain which can be found here: https://github.com/bazelbuild/apple_support#toolchain-setup
ERROR: /Users/brentley/Developer/rules_xcodeproj/examples/integration/CommandLine/CommandLineToolLib/BUILD:3:13: Analysis of target '//CommandLine/CommandLineToolLib:lib_defines' failed
ERROR: Analysis of target '//CommandLine/CommandLineTool:tool.library' failed; build aborted: Analysis failed
INFO: Elapsed time: 1.708s
INFO: 0 processes.
ERROR: Build did NOT complete successfully

If you then add --config=nobzlmod, it doesn't fail:

$ USE_BAZEL_VERSION=last_green bazel build //CommandLine/CommandLineTool:tool.library --config=nobzlmod
WARNING: Build options --@rules_xcodeproj//xcodeproj:extra_generator_flags, --apple_crosstool_top, --crosstool_top, and 3 more have changed, discarding analysis cache (this can be expensive, see https://bazel.build/advanced/performance/iteration-speed).
INFO: Analyzed target //CommandLine/CommandLineTool:tool.library (77 packages loaded, 572 targets configured).
INFO: Found 1 target...
Target //CommandLine/CommandLineTool:tool.library up-to-date:
  /Users/brentley/Developer/rules_xcodeproj/examples/integration/bazel-output-base/execroot/__main__/bazel-out/darwin_arm64-fastbuild/bin/CommandLine/CommandLineTool/libtool.library.a
INFO: Elapsed time: 0.548s, Critical Path: 0.00s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action

@comius
Copy link
Contributor Author

comius commented Sep 18, 2023

cc @meteorcloudy

It also looks like rules_apple works on incompatible pipeline. So it might also be due to bzlmod flip.

@brentleyjones
Copy link
Contributor

brentleyjones commented Sep 18, 2023

The default for --enable_bzlmod hasn't changed yet, and rules_apple doesn't enable it for any of its test. So I think the rules_apple one is a platforms related bug. Here's it failing on downsteam pipeline: https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/3314#018aa60c-9646-4249-b41c-6175aea97711. Not sure why that is different from https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/1652#018aa5d1-07de-448b-8cda-96802d7911e6.

@brentleyjones
Copy link
Contributor

brentleyjones commented Sep 18, 2023

Actually @meteorcloudy, there is a bug with the incompatible pipeline when only tests fail. You can see at the end of the one I linked above, it fails both paths (with new flags and without them):

With:

(03:08:56) ERROR: /private/var/tmp/_bazel_buildkite/117b4e0778316663e4de4f84f066ca76/external/rules_java_builtin/java/private/native.bzl:25:22: java_common may only be used from one of the following repositories or prefixes: @_builtins//, @bazel_tools//, @rules_java//, tools/build_defs/java. It may be temporarily re-enabled for general use by setting --incompatible_stop_exporting_language_modules=false
--
  | (03:08:56) ERROR: /private/var/tmp/_bazel_buildkite/117b4e0778316663e4de4f84f066ca76/external/rules_java_builtin/java/private/native.bzl:28:18: JavaInfo may only be used from one of the following repositories or prefixes: @_builtins//, @bazel_tools//, @rules_java//, tools/build_defs/java. It may be temporarily re-enabled for general use by setting --incompatible_stop_exporting_language_modules=false
  | (03:08:56) ERROR: /private/var/tmp/_bazel_buildkite/117b4e0778316663e4de4f84f066ca76/external/rules_java_builtin/java/private/native.bzl:31:24: JavaPluginInfo may only be used from one of the following repositories or prefixes: @_builtins//, @bazel_tools//, @rules_java//, tools/build_defs/java. It may be temporarily re-enabled for general use by setting --incompatible_stop_exporting_language_modules=false
  | (03:08:56) ERROR: Error computing the main repository mapping: at /private/var/tmp/_bazel_buildkite/117b4e0778316663e4de4f84f066ca76/external/rules_java_builtin/toolchains/local_java_repository.bzl:17:6: at /private/var/tmp/_bazel_buildkite/117b4e0778316663e4de4f84f066ca76/external/rules_java_builtin/java/defs.bzl:16:6: compilation of module 'java/private/native.bzl' failed

Without:

Executed 23 out of 875 tests: 851 tests pass, 21 fail locally and 3 were skipped.
--
  | There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are.
  | (03:14:07) INFO: Build Event Protocol files produced successfully.
  | Failure: Command failed, even without incompatible flags.

@meteorcloudy
Copy link
Member

I see, bazelisk-plus-incompatible-flags pipeline sometimes suffers from caching issue, it seems --incompatible_enable_cc_toolchain_resolution didn't trigger anything to rebuild during the build step. I added BAZELISK_CLEAN=1 to the pipeline which will cause bazelisk to run bazel clean --expunge between builds.

But this problem should be easy to reproduce locally, did anyone look into what's causing the breakage?

@meteorcloudy
Copy link
Member

meteorcloudy commented Sep 19, 2023

I guess the rules_xcodeproj failure is related to bazelbuild/rules_swift#1106 (comment)
@brentleyjones Can you try if registering the apple toolchain in the root module fixes the problem?

I have a fix for Bazel, but it'll need bazelbuild/rules_cc#196 and a new rules_cc version. /cc @buildbreaker2021

@buildbreaker2021
Copy link
Contributor

Thanks for the fix Yun! I will merge the PR and do the release today or tomorrow - unless it is urgent.

@brentleyjones
Copy link
Contributor

@meteorcloudy Yes, manually registering the toolchain fixed the bzlmod issue.

As for the incompatible-flags thing, it wasn't a caching issue. The bazel --migrate build passed, but bazel --migrate test failed, and that doesn't fail the pipeline it seems. The rules_apple failures are only in the running of tests.

@meteorcloudy
Copy link
Member

I see, bazelisk-plus-incompatible-flags uses last_green, so it's probably using a bazel binary already with incompatible_enable_cc_toolchain_resolution turned on, even without the flag.

The downstream test with Bazel@HEAD gives the same result:
https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/3316#018aabc8-d3ef-4422-b698-9e0c335f4d0f

We need to debug what caused the test failure.

@brentleyjones
Copy link
Contributor

Cached test results were because the bazelisk provided command-line flags didn't make it to the inner bazel test that those tests invoke: https://github.com/bazelbuild/rules_apple/blob/1228cf87883e1e76187a4ccef853a0844c1e4975/test/apple_shell_testutils.sh#L498-L510

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Configurability platforms, toolchains, cquery, select(), config transitions team-Rules-CPP Issues for C++ rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants