Re-introduce Mill special handling of ivydeps, overhaul test classpath management to allow testing #2476
Conversation
This file contains 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
lihaoyi
changed the title
|
This should also be reviewable I think |
This was referenced May 17, 2023
lihaoyi
added a commit
that referenced
this pull request
May 21, 2023
Should fix #2533, which was broken by #2476 Just requires shuffling `def testDeps` and related definitions around to make them available outside `MillScalaModule`. They moved to `MillCoursierModule`, which in order to satisfy their dependencies had to be converted to `MillJavaModule`. The overall logic of the build is unchanged Tighted up the test artifact resolution logic for `.local` example/integration tests. Previously, they would ignore the `artifactSuffix`, which is why tests exercising `runBackground` like `example.web[1-todo-webapp].local` passed despite the bug. With the more strict test artifact resolution, this bug gets surfaced properly on `main`, and is fixed by this PR
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #2474
These were overlooked in #2377 and deleted, and were not covered by any tests. But it's straightforward to put them back.
Major Changes
MillIvy.scalaand call it inMillBuildRootModule.scalaTesting
I added an example test
example/misc/6-contrib-importto both exercise the code as well as serve as an example we can include in our docsIn order to allow usage of contrib module in
.localintegration and example tests, I moved the handling of Mill test classpath overrides fromUtil.millProjectModuletoCoursierSupport#resolveDependencies. This lets us be more override contrib modules dependency resolution, even though they don't have a neat single location for us to call ourmillProjectModulehelper.I refactored
millProjectModuleto not need akey, so we just compute the key based on the dependency name, keeping them consistent and removing an unnecessary degree of freedomThe local-testing-classpath-overrides were moved from using JVM system properties to instead use classpath resources: we look for overrides in
mill/local-test-overrides/*. This should remove any security worries:JAVA_OPTSenvironment variable, and could replace the code of a Mill module to any local filesystem pathWhile it probably was not strictly necessary to clean up the test classpath overrides logic as part of this PR, the status quo in master was a pile of hacks, and I didn't feel like adding another hack to get
.localtestability of contrib libraries working. With these changes, contrib libraries work the same as the existing Mill test classpath overrides, and things are cleaned up so much that the net lines of code for this PR is negativeThere's still more cleanup to do in
build.sc, but those can come in follow up PRs