aspect-build / rules_js

High-performance Bazel rules for running Node.js tools and building JavaScript projects
https://docs.aspect.build/rules/aspect_rules_js
Apache License 2.0
310 stars 107 forks source link

[Bug]: bzlmod lockfile is not platform-invariant via npm extension's dependency on platform-specific yq #1880

Open willjschmitt opened 3 months ago

willjschmitt commented 3 months ago

What happened?

After trying to flip --lockfile_mode=error in our CI environment to try to enforce that the lockfile was appropriately updated within a PR, we found our Windows build was failing because the lockfile needed an update due to a platform-specific update for the yq toolchain dependency from the npm extension in rules_js. This means, in-general, commits submitted from windows devs and linux devs using --lockfile_mode=update will spuriously update the lockfile for unrelated changes, since each OS resolves to different yq toolchains.

After building on Windows with --lockfile_mode=update, the moduleDepGraph."@@aspect_rules_js~//npm:extensions.bzl%npm".general.recordedFileInputs and moduleDepGraph."@@aspect_rules_js~//npm:extensions.bzl%npm".general.recordedRepoMappingEntries entries in the lockfile updated with the following diff:

git diff
diff --git a/MODULE.bazel.lock b/MODULE.bazel.lock
index 45c5978..8b3c0e6 100644
--- a/MODULE.bazel.lock
+++ b/MODULE.bazel.lock
@@ -4753,7 +4753,7 @@
         "bzlTransitiveDigest": "95DPKNQTVRvKlYPcSf3TzPnzragr/FfG+tvoW80mBdA=",
         "recordedFileInputs": {
           "@@//pnpm-lock.yaml": "1e09e0ba3534328641caf9eac94634636e3354fbf7f7ae8e1260470bb34f5049",
-          "@@aspect_bazel_lib~~toolchains~yq_linux_amd64//yq": "042f7462ec8378f8b0d3bac85d1b1a063b63beef5d8e3826bb2377294116ae90"
+          "@@aspect_bazel_lib~~toolchains~yq_windows_amd64//yq.exe": "7d391921bf5481db063e2b0f043aab0ef2ddaa854ae0e03a4d6d9a1112d18fa8"
         },
         "recordedDirentsInputs": {},
         "envVariables": {},
@@ -84777,8 +84777,8 @@
           ],
           [
             "aspect_rules_js~",
-            "yq_linux_amd64",
-            "aspect_bazel_lib~~toolchains~yq_linux_amd64"
+            "yq_windows_amd64",
+            "aspect_bazel_lib~~toolchains~yq_windows_amd64"
           ],
           [
             "bazel_features~",

This might be an issue better suited for https://github.com/aspect-build/bazel-lib, but the extension itself is defined here, which has the dependency, so it might be something to be handled in the extension definition. I suspect this also might show up in different ways other extensions depend on any of the pre-built toolchains in bazel-lib

A side-note, but somewhat related: I also had a diff on the moduleDepGraph."@@aspect_rules_js~//npm:extensions.bzl%npm".general.recordedFileInputs."@@//pnpm-lock.yaml" entry on Windows, which was from line-ending differences. I can understand line endings changing the hash of the file, so I updated .gitattributes to force unix line-endings for the pnpm lock file. What I was surprised by, however, was that this lock file was the only instance I had of line-endings causing a diff in my lockfile, suggesting maybe there's a difference in how this file is handled from other bzlmod modules like rules_python and their requirements.txt files

Version

Development (host) and target OS/architectures:

Output of bazel --version: 7.1.1

Version of the Aspect rules, or other relevant rules from your WORKSPACE or MODULE.bazel file:

Language(s) and/or frameworks involved:

How to reproduce

No response

Any other information?

For now, we are going to make the linux-version of the MODULE.bazel.lock canonical and disable the --lockfile_mode=error flag on Windows CI.

It looks like rules_python had similar challenges with platform-specific inputs (specifically due to varying requirements.txt files across platforms, which kicked off support for os_dependent and arch_dependent to be introduced to bazel extension definitions (design doc and relevant bazelbuild/bazel issue discussing. rules_python initially added these arguments to quickly solve the issue in https://github.com/bazelbuild/rules_python/pull/1433, but later after cleanup removed them in the PRs solving https://github.com/bazelbuild/rules_python/issues/1643, so they didn't have the need any longer. Perhaps, a solve inspired by rules_python could be a path