tweag / rules_sh

Shell rules for Bazel
Apache License 2.0
42 stars 3 forks source link

Build and test commands fail when bzlmod is enabled. #42

Closed z8v closed 1 year ago

z8v commented 1 year ago

Describe the bug

The build and test commands fail when bzlmod is enabled.

To Reproduce

$ bazel build --experimental_enable_bzlmod --build_tag_filters=-stardoc_generation //...
ERROR: .../rules_sh/tests/sh_binaries/BUILD.bazel:3:23: While resolving toolchains for target //tests/sh_binaries:windows_strip_exe: com.google.devtools.build.lib.packages.BuildFileNotFoundException: no such package '@bazel_skylib~1.2.1//tests/sh_binaries': BUILD file not found in directory 'tests/sh_binaries' of external repository @bazel_skylib~1.2.1. Add a BUILD file to a directory to mark it as a package.
ERROR: Analysis of target '//tests/sh_binaries:windows_strip_exe_test' failed; build aborted: 
INFO: Elapsed time: 0.064s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded, 3 targets configured)

Expected behavior

The build should finish without errors.

Environment

aherrmann commented 1 year ago

Version of the rules: master

I assume 25e6d80250aa130b112976c47635e686d99f984c.


Interesting, I poked a bit at this and it seems to have to do with the platform transitions on the windows_strip_exe_test and linux_keep_exe_test rules.

I've applied the following patch to see if a fully qualified label helps (I also updated skylib, just to be sure this hasn't been fixed in the meantime)

diff --git a/MODULE.bazel b/MODULE.bazel
index 782f0c6..d592a79 100644
--- a/MODULE.bazel
+++ b/MODULE.bazel
@@ -3,7 +3,7 @@ module(
     version = "0.3.0",
     compatibility_level = 0,
 )
-bazel_dep(name = "bazel_skylib", version = "1.2.1")
+bazel_dep(name = "bazel_skylib", version = "1.4.0")
 bazel_dep(name = "platforms", version = "0.0.5")

 sh_configure = use_extension("//bzlmod:extensions.bzl", "sh_configure")
