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

Sort script sources to ensure deterministic generation of child module aliases in the build.mill/package.mill file #4113

Merged
merged 7 commits into from
Dec 12, 2024

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Dec 12, 2024

  • The Seq returned from MillBuildRootModule#scriptSources comes from the keys of a Map, meaning it is arbitrary non-deterministic orders
  • Causing the resultant child module aliases (generated in build.mill and package.mill files to reference sub-folder package.mill files) to occur in non-deterministic orders
  • Resulting in bytecode changes (e.g. varying the bit position of each reference the lazy val initialization bitmap) that causes lots of stuff to unnecessarily invalidate

Tested manually, seems to remove the misbehavior reported in #4112. Managed to reproduce it in integration/invalidation/codesig-subfolder with some tweaks, so I left the repro in place to guard against regression

This still leaves us open to unnecessary invalidations if someone adds a new subfolder/package.mill file in the middle of the list, but adding new package.mills is pretty uncommon so we can probably fix that in a follow up

@lihaoyi lihaoyi marked this pull request as ready for review December 12, 2024 01:58
@lihaoyi lihaoyi merged commit 7c9e136 into com-lihaoyi:main Dec 12, 2024
27 checks passed
@lefou lefou added this to the 0.12.4 milestone Dec 12, 2024
lihaoyi added a commit that referenced this pull request Dec 12, 2024
We tweak the codegen and codesig analysis to avoid unnecessarily
invalidating the code signatures when the set of sub-folder
`package.mill` files change. Follow up of
#4113

Covered by additional integration tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants