-
Notifications
You must be signed in to change notification settings - Fork 2
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
base: main
Are you sure you want to change the base?
Conversation
somehow i lost a bunch of logs in the GHA run
now that i know developers isn't getting set i just want logs for the github action please
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") | ||
) |
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.
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"))
Excluding the transitive scala-xml dependency from scalatest is a reasonable way of solving this, but IMO using
|
Failure now is due to file systems locally and in CI disagreeing about how much information 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 |
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.
Great job @jisantuc !
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 |
fat fingered it, it was not in fact ready for review again |
This PR:
Downstream, I got the scalatest upgrade to cooperate by setting
libraryDependencySchemes
forscala-xml
to"always"
. However, in a scalatest discussion, they recommend just excluding thescala-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 toalways
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 intoscala-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 thatdiscoverLibraries
isn't discovering any.discoverLibraries
works by searching the classpath for sub-classes ofExercise
. I think, based on printing the classes that get discovered onmain
inscala-exercises
, that those subclasses are the product of thegenerateExercises
command from this repo. Locally, I can'tgenerateExercises
for reasons (scala.runtime.ScalaRunTime$.wrapRefArray([Ljava/lang/Object;)Lscala/collection/immutable/ArraySeq;
🤦♂️ ), but CI claims to do so successfully. My suspicion is that thisscala-xml
incompatibility, which I previously toldsbt
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:addDependencyTreePlugin
toproject/plugins.sbt
exclude
calls inbuild.sbt
exclude
callsdefinitions / dependencyBrowseTree
andcompiler / dependencyBrowseTree
scala-xml
in each -- you should only see version 1.2.0 with no evictions