-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Enable MiMa in the new build #23910
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
Enable MiMa in the new build #23910
Conversation
* - `3.(M-1).0` if `P = 0` | ||
*/ | ||
val mimaPreviousDottyVersion = "3.7.0" | ||
val mimaPreviousDottyVersion = "3.7.3" // for 3.8.0, we compare against 3.7.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine since we only compare backward compatibility, not forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this change needed? Is it because of merging Scala 2.13 stdlib into this repo?
We should be really careful about this one. Let's at least store the forward compatibility issues in the gist for review. All of these should be documented here currently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could call the fat jar 3.7.0
if that helps. I don't think it really matters if it's 3.7.3
or 3.7.0
for 3.8.0
.
Test / develocityBuildCacheClient := None, | ||
extraDevelocityCacheInputFiles := Seq.empty, | ||
extraDevelocityCacheInputFiles / outputFileStamper := FileStamper.Hash, | ||
resolvers += ("Artifactory" at "https://repo.scala-lang.org/artifactory/fat-jar/"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be able to use MiMa, I created a fat jar that contains both Scala 3.7.3 and Scala 2.13.16 manually and pushed it to this resolver. Once we publish 3.8.0, we can tear down this system and compare against the artifacts from Maven Central.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add TODO notes to related changes that would need to be adjusted (resolvers, mimaPreviousArtifacts, etc)
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.runtime.Tuple3Zipped.coll2$extension"), | ||
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.runtime.Tuple3Zipped.coll3$extension"), | ||
|
||
// singleton case classes modules inherit AbstractFunction1?? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs investigation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have dedicated issue to track these? If no let's create one
96dd6a2
to
98867de
Compare
MiMa for Scala.js stdlib has been dropped in this PR ands not replaced yet. It should be fine and we cannot add it yet since we still need to patch some files over (aka what Scala.js call |
ProblemFilters.exclude[FinalClassProblem]("scala.annotation.experimental"), | ||
|
||
// ================================== STDLIB MIGRATION ================================== | ||
// ============================ module classes are not final ============================ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This category of filters will be dropped once we move to 3.8.1
.
3da880b
to
53713ae
Compare
…ary-bootstrapped`
53713ae
to
e274d0c
Compare
mima-scala-library-nonbootstrapped: | ||
runs-on: ubuntu-latest | ||
needs: scala-library-nonbootstrapped | ||
steps: | ||
- name: Git Checkout | ||
uses: actions/checkout@v5 | ||
|
||
- name: Set up JDK 17 | ||
uses: actions/setup-java@v5 | ||
with: | ||
distribution: 'temurin' | ||
java-version: 17 | ||
cache: 'sbt' | ||
|
||
- uses: sbt/setup-sbt@v1 | ||
- name: Report MiMa issues in `scala-library-nonbootstrapped` | ||
run: ./project/scripts/sbt scala-library-nonbootstrapped/mimaReportBinaryIssues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we either don't use build cache of it's misconfigured becouse in the last run scala-library is built at least twice: once in scala-library-nonbootstrapped
, again in mima-scala-library-nonbootstrapped
and once in every project that depends on it.
Even though splitting it might be easier to manage with the current build setup it's wasting resources.
Ideally we'd like to build everything once, but it's hard to achieve. If so, let's at least minimise the overhead by making the mima checks the next step in scala-library-nonbootstrapped
and other jobs.
The MiMa check itself takes only a few seconds, building artifact takes minutes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we either don't use build cache
I disabled it earlier today. It is misconfigured and very unpredictable. The intention is to use the cache, but this is independent of this PR and the cache will be enabled back soon. Just need the time to investigate why it doesn't sometime invalidated some builds.
* - `3.(M-1).0` if `P = 0` | ||
*/ | ||
val mimaPreviousDottyVersion = "3.7.0" | ||
val mimaPreviousDottyVersion = "3.7.3" // for 3.8.0, we compare against 3.7.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this change needed? Is it because of merging Scala 2.13 stdlib into this repo?
We should be really careful about this one. Let's at least store the forward compatibility issues in the gist for review. All of these should be documented here currently
Test / develocityBuildCacheClient := None, | ||
extraDevelocityCacheInputFiles := Seq.empty, | ||
extraDevelocityCacheInputFiles / outputFileStamper := FileStamper.Hash, | ||
resolvers += ("Artifactory" at "https://repo.scala-lang.org/artifactory/fat-jar/"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add TODO notes to related changes that would need to be adjusted (resolvers, mimaPreviousArtifacts, etc)
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.runtime.Tuple3Zipped.coll2$extension"), | ||
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.runtime.Tuple3Zipped.coll3$extension"), | ||
|
||
// singleton case classes modules inherit AbstractFunction1?? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have dedicated issue to track these? If no let's create one
// TO INVESTIGATE: This constructor changed, but it is private... why complaining? | ||
ProblemFilters.exclude[IncompatibleMethTypeProblem]("scala.collection.immutable.LazyList.this"), | ||
// This one should be fine, public class inside private object | ||
ProblemFilters.exclude[IncompatibleResultTypeProblem]("scala.collection.immutable.LazyList#LazyBuilder#DeferredState.eval"), | ||
|
||
// MIX IN FORWARDERS ISSUE (SHOULD BE FIXED WHEN WE REMERGE THE PR) | ||
ProblemFilters.exclude[NewMixinForwarderProblem]("scala.collection.StrictOptimizedSeqOps.prepended"), | ||
ProblemFilters.exclude[NewMixinForwarderProblem]("scala.collection.StrictOptimizedSeqOps.appended"), | ||
ProblemFilters.exclude[NewMixinForwarderProblem]("scala.collection.StrictOptimizedSeqOps.appendedAll"), | ||
ProblemFilters.exclude[NewMixinForwarderProblem]("scala.collection.StrictOptimizedSeqOps.prependedAll"), | ||
ProblemFilters.exclude[NewMixinForwarderProblem]("scala.collection.StrictOptimizedSeqOps.padTo"), | ||
ProblemFilters.exclude[NewMixinForwarderProblem]("scala.collection.immutable.StrictOptimizedSeqOps.updated"), | ||
ProblemFilters.exclude[NewMixinForwarderProblem]("scala.collection.immutable.StrictOptimizedSeqOps.patch"), | ||
|
||
// NO IDEA FOR NOW :) | ||
ProblemFilters.exclude[ReversedMissingMethodProblem]("scala.collection.mutable.ArrayDequeOps.scala$collection$mutable$ArrayDequeOps$$super$sliding"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for these: Let's make an issue so we don't forget about these.
Even a single tracker issue with list of problem kinds might be good for start
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have my own todo list with this and many others things that still need to be done. I don't want to open one issue per thing as it's too long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get it, but also a list like this one scala-native/scala-native#3165 might be helpful to communicate progress with rest of the team and might help @Gedochao to plan or adjust the current dev cycle.
// These are inner objects/private classes and it is fine to not have the outer reference captured. | ||
// The code that is emitted is correct within the compilation unit and does not escape it | ||
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.Enumeration#ValueOrdering.this"), | ||
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.IterableOnceOps#Maximized.this"), | ||
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.LinearSeqIterator#LazyCell.this"), | ||
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.collection.mutable.PriorityQueue#ResizableArrayAccess.this"), | ||
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.concurrent.BatchingExecutor#AbstractBatch.this"), | ||
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.concurrent.Channel#LinkedList.this"), | ||
ProblemFilters.exclude[DirectMissingMethodProblem]("scala.io.Source#RelaxedPosition.this"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: check those.
[skip ci]