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

Migrate to sbt-typelevel #2857

Merged
merged 18 commits into from
Apr 2, 2022

Conversation

armanbilge
Copy link
Member

Taking the conservative approach and not mixing up the build too much with sbt-typelevel gospel 😉

Opening for CI. I anticipate this PR will fail while we get MiMa up-to-speed on Scala 3 and possible Scala.js.

build.sbt Show resolved Hide resolved
Comment on lines 15 to 22
tags: [v*]

env:
PGP_PASSPHRASE: ${{ secrets.PGP_PASSPHRASE }}
SONATYPE_PASSWORD: ${{ secrets.SONATYPE_PASSWORD }}
SONATYPE_CREDENTIAL_HOST: ${{ secrets.SONATYPE_CREDENTIAL_HOST }}
SONATYPE_USERNAME: ${{ secrets.SONATYPE_USERNAME }}
PGP_SECRET: ${{ secrets.PGP_SECRET }}
Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I guess these are baked in a little too hard :) anyway should be harmless.

Copy link
Member

Choose a reason for hiding this comment

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

Bleh. :-) I'd definitely like to see these cleaned up, but it's kind of okay for now.

Comment on lines -159 to +158
ThisBuild / githubWorkflowBuildMatrixExclusions ++= {
ThisBuild / githubWorkflowBuildMatrixExclusions := {
Copy link
Member Author

Choose a reason for hiding this comment

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

sbt-typelevel already does some of these exclusions for us. Rather than tease apart what's been upstreamed we'll just keep using our own.

Copy link
Member

Choose a reason for hiding this comment

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

We shoudl try to inherit more in the future, but this is a fine start.

project/Common.scala Show resolved Hide resolved
@armanbilge
Copy link
Member Author

I just added like a 100+ mima filters. At a glance they all seem reasonable but would be good for someone to look with a more careful eye.

@armanbilge armanbilge marked this pull request as ready for review March 3, 2022 20:08
build.sbt Outdated
Comment on lines 29 to 30
ThisBuild / tlBaseVersion := "3.3"
ThisBuild / tlUntaggedAreSnapshots := false
Copy link
Member Author

Choose a reason for hiding this comment

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

I almost forgot 😆

Copy link
Member

@djspiewak djspiewak left a comment

Choose a reason for hiding this comment

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

Btw I do agree with the reformattings, but why are they happening as part of this PR?


val OldGuardJava = JavaSpec.temurin("8")
val LTSJava = JavaSpec.temurin("11")
val LatestJava = JavaSpec.temurin("17")
val ScalaJSJava = OldGuardJava
val GraalVM = JavaSpec.graalvm("21.3.0", "11")
val GraalVM = JavaSpec.graalvm("11")
Copy link
Member

Choose a reason for hiding this comment

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

Why is the Graal version not specified. This feels a little weird in general since it suggests that the GraalVM version would be either unstable or unconfigurable.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can revert this. Most people probably want the latest Graal and not a specific version (which is easy to forget to update, as we've forgotten to here :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Btw @vasilmkd what are your thoughts on this topic, maybe we need to revise the JDK index to support this?

Copy link
Member

Choose a reason for hiding this comment

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

We were already on latest, I was doing it manually. Graalvm also follows the JDK release cycle, so as soon as the latest drops, the previous one is obsolete and unsupported. I don't want to sound dismissive, but I'm not interested in changing the index to support old versions.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see the latest in the index is 22.0.0.2 and CE is on 21.3.0, so we've fallen behind again (at least on this branch).

Ok @djspiewak your call, how do you want to configure this for CE?

version would be either unstable

It depends on what you are targeting. If you are targeting JDK whatever then shouldn't it be as stable as any of the other JVMs?

OTOH, if you are targeting specific GraalVM APIs then obviously it makes a difference. Actually I have a project like this.

FTR I didn't remove the ability to configure a specific GraalVM (it still shells out to DeLaGuardo/setup-graalvm like you set it up) although it is now a little more "discouraged" because I expect 99% of projects don't actually want this.

Finally, if you truly want stability, you should really pin the JDK index to a particular hash anyway :)

build.sbt Show resolved Hide resolved
project/Common.scala Show resolved Hide resolved
Comment on lines 15 to 22
tags: [v*]

env:
PGP_PASSPHRASE: ${{ secrets.PGP_PASSPHRASE }}
SONATYPE_PASSWORD: ${{ secrets.SONATYPE_PASSWORD }}
SONATYPE_CREDENTIAL_HOST: ${{ secrets.SONATYPE_CREDENTIAL_HOST }}
SONATYPE_USERNAME: ${{ secrets.SONATYPE_USERNAME }}
PGP_SECRET: ${{ secrets.PGP_SECRET }}
Copy link
Member

Choose a reason for hiding this comment

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

Bleh. :-) I'd definitely like to see these cleaned up, but it's kind of okay for now.

@armanbilge
Copy link
Member Author

Formatting changes are probably because sbt-typelevel brings you latest sbt-scalafmt which this branch missed.

which includes a fix for:

@vasilmkd
Copy link
Member

In light of scala/scala3#14624 and scala/scala3#14662, I think we can go ahead and apply the MiMa filters for the Scala 3 issue.

@armanbilge
Copy link
Member Author

After this PR merges, I'll do a follow-up which adds JDK17/Scala3 to CI. That will force us to add the mima filters.

@djspiewak
Copy link
Member

show version
3.3.9-18-b0b4a99-20220402T054540Z-SNAPSHOT

How do I make it stop? :-( Worktrees do seem to work though.

@armanbilge
Copy link
Member Author

Cool! So not a blocker ... right? 😇

@djspiewak
Copy link
Member

LOL. Well, I really want to make that version stuff at least configurable, because the transitive impact it has on the ecosystem is… unpleasant. Not a hard blocker, but definitely a something. I'll see if I can fix it myself in sbt-tl

@djspiewak
Copy link
Member

djspiewak commented Apr 2, 2022

Oh, it's more sbt-git issues:

sbt:root> show gitUncommittedChanges
[info] benchmarks / gitUncommittedChanges
[info] 	true
[info] testsJVM / gitUncommittedChanges
[info] 	true

There are no uncommitted changes.

@djspiewak
Copy link
Member

Alright, managed to get it working, sort of. 3.3.9-20-44bf4e1 has been published to sonatype so we can make sure things are in good shape. I published it using Java 11, fwiw, so we can test the release flag while we're at it.

Basically, we needed one last fix for sbt-git which I completely forgot about. Namely, they're determining dirty directories incorrectly (by using git status and then grepping for text). This has issues between different versions of git, and so the workaround is to use git status -s and check to see if it returns any results.

@djspiewak
Copy link
Member

Release looks good. Branch appears to be generally functional. I'm in favor of merging this, though it would be nice to see the sbt-git workaround upstreamed at some point.

@vasilmkd
Copy link
Member

vasilmkd commented Apr 2, 2022

Let's go! 🚀

@armanbilge
Copy link
Member Author

🎉 I wasn't sure if we'd make it 😂 thank you! Obviously still plenty to upstream 😬

@armanbilge armanbilge merged commit 794c85d into typelevel:series/3.3.x Apr 2, 2022
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.

3 participants