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

Using invalid patch path with bzlmod + lockfile results in crash and bad lockfile #21845

Closed
keith opened this issue Mar 28, 2024 · 0 comments
Closed
Assignees
Labels
area-Bzlmod Bzlmod-specific PRs, issues, and feature requests P1 I'll work on this now. (Assignee required) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: bug

Comments

@keith
Copy link
Member

keith commented Mar 28, 2024

Description of the bug:

It's invalid to specify a patch in your MODULE.bazel that is from a repo that's also being loaded in the MODULE.bazel. If you do this you end up corrupting your lockfile and it results in bazel crashes until you delete the lockfile.

It looks like the issue is the invalid patch path gets encoded in the lockfile before it's rejected:

      "repoSpec": {
        "bzlFile": "@@bazel_tools//tools/build_defs/repo:http.bzl",
        "ruleClassName": "http_archive",
        "attributes": {
          "urls": [
            "https://github.com/keith/buildifier-prebuilt/archive/refs/tags/6.4.0.tar.gz"
          ],
          "integrity": "sha256-itqdiOUev1of3/N9de1B1R9eZ3zb6vsKIt2lR0fW4H4=",
          "strip_prefix": "buildifier-prebuilt-6.4.0",
          "remote_patches": {
            "https://bcr.bazel.build/modules/buildifier_prebuilt/6.4.0/patches/module_dot_bazel_version.patch": "sha256-FpUp/q4zJ2H12lwezrYaPUGLY2rr1XoWpiDRaE19udw="
          },
          "remote_patch_strip": 0,
          "patches": [
            "@@[unknown repo 'buildozer' requested from @@]//:foo.patch"
          ],
          "patch_cmds": [],

Then bazel crashes with:

Caused by: com.google.devtools.build.lib.cmdline.LabelSyntaxException: invalid repository name '[unknown repo 'buildozer' requested from @@]': repo names may contain only A-Z, a-z, 0-9, '-', '_', '.' and '~' and must not start with '~'

Which category does this issue belong to?

No response

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

  1. checkout this branch Add support for bzlmod google/copybara#287
  2. add something like this to the MODULE.bazel
single_version_override(
    module_name = "buildifier_prebuilt",
    patches = [
        "@buildozer//:foo.patch",
    ],
)

It has to be patching a module that is used during the build

  1. Build like USE_BAZEL_VERSION=last_green bazelisk build //java/com/google/copybara:copybara_deploy.jar
  2. Note the build failure for the invalid patch
  3. build again

Which operating system are you running Bazel on?

macOS

What is the output of bazel info release?

e03de75

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse HEAD ?

No response

Is this a regression? If yes, please try to identify the Bazel commit where the bug was introduced.

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

@sgowroji sgowroji added team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. area-Bzlmod Bzlmod-specific PRs, issues, and feature requests labels Mar 28, 2024
@meteorcloudy meteorcloudy added P1 I'll work on this now. (Assignee required) and removed untriaged labels Apr 16, 2024
Wyverald added a commit that referenced this issue Apr 18, 2024
Today, a repo rule's attributes might contain invalid labels because of unresolved apparent repo names. This could be due to either module extensions generating bad repo definitions, or users providing e.g. bad labels to `patches` attribute of overrides. This causes us to write a label like `@@[unknown repo 'foo' requested from @@]//:bar` into the lockfile, which will cause Bazel to crash on a subsequent attempt to parse the lockfile.

This PR fixes it so that we throw an error at the time of writing the lockfile, instead of writing a corrupted lockfile that users can't recover from unless they manually delete it. In the unlikely event that we do have a corrupted lockfile, we also no longer crash, but give a helpful error message instead.

Fixes #21845.
Wyverald added a commit that referenced this issue Apr 18, 2024
Today, a repo rule's attributes might contain invalid labels because of unresolved apparent repo names. This could be due to either module extensions generating bad repo definitions, or users providing e.g. bad labels to `patches` attribute of overrides. This causes us to write a label like `@@[unknown repo 'foo' requested from @@]//:bar` into the lockfile, which will cause Bazel to crash on a subsequent attempt to parse the lockfile.

This PR fixes it so that we throw an error at the time of writing the lockfile, instead of writing a corrupted lockfile that users can't recover from unless they manually delete it. In the unlikely event that we do have a corrupted lockfile, we also no longer crash, but give a helpful error message instead.

Fixes #21845.
Wyverald added a commit that referenced this issue Apr 18, 2024
Today, a repo rule's attributes might contain invalid labels because of unresolved apparent repo names. This could be due to either module extensions generating bad repo definitions, or users providing e.g. bad labels to `patches` attribute of overrides. This causes us to write a label like `@@[unknown repo 'foo' requested from @@]//:bar` into the lockfile, which will cause Bazel to crash on a subsequent attempt to parse the lockfile.

This PR fixes it so that we throw an error at the time of writing the lockfile, instead of writing a corrupted lockfile that users can't recover from unless they manually delete it. In the unlikely event that we do have a corrupted lockfile, we also no longer crash, but give a helpful error message instead.

Fixes #21845.
Wyverald added a commit that referenced this issue Apr 18, 2024
Today, a repo rule's attributes might contain invalid labels because of unresolved apparent repo names. This could be due to either module extensions generating bad repo definitions, or users providing e.g. bad labels to `patches` attribute of overrides. This causes us to write a label like `@@[unknown repo 'foo' requested from @@]//:bar` into the lockfile, which will cause Bazel to crash on a subsequent attempt to parse the lockfile.

This PR fixes it so that we throw an error at the time of writing the lockfile, instead of writing a corrupted lockfile that users can't recover from unless they manually delete it. In the unlikely event that we do have a corrupted lockfile, we also no longer crash, but give a helpful error message instead.

Fixes #21845.
Wyverald added a commit that referenced this issue Apr 18, 2024
Today, a repo rule's attributes might contain invalid labels because of unresolved apparent repo names. This could be due to either module extensions generating bad repo definitions, or users providing e.g. bad labels to `patches` attribute of overrides. This causes us to write a label like `@@[unknown repo 'foo' requested from @@]//:bar` into the lockfile, which will cause Bazel to crash on a subsequent attempt to parse the lockfile.

This PR fixes it so that we throw an error at the time of writing the lockfile, instead of writing a corrupted lockfile that users can't recover from unless they manually delete it. In the unlikely event that we do have a corrupted lockfile, we also no longer crash, but give a helpful error message instead.

Fixes #21845.
@fmeum fmeum assigned fmeum and unassigned Wyverald May 21, 2024
copybara-service bot pushed a commit that referenced this issue May 22, 2024
The following validation checks were not enforced due to backwards compatibility concerns, but ended up crashing Bazel when invalid labels made it into the lockfile, which is enabled by default. We might as well enable them now:

* Fail if a label passed to `use_extension` is not valid
* Fail if a label passed to the `patches` attribute of an override is not valid or points to a repo other than the main repo

Work towards #21845

Closes #22487.

PiperOrigin-RevId: 636255834
Change-Id: I55dda374cd1716e514c4d78642479b136fd3ad43
iancha1992 pushed a commit to iancha1992/bazel that referenced this issue May 22, 2024
The following validation checks were not enforced due to backwards compatibility concerns, but ended up crashing Bazel when invalid labels made it into the lockfile, which is enabled by default. We might as well enable them now:

* Fail if a label passed to `use_extension` is not valid
* Fail if a label passed to the `patches` attribute of an override is not valid or points to a repo other than the main repo

Work towards bazelbuild#21845

Closes bazelbuild#22487.

PiperOrigin-RevId: 636255834
Change-Id: I55dda374cd1716e514c4d78642479b136fd3ad43
github-merge-queue bot pushed a commit that referenced this issue May 22, 2024
The following validation checks were not enforced due to backwards
compatibility concerns, but ended up crashing Bazel when invalid labels
made it into the lockfile, which is enabled by default. We might as well
enable them now:

* Fail if a label passed to `use_extension` is not valid
* Fail if a label passed to the `patches` attribute of an override is
not valid or points to a repo other than the main repo

Work towards #21845

Closes #22487.

PiperOrigin-RevId: 636255834
Change-Id: I55dda374cd1716e514c4d78642479b136fd3ad43

Commit
cdace8f

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
bazel-io pushed a commit to bazel-io/bazel that referenced this issue May 24, 2024
Catches a common gotcha of referencing a repo created by an extension elsewhere in an extension without a `use_repo` and provides actionable advice to user. This prevents lockfile corruption caused by non-visible labels.

The same validation is applied to labels in tag attributes.

Fixes bazelbuild#21845

Closes bazelbuild#22495.

PiperOrigin-RevId: 636939357
Change-Id: Ib779207502f7767e4e8d3abc55ba7470f75821b9
github-merge-queue bot pushed a commit that referenced this issue May 24, 2024
…xtensions (#22537)

Catches a common gotcha of referencing a repo created by an extension
elsewhere in an extension without a `use_repo` and provides actionable
advice to user. This prevents lockfile corruption caused by non-visible
labels.

The same validation is applied to labels in tag attributes.

Fixes #21845

Closes #22495.

PiperOrigin-RevId: 636939357
Change-Id: Ib779207502f7767e4e8d3abc55ba7470f75821b9

Commit
aa436c3

Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Bzlmod Bzlmod-specific PRs, issues, and feature requests P1 I'll work on this now. (Assignee required) team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. type: bug
Projects
None yet
7 participants