Skip to content
This repository was archived by the owner on May 4, 2025. It is now read-only.

Conversation

@nightscape
Copy link
Contributor

@nightscape nightscape commented Dec 18, 2024

@ckipp01 this one was a little bit of a PITA, so it would be nice to get it merged soonish 😉
It adds compatibility with Mill 0.12, but still retains compatibility with previous versions.
Only the reconcileRanges test doesn't work, not sure what changed there...

@nightscape nightscape force-pushed the mill-0.12 branch 4 times, most recently from 595aebf to 6e7e43c Compare December 18, 2024 17:41
@nightscape nightscape marked this pull request as ready for review December 18, 2024 17:42
Copy link
Owner

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

Thanks!

build.sc Outdated
"reconciledRange" -> "verify",
"cyclical" -> "checkManifest"
).filter {
// The reconciledRange test doesn't work under 0.12 atm
Copy link
Owner

Choose a reason for hiding this comment

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

So this was due to what I believe is a bug in coursier. I created an issue for it in coursier/coursier#2682 but never got a response about what is actually happening. Maybe this is fixed with the new changes in coursier, but I'd need to dig deeper to better understand if that's the case. Right now you can go ahead and just ignore this and I'll try to look deeper at what's happening.

Copy link
Owner

Choose a reason for hiding this comment

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

I dug a bit deeper and created com-lihaoyi/mill#4165.

T {
val env = if(millTestVersion() >= "0.12")
Map(
"COURSIER_REPOSITORIES" -> s"central sonatype:releases ivy:file://${T.dest.toString.replaceFirst("testInvocations", "test")}/ivyRepo/local/[organisation]/[module]/[revision]/[type]s/[artifact].[ext]"
Copy link
Owner

Choose a reason for hiding this comment

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

So is this needed now for 0.12 versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this was the only way I found to make Mill / Coursier pick up the local dev repo.
In Mill 0.12+ there is a different way to integration-test Mill plugins, with which this hack/workaround would probably go away. But that doesn't work for 0.10 and 0.11, so I guess it's best to live with the workaround until usage of Mill < 0.12 dies out and one can completely switch to the new approach.

@nightscape
Copy link
Contributor Author

@ckipp01 I hope/think I fixed the remaining issues:

  • Updated from Java 8 to 11 (Mill 0.12.0-RC1 dropped support for Java 8)
  • Fixed the version typo in the reconciledRange test
  • Changed the way the tests discover the directory containing manifests.json

Could you approve GitHub Actions to run the tests again?

@nightscape
Copy link
Contributor Author

Ok, tests are looking good. I'll fix the style issues then it should be good to merge.

@nightscape nightscape force-pushed the mill-0.12 branch 4 times, most recently from f5fc7d3 to 6440c33 Compare December 20, 2024 22:53
@nightscape
Copy link
Contributor Author

I fixed all logged style errors and force-pushed the results.
Locally I still get an error like this with no further explanation:

domain.fix Something unexpected happened running Scalafix

I hope this doesn't happen in the GitHub Actions.
@ckipp01 could you trigger another run?

@nightscape
Copy link
Contributor Author

@ckipp01 the same error happened as on my local machine.
I updated the mill-scalafix plugin and ran the tests locally using act.
I got up to the submit-dependency-graph step, which I think should fail locally, as I don't have the required GitHub credentials.
So hopefully this is the final try 😉

@nightscape
Copy link
Contributor Author

@ckipp01 all green, ready to merge from my side 👍

Copy link
Owner

@ckipp01 ckipp01 left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your work on this @nightscape. LGTM

@ckipp01 ckipp01 merged commit 3aac770 into ckipp01:main Dec 22, 2024
3 checks passed
@nightscape nightscape deleted the mill-0.12 branch December 22, 2024 13:32
@nightscape
Copy link
Contributor Author

@ckipp01 looks like the release process has 0.12 problems...
We could

  • build the project using Mill 0.11, but still publish artifacts for 0.10, 0.11 and 0.12
  • update mill-ci-release to support Mill 0.12
  • change the build to publish without mill-ci-release

@ckipp01
Copy link
Owner

ckipp01 commented Dec 22, 2024

Yes, I see that. I think maybe for now we should just use 0.11.x to publish, then try to update mill-ci-release to work with 0.12.x.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants