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

Let module extensions track calls to Label() #20742

Closed
wants to merge 4 commits into from
Closed

Conversation

Wyverald
Copy link
Member

@Wyverald Wyverald commented Jan 4, 2024

... that use repo mapping. This is a rather obscure case of the lockfile being stale; if the Label() constructor is called in an extension impl function, and that call uses repo mapping of any form (i.e. the argument looks like @foo//bar), then we need to be ready to rerun the extension if @foo were to suddenly map to something else.

I also did a minor refactoring in SingleExtensionEvalFunction around the logic to decide whether the locked extension is up-to-date. Right now we perform a "diff" between the locked extension and what we expect to be up-to-date, and if a "diff" is found and --lockfile_mode=error, we basically perform a diff again. We also don't short circuit; that is, if the transitive bzl digest has changed, there's no point in seeing whether any files have changed, but we do right now.

A follow-up will be sent to fix the analogous bug for repo rules.

Fixes #20721.

... that use repo mapping. This is a rather obscure case of the lockfile being stale; if the `Label()` constructor is called in an extension impl function, and that call uses repo mapping of any form (i.e. the argument looks like `@foo//bar`), then we need to be ready to rerun the extension if `@foo` were to suddenly map to something else.

I also did a minor refactoring in `SingleExtensionEvalFunction` around the logic to decide whether the locked extension is up-to-date. Right now we perform a "diff" between the locked extension and what we expect to be up-to-date, and if a "diff" is found *and* `--lockfile_mode=error`, we basically perform a diff again. We also don't short circuit; that is, if the transitive bzl digest has changed, there's no point in seeing whether any files have changed, but we do right now.

A follow-up will be sent to fix the analogous bug for repo rules.

Fixes #20721.
@Wyverald Wyverald requested a review from fmeum January 4, 2024 01:05
@Wyverald Wyverald requested review from a team and meteorcloudy as code owners January 4, 2024 01:05
@Wyverald Wyverald requested review from katre and removed request for a team January 4, 2024 01:05
@github-actions github-actions bot added team-Configurability platforms, toolchains, cquery, select(), config transitions team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. awaiting-review PR is awaiting review from an assigned reviewer labels Jan 4, 2024
&& envVariables.equals(lockedExtension.getEnvVariables())) {
LockedExtensionDiffDetector diffDetector = new LockedExtensionDiffDetector(/* recordDiffMessages= */
lockfileMode.equals(LockfileMode.ERROR));
diffDetector.detectDiffs(extensionId, env, extension, lockedExtension,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having to call the constructor followed by this method, could we make his method static and have it create an immutable LockedExtensionDiff instead? I could see this making it harder to misuse.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the point of the inner class is more to make control flow less verbose. to emphasize that, I moved the detectDiffs part back into this method, and renamed the inner class to just DiffRecorder. hopefully that makes the intent clearer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still preferred it as a separate method, whether in the inner class or directly in the main class is up to you.

@@ -283,6 +301,68 @@ public Optional<T> read(JsonReader jsonReader) throws IOException {
}
}

/**
* Converts Guava tables into a JSON array of 3-tuples (one per cell). Each 3-tuple is a JSON
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this comment: can you provide a small example JSON to clarify?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a small snippet.

throws IOException {
jsonWriter.beginArray();
for (Table.Cell<Object, Object, Object> cell : t.cellSet()) {
jsonWriter.beginArray();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is my confusion with the Javadoc: there aren't any tuples here (are tuples even a json concept?), just an array of arrays.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tuples are not JSON concepts. I wanted to emphasize that each "sub-array" has exactly 3 elements, so conceptually a 3-tuple.

&& envVariables.equals(lockedExtension.getEnvVariables())) {
LockedExtensionDiffDetector diffDetector = new LockedExtensionDiffDetector(/* recordDiffMessages= */
lockfileMode.equals(LockfileMode.ERROR));
diffDetector.detectDiffs(extensionId, env, extension, lockedExtension,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@Wyverald Wyverald added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Jan 4, 2024
@copybara-service copybara-service bot closed this in 5cba057 Jan 4, 2024
@github-actions github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Jan 4, 2024
iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Jan 4, 2024
... that use repo mapping. This is a rather obscure case of the lockfile being stale; if the `Label()` constructor is called in an extension impl function, and that call uses repo mapping of any form (i.e. the argument looks like `@foo//bar`), then we need to be ready to rerun the extension if `@foo` were to suddenly map to something else.

I also did a minor refactoring in `SingleExtensionEvalFunction` around the logic to decide whether the locked extension is up-to-date. Right now we perform a "diff" between the locked extension and what we expect to be up-to-date, and if a "diff" is found *and* `--lockfile_mode=error`, we basically perform a diff again. We also don't short circuit; that is, if the transitive bzl digest has changed, there's no point in seeing whether any files have changed, but we do right now.

A follow-up will be sent to fix the analogous bug for repo rules.

Fixes bazelbuild#20721.

Closes bazelbuild#20742.

PiperOrigin-RevId: 595818144
Change-Id: Id660b7a659a7f2e4dde19c16784c2ab18a9ceb69
iancha1992 added a commit that referenced this pull request Jan 5, 2024
... that use repo mapping. This is a rather obscure case of the lockfile
being stale; if the `Label()` constructor is called in an extension impl
function, and that call uses repo mapping of any form (i.e. the argument
looks like `@foo//bar`), then we need to be ready to rerun the extension
if `@foo` were to suddenly map to something else.

I also did a minor refactoring in `SingleExtensionEvalFunction` around
the logic to decide whether the locked extension is up-to-date. Right
now we perform a "diff" between the locked extension and what we expect
to be up-to-date, and if a "diff" is found *and*
`--lockfile_mode=error`, we basically perform a diff again. We also
don't short circuit; that is, if the transitive bzl digest has changed,
there's no point in seeing whether any files have changed, but we do
right now.

A follow-up will be sent to fix the analogous bug for repo rules.

Fixes #20721.

Closes #20742.

PiperOrigin-RevId: 595818144
Change-Id: Id660b7a659a7f2e4dde19c16784c2ab18a9ceb69

Co-authored-by: Xdng Yng <wyverald@gmail.com>
@Wyverald Wyverald deleted the wyv-track-label branch January 10, 2024 00:33
Wyverald added a commit that referenced this pull request Jan 10, 2024
In the same vein as #20742, we record all repo mapping entries used during the load of a .bzl file too, including any of its `load()` statements and calls to `Label()` that contain an apparent repo name.

See #20721 (comment) for a more detailed explanation for this change, and the test cases in this commit for more potential triggers.

Fixes #20721
Wyverald added a commit that referenced this pull request Jan 10, 2024
In the same vein as #20742, we record all repo mapping entries used during the load of a .bzl file too, including any of its `load()` statements and calls to `Label()` that contain an apparent repo name.

See #20721 (comment) for a more detailed explanation for this change, and the test cases in this commit for more potential triggers.

Fixes #20721
copybara-service bot pushed a commit that referenced this pull request Jan 10, 2024
In the same vein as #20742, we record all repo mapping entries used during the load of a .bzl file too, including any of its `load()` statements and calls to `Label()` that contain an apparent repo name.

See #20721 (comment) for a more detailed explanation for this change, and the test cases in this commit for more potential triggers.

Fixes #20721

Closes #20830.

PiperOrigin-RevId: 597351525
Change-Id: I8f6ed297b81d55f7476a93bdc6668e1e1dcbe536
Wyverald added a commit that referenced this pull request Jan 10, 2024
In the same vein as #20742, we record all repo mapping entries used during the load of a .bzl file too, including any of its `load()` statements and calls to `Label()` that contain an apparent repo name.

See #20721 (comment) for a more detailed explanation for this change, and the test cases in this commit for more potential triggers.

Fixes #20721

Closes #20830.

PiperOrigin-RevId: 597351525
Change-Id: I8f6ed297b81d55f7476a93bdc6668e1e1dcbe536
Wyverald added a commit that referenced this pull request Jan 11, 2024
In the same vein as #20742, we
record all repo mapping entries used during the load of a .bzl file too,
including any of its `load()` statements and calls to `Label()` that
contain an apparent repo name.

See
#20721 (comment)
for a more detailed explanation for this change, and the test cases in
this commit for more potential triggers.

Fixes #20721

Closes #20830.

PiperOrigin-RevId: 597351525
Change-Id: I8f6ed297b81d55f7476a93bdc6668e1e1dcbe536
Wyverald added a commit that referenced this pull request Jan 25, 2024
... that use repo mapping. This is a rather obscure case of the lockfile being stale; if the `Label()` constructor is called in an extension impl function, and that call uses repo mapping of any form (i.e. the argument looks like `@foo//bar`), then we need to be ready to rerun the extension if `@foo` were to suddenly map to something else.

I also did a minor refactoring in `SingleExtensionEvalFunction` around the logic to decide whether the locked extension is up-to-date. Right now we perform a "diff" between the locked extension and what we expect to be up-to-date, and if a "diff" is found *and* `--lockfile_mode=error`, we basically perform a diff again. We also don't short circuit; that is, if the transitive bzl digest has changed, there's no point in seeing whether any files have changed, but we do right now.

A follow-up will be sent to fix the analogous bug for repo rules.

Fixes #20721.

Closes #20742.

PiperOrigin-RevId: 595818144
Change-Id: Id660b7a659a7f2e4dde19c16784c2ab18a9ceb69
Wyverald added a commit that referenced this pull request Jan 25, 2024
In the same vein as #20742, we record all repo mapping entries used during the load of a .bzl file too, including any of its `load()` statements and calls to `Label()` that contain an apparent repo name.

See #20721 (comment) for a more detailed explanation for this change, and the test cases in this commit for more potential triggers.

Fixes #20721

Closes #20830.

PiperOrigin-RevId: 597351525
Change-Id: I8f6ed297b81d55f7476a93bdc6668e1e1dcbe536
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Configurability platforms, toolchains, cquery, select(), config transitions team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bzlmod lockfile does not track repository mappings of extension .bzl files
3 participants