diff --git a/tests/sh_binaries/sh_binaries_test.bzl b/tests/sh_binaries/sh_binaries_test.bzl
index f9df010..97c8f88 100644
--- a/tests/sh_binaries/sh_binaries_test.bzl
+++ b/tests/sh_binaries/sh_binaries_test.bzl
@@ -916,7 +916,7 @@ def _windows_strip_exe_test_impl(ctx):
 windows_strip_exe_test = analysistest.make(
     _windows_strip_exe_test_impl,
     config_settings = {
-        "//command_line_option:platforms": "//tests/sh_binaries:windows",
+        "//command_line_option:platforms": "@rules_sh//tests/sh_binaries:windows",
     },
     # TODO[AH] The target_under_test should be provided in the exec
     # configuration to be sure that the Windows platform check considers the
@@ -958,7 +958,7 @@ def _linux_keep_exe_test_impl(ctx):
 linux_keep_exe_test = analysistest.make(
     _linux_keep_exe_test_impl,
     config_settings = {
-        "//command_line_option:platforms": "//tests/sh_binaries:linux",
+        "//command_line_option:platforms": "@rules_sh//tests/sh_binaries:linux",
     },
     # TODO[AH] The target_under_test should be provided in the exec
     # configuration to be sure that the Windows platform check considers the

However, this still fails

$ USE_BAZEL_VERSION=6.0.0 bazel build --enable_bzlmod --build_tag_filters=-stardoc_generation //...
ERROR: /home/aj/tweag.io/bazel/rules_sh/tests/sh_binaries/BUILD.bazel:3:23: While resolving toolchains for target //tests/sh_binaries:windows_strip_exe: com.google.devtools.build.lib.packages.BuildFileNotFoundException: no such package '@[unknown repo 'rules_sh' requested from @bazel_skylib~1.4.0]//tests/sh_binaries': The repository '@[unknown repo 'rules_sh' requested from @bazel_skylib~1.4.0]' could not be resolved: No repository visible as '@rules_sh' from repository '@bazel_skylib~1.4.0'
ERROR: Analysis of target '//tests/sh_binaries:windows_strip_exe_test' failed; build aborted:
INFO: Elapsed time: 0.115s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (0 packages loaded, 0 targets configured)
    Fetching repository @rules_java~5.3.5~toolchains~remotejdk11_linux; starting
    Fetching repository @bazel_tools~cc_configure_extension~local_config_cc; starting

It looks like this transition is evaluated in the scope of @bazel_skylib and doesn't have access to labels coming from the outside. This seems problematic, as it seems to imply that it's impossible to transition to anything involving labels not explicitly imported into bazel_skylib.

Indeed, if I patch skylib to define the needed platforms and change the tests to use these then these tests pass (while other still fail): patch to skylib

diff --git a/BUILD b/BUILD
index 11751a9..00de021 100644
--- a/BUILD
+++ b/BUILD
@@ -4,6 +4,20 @@ licenses(["notice"])

 package(default_visibility = ["//visibility:public"])

+platform(
+    name = "linux",
+    constraint_values = [
+        "@platforms//os:linux",
+    ],
+)
+
+platform(
+    name = "windows",
+    constraint_values = [
+        "@platforms//os:windows",
+    ],
+)
+
 # buildifier: disable=skylark-comment
 # gazelle:exclude skylark_library.bzl

diff --git a/MODULE.bazel b/MODULE.bazel
index 1d18e9a..4b8464c 100644
--- a/MODULE.bazel
+++ b/MODULE.bazel
@@ -22,8 +22,8 @@ bazel_dep(name = "rules_cc", version = "0.0.2", dev_dependency = True)
 # Needed for bazelci and for building distribution tarballs.
 # If using an unreleased version of bazel_skylib via git_override, apply
 # MODULE.bazel-remove-override.patch to remove the following lines:
-bazel_dep(name = "bazel_skylib_gazelle_plugin", dev_dependency = True)
-local_path_override(
-    module_name = "bazel_skylib_gazelle_plugin",
-    path = "gazelle",
-)

patch to rules_sh

diff --git a/MODULE.bazel b/MODULE.bazel
index 782f0c6..417a27e 100644
--- a/MODULE.bazel
+++ b/MODULE.bazel
@@ -3,7 +3,8 @@ module(
     version = "0.3.0",
     compatibility_level = 0,
 )
-bazel_dep(name = "bazel_skylib", version = "1.2.1")
+bazel_dep(name = "bazel_skylib", version = "1.4.0")
+local_path_override(module_name = "bazel_skylib", path = "/home/aj/tweag.io/bazel/bazel-skylib")
 bazel_dep(name = "platforms", version = "0.0.5")

 sh_configure = use_extension("//bzlmod:extensions.bzl", "sh_configure")
diff --git a/tests/sh_binaries/sh_binaries_test.bzl b/tests/sh_binaries/sh_binaries_test.bzl
index f9df010..811851a 100644
--- a/tests/sh_binaries/sh_binaries_test.bzl
+++ b/tests/sh_binaries/sh_binaries_test.bzl
@@ -916,7 +916,7 @@ def _windows_strip_exe_test_impl(ctx):
 windows_strip_exe_test = analysistest.make(
     _windows_strip_exe_test_impl,
     config_settings = {
-        "//command_line_option:platforms": "//tests/sh_binaries:windows",
+        "//command_line_option:platforms": "@bazel_skylib//:windows",
     },
     # TODO[AH] The target_under_test should be provided in the exec
     # configuration to be sure that the Windows platform check considers the
@@ -958,7 +958,7 @@ def _linux_keep_exe_test_impl(ctx):
 linux_keep_exe_test = analysistest.make(
     _linux_keep_exe_test_impl,
     config_settings = {
-        "//command_line_option:platforms": "//tests/sh_binaries:linux",
+        "//command_line_option:platforms": "@bazel_skylib//:linux",
     },
     # TODO[AH] The target_under_test should be provided in the exec
     # configuration to be sure that the Windows platform check considers the

For context the tests are defined here and here.

aherrmann commented 1 year ago

@z8v thanks to the help from @Wyverald on the Bazel Slack. Wrapping the labels in str(Label(...)) turns them into canonical form so that they are no longer subject to repo remapping. So changing the config_settings to

"//command_line_option:platforms": str(Label("//tests/sh_binaries:linux")),
"//command_line_option:platforms": str(Label("//tests/sh_binaries:windows")),

respectively should fix the issue.

z8v commented 1 year ago

I'm closing this now that #44 is merged.