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.

Sign up for GitHub

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

Jump to bottom

Update mill to 0.12.4 #172

Merged
merged 1 commit into from
Dec 22, 2024
Merged

Update mill to 0.12.4 #172

merged 1 commit into from
Dec 22, 2024

Conversation

nightscape
Copy link
Contributor

@nightscape nightscape commented Dec 18, 2024 •
edited
Loading

@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
@nightscape nightscape mentioned this pull request Dec 18, 2024
Closed
ckipp01
ckipp01 approved these changes Dec 19, 2024
View reviewed changes
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.

build.sc
@@ -102,25 +102,31 @@ trait ItestCross extends Cross.Module[String] with MillIntegrationTestModule {

override def testInvocations: T[Seq[(PathRef, Seq[TestInvocation.Targets])]] =
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.

@ckipp01 ckipp01 mentioned this pull request Dec 19, 2024
Open
@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 👍

ckipp01
ckipp01 approved these changes Dec 22, 2024
View reviewed changes
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 join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ckipp01 ckipp01 ckipp01 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nightscape @ckipp01