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

build.mill files compiled by Scala 3 #3369

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

bishabosha
Copy link
Contributor

@bishabosha bishabosha commented Aug 13, 2024

This is work in progress to fix #3152

Numerous fixes were also needed to com-lihaoyi/mainargs, com-lihaoyi/sourcecode, and com-lihaoyi/mill-moduledefs

With the current state, only 1 example/integration tests is still failing:

  • integration.feature[plugin-classpath].local

known TODOs:

  • Discover macro
  • Applicative macro
  • Caller macro
  • Cross.Factory macro
  • EnclosingClass macro
  • Task macros
  • Cacher macro
  • Moduledefs compiler plugin (override inferrence)
  • All core Mill modules compile with Scala 3.5.0
  • Fix Zinc reporter patch linenumbers of build scripts
  • Check that bytecode analyzers work with Scala 3
  • cleanup library dependency conflicts
  • Support new Scala 3 syntax in build.sc files
  • Fix BSP reporter linenumbers for build scripts (Zinc reporter forwards to bsp)
  • Cleanup compiler warnings for outdated syntax

known incompatibilities:

  • can't use ExplicitResultTypes scalafix rule - need to upgrade mill-scalafix
  • Cross.scala uses the new quoted type syntax which scalafmt crashes on, (and version is frozen) so skip the file upgraded Scalafmt so not skipped anymore
  • skipping Mima currently due to 1000s of errors (perhaps we should generate filters?)
  • filtered one flaky test from example.thirdparty[mockito]
  • filtered out integration.feature[plugin-classpath] due to third party plugin dep

@lihaoyi
Copy link
Member

lihaoyi commented Aug 13, 2024

Thanks @bishabosha ! Lets get your mainargs changes landed, that way we can cut a release and you can test your WIP in CI

@lefou
Copy link
Member

lefou commented Aug 13, 2024

+1 for switching over to using directives!

@bishabosha
Copy link
Contributor Author

@bishabosha bishabosha force-pushed the scala3-build-sc branch 3 times, most recently from 9d1a986 to dc71022 Compare August 16, 2024 09:51
@bishabosha
Copy link
Contributor Author

bishabosha commented Aug 16, 2024

[Edit: after switching to depending on mainargs 0.7.2] in https://github.com/com-lihaoyi/mill/actions/runs/10418081276/job/28853542122?pr=3369 you can see integration tests like example/tasks/6-workers/local passing

@bishabosha
Copy link
Contributor Author

Rebased and included the new mill-moduledefs 0.11.0-M1

@bishabosha bishabosha force-pushed the scala3-build-sc branch 4 times, most recently from a06daca to 37ba541 Compare September 11, 2024 11:19
@bishabosha bishabosha force-pushed the scala3-build-sc branch 5 times, most recently from 917a024 to 78beea1 Compare September 17, 2024 18:59
@bishabosha
Copy link
Contributor Author

bishabosha commented Sep 18, 2024

@lihaoyi just recording here that the mockito third party test can be flaky (specifically StubbingLookupListenerCallbackTest.add_listeners_concurrently_sanity_check):

https://github.com/mockito/mockito/blob/1e337ed703ce2b78ce9ea40ff8b8c645f8219cf2/src/test/java/org/mockitousage/debugging/StubbingLookupListenerCallbackTest.java#L199-L202

as seen in the CI run https://github.com/com-lihaoyi/mill/actions/runs/10909315276/job/30277476457?pr=3369#step:9:4941

@bishabosha bishabosha force-pushed the scala3-build-sc branch 9 times, most recently from 022c789 to 368fec9 Compare September 20, 2024 23:18
enhancement to zinc reporter and avoid mangling ansi escapes
- update com-lihaoyi/sourcecode to use new macro implementation
- fix callgraph 4-actors test: account for private[this] inferrence
- fix - callgraph 8-linked-list-scala test: account for new override semantics
- fix 5-parser codesig test to account for new expansion
- use custom SAM type in lambda tests for scala (specialisation dropped in scala 3)
- tweak CodeSig ignoreCall logic to account for Scala 3 lambdas
- update some dependencies so invalidation tests actually run
- fix indentation in codesig tests
- In Scala 3 an implicitly inserted else branch will not be implicitly converted
  to the type of the then branch, so use explicit else branch with NodeSeq.Empty
