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

Automatically handle MiMa for all versions #532

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mdedetrich
Copy link
Contributor

@mdedetrich mdedetrich commented Aug 3, 2023

The primary goal of this PR is to automatically handle checking for MiMa while in the process removing the hardcoding of latestPatchOf10 (which is currently incorrect since it should be 1 due to Pekko 1.0.1 being released but the fact that we forgot to increment this strengthens the argument for doing this automatically).

The PR is designed to handle the following cases

  • Minor being bumped (i.e. when we just went from 1.0.x to 1.1.x) but no version yet released. For this case we compare to the entire previous minor release (i.e. 1.0.0 and 1.0.1 with the example of 1.1.x)
  • Just after the first release of a minor (i.e. 1.0.0) which implies a snapshot in which case we just compare with 1.0.0
  • The standard case, i.e. with 1.0.1 released then we compare to all of the previous versions from the major series up until this one.

The code is cleanly commented and its also easy to test, i.e. in build.set set

ThisBuild / version := "<VERSION>"

to whatever version you want to test with and then just run sbt mimaPreviousArtifacts. This will give you a list of all of computed artifacts to check against.

@pjfanning
Copy link
Contributor

pjfanning commented Aug 3, 2023

I think it would be easier to explicitly name the version that we are diffing against. For me, both 1.0.x and main should diff against 1.0.0 release.

@mdedetrich
Copy link
Contributor Author

I think it be easier to explicitly name the version that we are diffing against.

The problem is that this is brittle, if we forget to bump the version (as we just did) then we can introduce bincompat problems. The logic of what we want to do is perfectly easy to automate

For me, both 1.0.x and main should diff against 1.0.0 release.

Yes and this is retained

@mdedetrich mdedetrich force-pushed the automatically-handle-MiMa branch 2 times, most recently from 5345b96 to 7194968 Compare August 3, 2023 15:39
@mdedetrich mdedetrich marked this pull request as draft August 3, 2023 15:42
@mdedetrich mdedetrich force-pushed the automatically-handle-MiMa branch 3 times, most recently from 26257ed to b26b3ed Compare August 3, 2023 16:11
@mdedetrich mdedetrich marked this pull request as ready for review August 3, 2023 16:31
@mdedetrich
Copy link
Contributor Author

Okay PR is ready to review.

@pjfanning
Copy link
Contributor

build failed because we have no 1.1.0-M0 jars to compare against.

@mdedetrich
Copy link
Contributor Author

build failed because we have no 1.1.0-M0 jars to compare against.

So this solution would be much more cleanly solved by just publishing a milestone artifact. Tagging something as milestone but not publishing its artifact is all things considered not idiomatic. We can discuss whether it makes sense to bundled a M0 release together with the last patch version of the previous minor since technically speaking the source will be the same, its just that the versioning is changing since we are creating a branch.

Otherwise the only other way to solve this problem is to hack some kind of workaround and/or not use milestone tags and deal with the fact that until the first release is made for a new minor the snapshots will not line up.

@He-Pin
Copy link
Member

He-Pin commented Sep 3, 2023

Let me close and reopen to trigger a rebuild.

@He-Pin He-Pin closed this Sep 3, 2023
@He-Pin He-Pin reopened this Sep 3, 2023
checkMimaFilterDirectories := checkFilterDirectories(baseDirectory.value, version.value))

def checkFilterDirectories(moduleRoot: File, version: String): Unit = {
val strippedPatchRegex = """(\d+)(.*)""".r
Copy link
Member

Choose a reason for hiding this comment

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

This regex doesn't make sense if we're matching against the whole version—if you have a version of 1.1.0 then you're extracting .1.0 as the patch version which doesn't parse as an int. Did you want something like \d+[.]\d+[.](\d+).*?

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.

None yet

4 participants