Skip to content

Fixed incompatibility issues failing BCR dependents#13

Closed
UebelAndre wants to merge 1 commit into
jmillikin:trunkfrom
UebelAndre:trunk
Closed

Fixed incompatibility issues failing BCR dependents#13
UebelAndre wants to merge 1 commit into
jmillikin:trunkfrom
UebelAndre:trunk

Conversation

@UebelAndre

Copy link
Copy Markdown

@jmillikin

Copy link
Copy Markdown
Owner

I'm trying to verify the difference in behavior when @rules_cc comes implicitly from Bazel or explicitly from MODULE.bazel, so as to better understand the potential compatibility implications of adding load("@rules_cc//..."). Basically, testing whether this would be rules_flex v0.3.2 or v0.4.

It seems that even in the most recent release (8.2.1) the loads from @rules_cc don't need to be explicit, which is surprising. Am I doing this test incorrectly?

$ git diff
diff --git a/MODULE.bazel b/MODULE.bazel
index 348032e..b60f435 100644
--- a/MODULE.bazel
+++ b/MODULE.bazel
@@ -5,6 +5,7 @@ module(
 )
 
 bazel_dep(name = "rules_m4", version = "0.2.3")
+bazel_dep(name = "rules_shell", version = "0.4.0", dev_dependency = True)
 
 bazel_dep(
     name = "googletest",
diff --git a/tests/BUILD b/tests/BUILD
index bb6d5aa..b392c24 100644
--- a/tests/BUILD
+++ b/tests/BUILD
@@ -1,3 +1,4 @@
+load("@rules_shell//shell:sh_test.bzl", "sh_test")
 load("//flex:flex.bzl", "flex_cc_library")
 
 cc_library(
$ bazel version
Build label: 8.2.1
Build target: @@//src/main/java/com/google/devtools/build/lib/bazel:BazelServer
Build time: Thu Apr 17 18:22:55 2025 (1744914175)
Build timestamp: 1744914175
Build timestamp as int: 1744914175
$ bazel test --incompatible_autoload_externally= --incompatible_disable_autoloads_in_main_repo //tests:flex_test
INFO: Invocation ID: 4eb81fa4-25d8-42d3-ae42-4a9ffbceb375
INFO: Analyzed target //tests:flex_test (0 packages loaded, 0 targets configured).
INFO: Found 1 test target...
Target //tests:flex_test up-to-date:
  bazel-bin/tests/flex_test
INFO: Elapsed time: 0.170s, Critical Path: 0.00s
INFO: 1 process: 4 action cache hit, 1 internal.
INFO: Build completed successfully, 1 total action
//tests:flex_test                                               (cached) PASSED in 0.0s

Executed 0 out of 1 test: 1 test passes.

Same with a Bazel binary built at current master HEAD (bazelbuild/bazel@f9e3161).

@UebelAndre

UebelAndre commented Apr 18, 2025

Copy link
Copy Markdown
Author

Based on my read of bazelbuild/bazel#23043 I think this will only break once that flag is flipped or rules are not autoloaded. Both of which seem like they're scheduled for Bazel 9. I'm not sure if master is currently expected to have the full breadth of Bazel 9 changes or if this migration is on schedule.

I think this is a patch release since it looks like rules_cc is still referring to native rules:

@jmillikin

Copy link
Copy Markdown
Owner

Figured out the right flag syntax. Running Bazel with --incompatible_autoload_externally=-@rules_cc,-@rules_java does what I want (disabling the old built-in rules), but that revealed a new problem, which is that the non-native implementation of @rules_cc doesn't actually exist yet.

It's still just a thin wrapper around native.cc_{binary,library,test}, so disabling the built-in C++ rules breaks @rules_cc. This means there's no way to test that the loads of @rules_cc symbols have full coverage.

Interestingly disabling built-in Java rules also breaks @rules_cc, via //cc/toolchains:propeller_optimize.bzl.

On the bright side, @rules_shell works.

@UebelAndre would you be OK with new releases of @rules_{bison,flex,m4} that fixed the other stuff but left the handling of C++ and Java unchanged? Once the Bazel side gets closer to being ready I can cut new releases that switches over to @rules_cc / @rules_java.

@UebelAndre

UebelAndre commented Apr 19, 2025

Copy link
Copy Markdown
Author

@UebelAndre would you be OK with new releases of @rules_{bison,flex,m4} that fixed the other stuff but left the handling of C++ and Java unchanged? Once the Bazel side gets closer to being ready I can cut new releases that switches over to @rules_cc / @rules_java.

@jmillikin It's really your call but the way the incompatibility flags are implemented means that all consumers of the rules are gonna be forced to leave this one off and all module maintainers are gonna be forced to deal with the error showing up in their builds. I think it'd be better to add rules_cc and rules_java to eliminate the overhead for consumers and to then later bump the version of the rules to something that contains the pure Starlark implementation.

@UebelAndre

Copy link
Copy Markdown
Author

@jmillikin friendly ping here

@jmillikin

Copy link
Copy Markdown
Owner

Sorry for the delay, ran into an unexpected complication in a separate rules_bison bug report: jmillikin/rules_bison#19 (comment)

I'm going to be cutting two releases of each of the three projects, with the @rules_cc conversion in v0.(x+1). The PRs for rules_m4 are now live in the BCR repository:

rules_m4@0.2.5
rules_m4@0.3

I'm going to do the same for rules_flex later today.

For rules_bison, I think I've found a workaround for the issue above, which if successful will let me avoid thinking too hard about Bazel runfiles.

@jmillikin

jmillikin commented Apr 24, 2025

Copy link
Copy Markdown
Owner

Fixes for the various issues in this PR have been applied, and are available in release v0.3.2 (most of the fixes) and v0.4 (the switch to @rules_cc).

rules_flex@0.3.2
rules_flex@0.4

Thank you for the PR!

@jmillikin jmillikin closed this Apr 24, 2025
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.

2 participants