@bishabosha bishabosha force-pushed the scala3-build-sc branch 2 times, most recently from ca487d1 to 7620712 Compare October 18, 2024 20:37
@bishabosha
Copy link
Contributor Author

bishabosha commented Oct 18, 2024

@lihaoyi it seems like Scala 3 really doesn't like the task invocation syntax () (without .apply()) in a lot of cases (see ci/mill-bootstrap.patch), if this will be a wont-fix on the compiler side perhaps it would be worth an alternative like .eval ?

- create and load scala compiler worker from MillMain
- splitScript (mill scripts are a compilation unit where top stats are a template stat sequence)
- cached initCtx
- tested concurrent parsing
- bin-pack comma separated import clauses into one
- extract ImportTree from import code snippets
- extract ObjectData from object code snippets
- patch end marker of wrapper object
- splice millDiscover into wrapper object after last statement
- add passing scala-3-syntax test
- account for code splice offset in ZincWorkerImpl reporter
- Fix FullRunLogsTests, mill-build has an extra task
- fix line offset in compile failure tests
@lihaoyi
Copy link
Member

lihaoyi commented Oct 19, 2024

@bishabosha am I correct to say that single () style syntax is fine, and the problem is mostly with double ()() style syntax? I think that's something I want to find a way around anyway, so even if this turns out to be a wontfix upstream we probably want to find another syntax locally, but the single () for the common cases should probably remain

@bishabosha
Copy link
Contributor Author

bishabosha commented Oct 19, 2024

Yeah I have seen no problems when it's a single apply

@bishabosha
Copy link
Contributor Author

bishabosha commented Oct 19, 2024

@lihaoyi I'm also not sure why there is a difference in the integration.invalidation[codesig-subfolder] test - (any change to a single build file compiles all build files) - where is the logic that decides this based on codesig? or perhaps its a difference in how Scala 3 emits Zinc API

The test-docs task also a bit of a mystery I didn't look into too deeply - but yeah whole pages are just not being generated, so the proofer correctly identifies the links do not exist

@lihaoyi
Copy link
Member

lihaoyi commented Oct 19, 2024

@bishabosha for integration.invalidation[codesig-subfolder], let's just skip that for the moment. I expect that to change when sbt/zinc#1462 lands, which is likely related to the differences in compilation issues. The "who is recompiled" is 100% on zinc, and will change when that linked PR lands

Not sure about the test-docs stuff. Guessing it's some trivial bug in the doc-generation pipeline as part of the scala3 changes. I'd say it's not super critical for you to look into it, since it's something anyone should be able to debug later if necessary

@lihaoyi
Copy link
Member

lihaoyi commented Oct 19, 2024

let's just skip that for the moment.

Or just comment out the "number of files being recompiled" asserts, the "task hashes are properly invalidated" asserts are still useful (you didn't mention them failing so I assume they still work?)

@bishabosha
Copy link
Contributor Author

@lihaoyi is there any updates to the plan for merging this post 0.12.0?

@lihaoyi
Copy link
Member

lihaoyi commented Oct 24, 2024

@bishabosha let's hold the PR open for now, going forward I can take over responsibilities for keeping the PR up to date so you don't need to maintain it forever.

I don't know exactly how much development we'll see in 0.12.x yet, so not sure when the best time to break bincompat in main will be, but I expect there'll be some short-term follow up from the 0.12.0 release.

  1. Keeping the PR open and occasionally merging main into it would be isomorphic to merging it, cutting a 0.12.x release branch, sending PRs to the release branch and then doing batch-merges into main.

  2. Either of those options would probably be less tedious than having to backport every PR if 0.12.x is still under active development

  3. But keeping the PR open and updated is probably going to be less confusing than cutting/targeting 0.12.x and batch-merging into main

So for now I think it makes the most sense to keep the PR open and periodically merge main into it until we're ready to break bincompat. I can do the work of keeping it up to date with main going forward

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.

Migrate build.sc to Scala 3
5 participants