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

Update sbt-twirl to 1.5.2 in series/0.23 #57

Merged
merged 5 commits into from
Jan 8, 2023

Conversation

http4s-steward[bot]
Copy link
Contributor

Updates com.typesafe.sbt:sbt-twirl from 1.5.1 to 1.5.2.

I'll automatically update this PR to resolve conflicts as long as you don't change it yourself.

If you'd like to skip this version, you can just close this PR. If you have any feedback, just mention me in the comments below.

Configure Scala Steward for your repository with a .scala-steward.conf file.

Have a fantastic day writing Scala!

Adjust future updates

Add this to your .scala-steward.conf file to ignore future updates of this dependency:

updates.ignore = [ { groupId = "com.typesafe.sbt", artifactId = "sbt-twirl" } ]

Or, add this to slow down future updates of this dependency:

dependencyOverrides = [{
  pullRequests = { frequency = "@monthly" },
  dependency = { groupId = "com.typesafe.sbt", artifactId = "sbt-twirl" }
}]

labels: sbt-plugin-update, early-semver-patch, semver-spec-patch, commit-count:1

@rossabaker
Copy link
Member

This was a major bump of scala-xml, which breaks things. This should not be in a patch version of http4s-twirl, but could be the basis of an 0.24, as we've done for other breaking integrations.

@rossabaker rossabaker requested a review from a team January 4, 2023 18:34
@kailuowang
Copy link
Contributor

On vacation now, will take a look when I am back

@kailuowang kailuowang merged commit f325a5b into series/0.23 Jan 8, 2023
@kailuowang kailuowang deleted the update/series/0.23/sbt-twirl-1.5.2 branch January 8, 2023 12:16
@rossabaker
Copy link
Member

Do we want this on series/0.23, or use it to start an 0.24 to reflect the breaking change? This won't be a drop-in replacement for people with other scala-xml-1 dependencies.

@kailuowang
Copy link
Contributor

kailuowang commented Jan 9, 2023

use it to start an 0.24 to reflect the breaking change

We already have a 0.24 branch (with 1 0.24 milestone release), we can drop it there. Let me know.

@kailuowang
Copy link
Contributor

kailuowang commented Jan 9, 2023

I will revert this one if we place it in 0.24

@kailuowang
Copy link
Contributor

Sorry, @rossabaker I am still jet-lagging. We can't place it in 0.24 since it's on 1.6.0-M7. So I don't know what's the solution w.r.t 1.5.2 breaking, and don't have the brain to think clearly about it atm.

@rossabaker
Copy link
Member

Oh, I forgot there was an 0.24. Yeah, we got out in an awkward position because Twirl didn't consider it breaking. The prevailing wisdom in the Scala community is that this has been "compatible enough" and people are quietly killing scala-xml-1 in patch releases. But it's not binary compatible, and even when it doesn't cause linker errors, it causes loud SBT eviction errors.

Any of these are fine with me:

  • Move 0.24 back to twirl-1.5.2. I can't remember what was exciting about 1.6, but that could go into an 0.25 if there's demand.

  • Pin 0.23 to twirl-1.5.1 and keep 0.24 on the twirl-1.6 series.

  • Leave it as is and scream real loud in the release notes that it's a breaking change. I don't like it, because it imposes pinning on downstream users and breaks the semver contract, but it's how most of the rest of the scala-xml handled it.

@kailuowang
Copy link
Contributor

We introduced 0.24 with milestone release to add support for Scala 3.

Thus for option 1, we also need to make an equally loud release since users might be surprised that we strip away the Scala 3 support from this branch. and there will be slightly more work on our end.

I like Option 2 the least since that leaves no options for incoming security patches on twirl-1.5.x.

I know you dislike the 3rd option, but http4s-twirl is a bit different in that its users are less likely to have downstream users themselves. It's more likely that users use http4s-twirl to render HTML for their web apps rather than provide some functions as a library.

Give a final thumbs-up on this comment and I will implement option 3 - leave it as is and scream real loud in the release note.

@rossabaker
Copy link
Member

That works for me. I don't love it, but I don't think we're left with a better option based on what all has happened upstream.

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.

2 participants