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

Clean up unused, undeclared, and conflicting dependencies #201

Draft
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

jisantuc
Copy link
Contributor

@jisantuc jisantuc commented Feb 8, 2022

This PR:

  • updates scalatest to 3.2.11 for downstream compatibility
  • excludes scala-xml from scalatest dependencies to avoid a much more painful compatibility issue

Downstream, I got the scalatest upgrade to cooperate by setting libraryDependencySchemes for scala-xml to"always". However, in a scalatest discussion, they recommend just excluding the scala-xml 2.x dependency, since scalatest doesn't need any of the parts that are different. This PR takes the latter approach here, which I'll chase through to the downstream repos in an effort to fix what's going wrong post-upgrade with the cats-effect / http4s / github4s / etc. upgrade PR.

I believe that this approach is better than what I did in exercises-cats -- setting the library scheme to always tells SBT "don't worry, I know it's fine, just go ahead even though the compatibility freaks you out a bit," but the thing is, I didn't know it was fine, and there were enough downstream surprises that I think it might not have been fine. This PR is the first step to digging out of the hole I dug myself.

Bigger picture context

The exercises each extend AnyFlatSpec from scalatest. Scalatest 3.2.11 is the first scalatest version that depends on scala-xml >= 2.0. Part of the series of the dependency upgrades that I'm integrating into scala-exercises/scala-exercises is upgrading all of the scalatest versions to 3.2.11. The problem I'm running into with integrating all of the libraries is that discoverLibraries isn't discovering any. discoverLibraries works by searching the classpath for sub-classes of Exercise. I think, based on printing the classes that get discovered on main in scala-exercises, that those subclasses are the product of the generateExercises command from this repo. Locally, I can't generateExercises for reasons (scala.runtime.ScalaRunTime$.wrapRefArray([Ljava/lang/Object;)Lscala/collection/immutable/ArraySeq; 🤦‍♂️ ), but CI claims to do so successfully. My suspicion is that this scala-xml incompatibility, which I previously told sbt is totally fine, is in fact not fine, and that its not being fine is messing with codegen / other classpath shenanigans in a way that's causing the exercises not to be loaded at runtime, the upshot of which is that you don't see any libraries when you load up the app.

Testing

The point of this PR is to get scalatest upgraded and to dodge a major version incompatibility for scala-xml. To that end, you can do the following to verify that this accomplishes those goals:

  • add addDependencyTreePlugin to project/plugins.sbt
  • fire up an sbt shell
  • eliminate the exclude calls in build.sbt
  • try to update -- you won't be able to, because sbt >= 1.5.x throws errors for suspected binary-incompatible evictions
  • restore the exclude calls
  • run definitions / dependencyBrowseTree and compiler / dependencyBrowseTree
  • search scala-xml in each -- you should only see version 1.2.0 with no evictions

build.sbt Outdated Show resolved Hide resolved
build.sbt Outdated
"com.github.alexarchambault" %% "scalacheck-shapeless_1.15" % V.scalacheckShapeless
"com.github.alexarchambault" %% "scalacheck-shapeless_1.15" % V.scalacheckShapeless,
"org.scalatest" %% "scalatest-core" % V.scalatest exclude ("org.scala-lang.modules", "scala-xml_2.12") exclude ("org.scala-lang.modules", "scala-xml_2.13"),
"org.scalatest" %% "scalatest" % V.scalatest exclude ("org.scala-lang.modules", "scala-xml_2.12") exclude ("org.scala-lang.modules", "scala-xml_2.13")
)
Copy link

Choose a reason for hiding this comment

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

It's fine like this, but just as a tip, if I need to nuke a certain dependency from my classpath I often use map to add the exclude rule to every single entry in libraryDependencies. That reduces the risk of the unwanted transitive dependency slipping through the cracks, e.g. when you add more dependencies to your project later.

libraryDependencies ++= Seq(
   ...
   ...
).map(_ exclude ("org.foo", "bar"))

build.sbt Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
@cb372
Copy link

cb372 commented Feb 10, 2022

Excluding the transitive scala-xml dependency from scalatest is a reasonable way of solving this, but IMO using dependencyOverrides is a bit simpler and signals your intent a bit more clearly:

dependencyOverrides += "org.scala-lang.modules" %% "scala-xml" % "1.2.0",

@jisantuc
Copy link
Contributor Author

Failure now is due to file systems locally and in CI disagreeing about how much information getParentFile returns 🤦

I'm going to add mac os to the build matrix for a sec to see if that's the source of the discrepancy or if it's something else

@jisantuc jisantuc changed the title Upgrade scalatest to 3.2.11 for downstream compatibility Clean up unused, undeclared, and conflicting dependencies Feb 10, 2022
Copy link
Contributor

@juanpedromoreno juanpedromoreno left a comment

Choose a reason for hiding this comment

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

Great job @jisantuc !

@jisantuc jisantuc marked this pull request as draft February 14, 2022 23:30
@jisantuc
Copy link
Contributor Author

I converted this to a draft to prevent merge -- I've had some problems with the integrating things, and I haven't resolved them here. For about two hours Thursday last week it was possible to run publishLocal in exercises-stdlib but when I turned my computer on Friday morning that wasn't the case anymore. I've outlined the problems here.

@jisantuc jisantuc marked this pull request as ready for review February 21, 2022 20:03
@jisantuc jisantuc marked this pull request as draft February 21, 2022 20:03
@jisantuc
Copy link
Contributor Author

fat fingered it, it was not in fact ready for review again

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

3 participants