Let module extensions track calls to Label()#20742
Closed
Conversation
... 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.
github-actions
Bot
added
team-Configurability
fmeum
approved these changes
Jan 4, 2024
| && envVariables.equals(lockedExtension.getEnvVariables())) { | ||
| LockedExtensionDiffDetector diffDetector = new LockedExtensionDiffDetector(/* recordDiffMessages= */ | ||
| lockfileMode.equals(LockfileMode.ERROR)); | ||
| diffDetector.detectDiffs(extensionId, env, extension, lockedExtension, |
Collaborator
There was a problem hiding this comment.
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.
Member
Author
There was a problem hiding this comment.
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.
Collaborator
There was a problem hiding this comment.
I still preferred it as a separate method, whether in the inner class or directly in the main class is up to you.
katre
suggested changes
Jan 4, 2024
| } | ||
|
|
||
| /** | ||
| * Converts Guava tables into a JSON array of 3-tuples (one per cell). Each 3-tuple is a JSON |
Collaborator
There was a problem hiding this comment.
I don't understand this comment: can you provide a small example JSON to clarify?
| throws IOException { | ||
| jsonWriter.beginArray(); | ||
| for (Table.Cell<Object, Object, Object> cell : t.cellSet()) { | ||
| jsonWriter.beginArray(); |
Collaborator
There was a problem hiding this comment.
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.
Member
Author
There was a problem hiding this comment.
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, |
Wyverald
added
awaiting-PR-merge
katre
approved these changes
Jan 4, 2024
github-actions
Bot
removed
the
awaiting-PR-merge
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 <[email protected]>
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
... 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@foowere to suddenly map to something else.I also did a minor refactoring in
SingleExtensionEvalFunctionaround 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.