Skip to content

Bump Mill to 1.0.6 (was 0.12.16)#1686

Merged
alexarchambault merged 1 commit into
com-lihaoyi:mainfrom
Gedochao:update/mill-1.x
Oct 10, 2025
Merged

Bump Mill to 1.0.6 (was 0.12.16)#1686
alexarchambault merged 1 commit into
com-lihaoyi:mainfrom
Gedochao:update/mill-1.x

Conversation

@Gedochao

@Gedochao Gedochao commented Oct 9, 2025

Copy link
Copy Markdown
Contributor

No description provided.

@Gedochao Gedochao marked this pull request as ready for review October 9, 2025 13:21
@Gedochao

Gedochao commented Oct 9, 2025

Copy link
Copy Markdown
Contributor Author

Working around the Mill file system checker is tricky here, I'm thinking I'd rather try it later in a follow-up PR.
Otherwise, I think it should be ready to review.
@alexarchambault @lolgab care to take a look?

lolgab
lolgab reviewed Oct 9, 2025
View reviewed changes
Comment thread .github/workflows/publish-artifacts.yml Outdated
@alexarchambault

alexarchambault commented Oct 9, 2025

Copy link
Copy Markdown
Collaborator

Right now, IIUC, we'd have to pass --no-filesystem-checker to all Mill invocations locally, which is a bit cumbersome.

The file check stuff should "just" be a matter of wrapping some code sections around with BuildCtx.withFilesystemCheckerDisabled, isn't it?

@Gedochao

Gedochao commented Oct 9, 2025

Copy link
Copy Markdown
Contributor Author

The file check stuff should "just" be a matter of wrapping some code sections around with BuildCtx.withFilesystemCheckerDisabled, isn't it?

Normally, yes.
But in this case, we'd need to provide Task.Sources instances for all custom source directories, of which there are a lot here. The old implementation just dumps them all while overriding sources, but that doesn't seem to do the trick anymore. I handled simple cases of this elsewhere, but nothing quite like this.
I can give it another go later, but a quick attempt failed, so I didn't want to block this PR.

@Gedochao Gedochao force-pushed the update/mill-1.x branch 5 times, most recently from fc375f7 to 7881396 Compare October 10, 2025 10:03
@Gedochao

Gedochao commented Oct 10, 2025

Copy link
Copy Markdown
Contributor Author

It seems I managed to fix the filesystem checker... the flag is no longer necessary.
The fix isn't beautiful, but I've run out of ideas on how to do it better.
Feel free to try to refactor it later.

Gedochao
Gedochao commented Oct 10, 2025
View reviewed changes

@Gedochao Gedochao left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ugly workaround, but seems to compile.

Comment thread build.mill Outdated
Comment thread build.mill Outdated
@Gedochao Gedochao force-pushed the update/mill-1.x branch 2 times, most recently from 22c2ef1 to b6b29ea Compare October 10, 2025 10:52
@Gedochao

Gedochao commented Oct 10, 2025

Copy link
Copy Markdown
Contributor Author

Ok... I seem to have found a slightly less ugly workaround

@Gedochao Gedochao requested a review from lolgab October 10, 2025 11:49
@Gedochao

Gedochao commented Oct 10, 2025

Copy link
Copy Markdown
Contributor Author

@lolgab @alexarchambault ok... should be ready to re-review

@Gedochao

Gedochao commented Oct 10, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

I reverted the change to publishing, as that smh seems to break Scala 2.13.17 tests 😕 (seemingly unconnected, and yet I can reproduce the issue locally).
The CI was green before that.
Let's follow that up separately.

alexarchambault
alexarchambault approved these changes Oct 10, 2025
View reviewed changes

@alexarchambault alexarchambault left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Apart from the minor point about CoursierModule, LGTM

Comment thread build.mill Outdated
}

trait AmmInternalModule extends CrossSbtModule with Bloop.Module {
trait AmmInternalModule extends CrossSbtModule with CoursierModule {

@alexarchambault alexarchambault Oct 10, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we need that CoursierModule here?

@Gedochao Gedochao Oct 10, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hm. Perhaps we don't.

@Gedochao Gedochao Oct 10, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed it.

@Gedochao

Gedochao commented Oct 10, 2025

Copy link
Copy Markdown
Contributor Author

I'm starting to think this is flaky:

[2167] amm[2.13.17].test.testForked 1 tests failed: 
  ammonite.interp.script.AmmoniteBuildServerTests ammonite.interp.script.AmmoniteBuildServerTests.#utestAfterAll

fails inconsistently.

@alexarchambault

alexarchambault commented Oct 10, 2025

Copy link
Copy Markdown
Collaborator

I'm starting to think this is flaky:

[2167] amm[2.13.17].test.testForked 1 tests failed: 
  ammonite.interp.script.AmmoniteBuildServerTests ammonite.interp.script.AmmoniteBuildServerTests.#utestAfterAll

fails inconsistently.

Same here, not sure what the problem is

@Gedochao Gedochao mentioned this pull request Oct 10, 2025
Merged
@alexarchambault

alexarchambault commented Oct 10, 2025

Copy link
Copy Markdown
Collaborator

Merging, thanks @Gedochao!

@alexarchambault alexarchambault merged commit d15bb1e into com-lihaoyi:main Oct 10, 2025
25 of 26 checks passed
@Gedochao Gedochao deleted the update/mill-1.x branch October 10, 2025 19:45
lolgab added a commit that referenced this pull request Oct 11, 2025
@Gedochao @lolgab
Migrate publishing to Sonatype Central Portal (#1690)
178b215 
Following up to #1686 

Requires (is on top of):
- #1686

The actual change is just the last commit (3 lines)

---------

Co-authored-by: Lorenzo Gabriele <lorenzolespaul@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexarchambault alexarchambault alexarchambault approved these changes
@lolgab lolgab Awaiting requested review from lolgab

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Gedochao @alexarchambault @lolgab