Patch fails in rules_python @0.14.0

[bazelbuild]

Patch fails in rules_python @0.14.0

[fmeum]

@kormide Do you know what the difference is? An early EOF that Bazel's patch implementation doesn't accept?

@meteorcloudy The presubmit passed for this, which seems to indicate that the patch is applied in different places.

[kirschem]

Judging from the output, it looks like the presubmit uses the patch command instead of going through bazel's patch logic.

[kirschem]

I can reproduce this when adding the following test to bazel here:

  @Test
  public void testModuleExample() throws IOException, PatchFailedException {
    Path oldFile = scratch.file("/root/oldfile",
        "module(",
        "    name = \"rules_python\",",
        "    compatibility_level = 1,",
        "    version = \"0.0.0\",",
        ")"
    );
    Path patchFile =
        scratch.file(
            "/root/patchfile",
            "--- a/oldfile",
            "+++ b/oldfile",
            "@@ -1,7 +1,7 @@",
            " module(",
            "     name = \"rules_python\",",
            "     compatibility_level = 1,",
            "-    version = \"0.0.0\",",
            "+    version = \"0.14.0\",",
            " )"
        );
    PatchUtil.apply(patchFile, 1, root);
    assertThat(oldFile.exists()).isFalse();
  }

I think the patch should rather be

--- MODULE.bazel
+++ MODULE.bazel
@@ -1,4 +1,4 @@
 module(
     name = "rules_python",
     compatibility_level = 1,
-    version = "0.0.0",
+    version = "0.14.0",
 )

It is line 4 that we want to edit, somehow it is set to 7 and it still works with the patch command. Maybe the header has been counted too?

[fmeum]

CC @kormide

[fmeum]

I will try to understand why this applies with native patch and fails with Bazel's implementation, maybe we can make the latter one more lenient. Thanks for the test case, that will come in handy!

[kormide]

@kormide Do you know what the difference is? An early EOF that Bazel's patch implementation doesn't accept?

@meteorcloudy The presubmit passed for this, which seems to indicate that the patch is applied in different places.

I manually created this patch using git diff (then removing the prefixes). Not exactly sure how the line numbers were wrong, but I assumed it worked since it passed CI.

[meteorcloudy]

I think the reason the presubmit passed is because we only have bcr_test_module test in https://github.com/bazelbuild/bazel-central-registry/pull/286/files#diff-d0c31861747eedc28c0d4eeca25591a11e59acf25741f5e519f0963cf01a191aR1, which means the source was downloaded and patched by the bcr_presubmit.py script instead of Bazel itself. I would recommend to also specify some build targets testing in the presubmit.yml file (we probably should enforce this in the presubmit validations).

[fmeum]

@meteorcloudy I think I don't quite understand this process: Doesn't this only set up the test module in a way that doesn't go through Bazel? The test module should then cause the tested module to be loaded by Bazel and should thus have failed in the same way as reported in this issue.

If that isn't how it works, I would be more comfortable with making test module tests more realistic rather than requiring two different types of presubmit checks for decent protection against downstream breakages.

[Wyverald]

The test module should then cause the tested module to be loaded by Bazel and should thus have failed in the same way as reported in this issue.

IIUC the Python script downloads and unpacks the entire source archive, which contains both the test module and the module under test. The test module simply uses the module under test with a local path override.

If that isn't how it works, I would be more comfortable with making test module tests more realistic rather than requiring two different types of presubmit checks for decent protection against downstream breakages.

Agreed, I often find myself confused by the setup as well -- would be nice to unify the two ways somehow.

[meteorcloudy]

The test module can actually depend on the target module with a normal bazel_dep, which would work, but I think that makes it hard to run the test module locally at HEAD because you don't have the version available, then you'll need to add a local_path_override and remember to remove it before creating the release. Also it's hard to verify if a test module actually follows this in the BCR presubmit.

To make Bazel fetching the module source from the BCR, we'll need to build some targets from the module, but I cannot think of any targets that's guaranteed to be built successful. 🤔

[Wyverald]

We could actually just build @module_name//:whatever, which will probably fail since the target whatever doesn't exist, but by that time the module would have already been fetched. Admittedly a bit hacky, but it could work :P

[meteorcloudy]

Yeah.. we can do that to use Bazel to fetch the source, but then we'll need to distinguish the failure type by parsing the error message, which could work but not very nice 😅

[fmeum]

How about a wildcard with a more lightweight command such as bazel build --nobuild @module_name//... or bazel query @module_name//...? Or could modquery be used to trigger a fetch?

[meteorcloudy]

I just found out that bazel fetch also works with Bzlmod enabled, and bazel fetch @module_name//... seem to cause fewer packages being loaded that bazel build --nobuild @module_name//....

[meteorcloudy]

It seems bazel fetch @module_name//... may not even work for some existing modules in the BCR.
For example, with MODULE.bazel

bazel_dep(name = "rules_python", version = "0.15.0")

bazel fetch --enable_bzlmod @rules_python//... fails with

ERROR: error loading package under directory '': error loading package '@rules_python~0.15.0//examples/multi_python_versions/tests': Unable to find package for @[unknown repo 'python' requested from @rules_python~0.15.0]//3.9:defs.bzl: The repository '@[unknown repo 'python' requested from @rules_python~0.15.0]' could not be resolved: No repository visible as '@python' from repository '@rules_python~0.15.0'.
Loading: 4 packages loaded
    currently loading: @rules_python~0.15.0//examples ... (35 packages)

Even bazel fetch --enable_bzlmod @rules_python//:MODULE.bazel fails with

ERROR: error loading package '@rules_python~0.15.0//': Unable to find package for @[unknown repo 'bazel_gazelle' requested from @rules_python~0.15.0]//:def.bzl: The repository '@[unknown repo 'bazel_gazelle' requested from @rules_python~0.15.0]' could not be resolved: No repository visible as '@bazel_gazelle' from repository '@rules_python~0.15.0'.
Loading: 0 packages loaded
    currently loading: @rules_python~0.15.0//

[kormide]

It seems bazel fetch @module_name//... may not even work for some existing modules in the BCR. For example, with MODULE.bazel

bazel_dep(name = "rules_python", version = "0.15.0")

bazel fetch --enable_bzlmod @rules_python//... fails with

ERROR: error loading package under directory '': error loading package '@rules_python~0.15.0//examples/multi_python_versions/tests': Unable to find package for @[unknown repo 'python' requested from @rules_python~0.15.0]//3.9:defs.bzl: The repository '@[unknown repo 'python' requested from @rules_python~0.15.0]' could not be resolved: No repository visible as '@python' from repository '@rules_python~0.15.0'.
Loading: 4 packages loaded
    currently loading: @rules_python~0.15.0//examples ... (35 packages)

Even bazel fetch --enable_bzlmod @rules_python//:MODULE.bazel fails with

ERROR: error loading package '@rules_python~0.15.0//': Unable to find package for @[unknown repo 'bazel_gazelle' requested from @rules_python~0.15.0]//:def.bzl: The repository '@[unknown repo 'bazel_gazelle' requested from @rules_python~0.15.0]' could not be resolved: No repository visible as '@bazel_gazelle' from repository '@rules_python~0.15.0'.
Loading: 0 packages loaded
    currently loading: @rules_python~0.15.0//

I'll look into this when I get the chance.