Skip to content

Conversation

@fmeum
Copy link
Collaborator

@fmeum fmeum commented Aug 25, 2023

Sorts the extensions, which would otherwise be added in the non-deterministic order in which module extension resolution events are fired, by their ID.

Since Gson doesn't support custom comparators on a TreeMap, ModuleExtensionId now implements Comparable directly.

@github-actions github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. labels Aug 25, 2023
@fmeum fmeum force-pushed the deterministic-lockfile branch from b6ab7f7 to 017112c Compare August 25, 2023 19:09
Sorts the extensions, which would otherwise be added in the
non-deterministic order in which module extension resolution events are
fired, by their ID.
@fmeum fmeum force-pushed the deterministic-lockfile branch from 017112c to b04a445 Compare August 26, 2023 07:02
@fmeum fmeum requested review from SalmaSamy and removed request for meteorcloudy August 26, 2023 07:03
Copy link
Member

@meteorcloudy meteorcloudy left a comment

Choose a reason for hiding this comment

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

Nit: is there any simple way to test this?

@fmeum
Copy link
Collaborator Author

fmeum commented Aug 28, 2023

Nit: is there any simple way to test this?

While not a dedicated test for this purpose, #19343 generates a lockfile twice and compares the outcomes. We could have an additional test with more extensions, but it may still not fail reliably in the face of actual non-determinism.

@meteorcloudy
Copy link
Member

OK, I'm fine with the test in #19343

@meteorcloudy meteorcloudy 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 Aug 28, 2023
@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 Aug 29, 2023
@fmeum fmeum deleted the deterministic-lockfile branch August 29, 2023 09:14
@fmeum
Copy link
Collaborator Author

fmeum commented Aug 29, 2023

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Aug 29, 2023
@fmeum
Copy link
Collaborator Author

fmeum commented Aug 29, 2023

@SalmaSamy Just saw that Git authorship wasn't preserved for the commit. Not a big deal, but maybe the way this was merged internally needs to be adjusted for future commits?

@meteorcloudy
Copy link
Member

Yes, we should not push the update mater branch by merge button for normal PRs.

@iancha1992
Copy link
Member

@bazel-io fork 6.4.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Aug 29, 2023
bazel-io pushed a commit to bazel-io/bazel that referenced this pull request Aug 29, 2023
Sorts the extensions, which would otherwise be added in the non-deterministic order in which module extension resolution events are fired, by their ID.

Since Gson doesn't support custom comparators on a `TreeMap`, `ModuleExtensionId` now implements `Comparable` directly.

Closes bazelbuild#19339.

PiperOrigin-RevId: 560959804
Change-Id: Ic5579c032e6cd32fe4bd2114378812c0110109c2
iancha1992 pushed a commit to iancha1992/bazel that referenced this pull request Aug 30, 2023
Sorts the extensions, which would otherwise be added in the non-deterministic order in which module extension resolution events are fired, by their ID.

Since Gson doesn't support custom comparators on a `TreeMap`, `ModuleExtensionId` now implements `Comparable` directly.

Closes bazelbuild#19339.

PiperOrigin-RevId: 560959804
Change-Id: Ic5579c032e6cd32fe4bd2114378812c0110109c2
iancha1992 added a commit that referenced this pull request Aug 31, 2023
Sorts the extensions, which would otherwise be added in the
non-deterministic order in which module extension resolution events are
fired, by their ID.

Since Gson doesn't support custom comparators on a `TreeMap`,
`ModuleExtensionId` now implements `Comparable` directly.

Closes #19339.

Commit
ede4d49

PiperOrigin-RevId: 560959804
Change-Id: Ic5579c032e6cd32fe4bd2114378812c0110109c2

Co-authored-by: Fabian Meumertzheim <[email protected]>
@iancha1992
Copy link
Member

The changes in this PR have been included in Bazel 6.4.0 RC1. Please test out the release candidate and report any issues as soon as possible. If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=last_rc.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants