-
Notifications
You must be signed in to change notification settings - Fork 949
Override scala organization and version transitively at the Ivy level #2634
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
Conversation
Hi @milessabin, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Typesafe Contributors License Agreement: |
@@ -7,7 +7,7 @@ import Sxr.sxr | |||
// but can be shared across the multi projects. | |||
def buildLevelSettings: Seq[Setting[_]] = inThisBuild(Seq( | |||
organization := "org.scala-sbt", | |||
version := "0.13.12-SNAPSHOT", | |||
version := "0.13.12-devel", |
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.
you probably didn't mean to commit this
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.
This is a PoC for comment ... Yolo etc. ...
I have a general comment, that moves this PR beyond its current scope, but I'll mention it in any case. In this PR, functionality is added to override the groupid for some "known" modules - the scala-lang ones. But it could be that other builds may want to do the same on their "known" modules. So would it be useful to have a general |
@inthenow I think that would be extremely useful, but I think it deserves a separate PR ... most likely it would use the same underlying mechanisms. |
@milessabin Excellent - I think two PR's is best as well, with this one setting "the scene". Just thought I should mention it sooner rather than later |
@inthenow 👍 ... it was at the back of my mind as well :-) |
Nice and simple, nice. |
if (check.filterImplicit) | ||
excludeScalaJars(module, check.configurations) | ||
if (check.overrideScalaVersion) | ||
overrideScalaVersion(module, check.scalaFullVersion) | ||
if (true || check.overrideScalaVersion) |
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.
If an attribute of
UpdateOptions
, how would that be made available toIvyScala
?
Maybe we can use this overrideScalaVersion
, which is where you're hooking your code anyway.
I'm ok with flipping the default value of this flag to true
.
70f441f
to
ccd9f29
Compare
Hi @milessabin, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Typesafe Contributors License Agreement: |
@@ -1193,6 +1190,9 @@ object Classpaths { | |||
projectDescriptors <<= depMap, | |||
updateConfiguration := new UpdateConfiguration(retrieveConfiguration.value, false, ivyLoggingLevel.value), | |||
updateOptions := (updateOptions in Global).value, | |||
ivyScala <<= ivyScala or (scalaHome, updateOptions in Global, scalaVersion in update, scalaBinaryVersion in update, scalaOrganization, sbtPlugin) { (sh, uo, fv, bv, so, plugin) => | |||
Some(new IvyScala(fv, bv, Nil, filterImplicit = false, checkExplicit = true, overrideScalaVersion = plugin | uo.overrideScalaVersion, scalaOrganization = so)) | |||
}, |
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.
I'd welcome review on this change ... is this the right way to pass the new UpdateOptions
field through to IvyScala
?
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.
I'm actually ok with just putting true
where it says plugin | uo.overrideScalaVersion
, and roll back the change you made in UpdateOptions
.
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.
In that case there wouldn't be a mechanism to escape back to the current no-override behaviour. I have no problem with that, and it means that we could get rid of the conditional in checkModule
... if you're sure that's what you want I'm happy to do that.
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.
The incantation to get the old school behavior back would be:
ivyScala := { ivyScala.value map {_.copy(overrideScalaVersion = sbtPlugin.value)} }
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.
Ah ... gotcha. Right, I'll do that.
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.
Ah ... gotcha. Right, I'll do that.
Hi @milessabin, Thank you for your contribution! We really value the time you've taken to put this together. Before we proceed with reviewing this pull request, please sign the Typesafe Contributors License Agreement: |
f886b8a
to
0d52125
Compare
Tests added, comments addressed, squashed and rebased. |
LGTM pending Travis Could you add a release note to https://github.com/sbt/sbt/tree/0.13/notes/0.13.12/ under "Fixes with compatibility implications" header (see https://github.com/sbt/sbt/blob/0.13/notes/0.13.11.markdown for examples) please? |
Will do. Do you want that squashed into the same commit? |
Either is fine with me. |
Could you also forward port this to https://github.com/sbt/librarymanagement please? |
I seem to have broken dependency-management/exclude-scala ... I don't really know what that test is testing ... help? |
lazy val root = Project("root", file(".")) settings( | ||
libraryDependencies <++= baseDirectory(dependencies), | ||
scalaVersion := "2.9.2", | ||
ivyScala := { ivyScala.value map {_.copy(overrideScalaVersion = sbtPlugin.value)} }, |
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.
@eed3si9n this fixes the test. Are you happy with the way that's being done?
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.
Yea. That looks fine.
533a0bb
to
f4929b0
Compare
f4929b0
to
e98b236
Compare
did we miss the release notes? |
no there're there |
Ref sbt#2634 updateSbtClassifiers uses an artificially created dependency graph set in classifiersModule. The problem is that ivyScala instance is reused from the outer scope that has the user project's scalaVersion as demonstrated as follows: scala> val is = (ivyScala in updateSbtClassifiers).eval is: Option[sbt.IvyScala] = Some(IvyScala(2.9.3,2.9.3,List(),true,false,true,org.scala-lang)) This change fixes sbt#2686 by redefining ivyScala with scalaVersion and scalaBinaryVersion scoped to updateSbtClassifiers task. The existing scripted test was modified to reproduce the bug.
Fixes sbt#2786. Ref sbt#2634. sbt 0.13.12 added Ivy mediator that enforces scalaOrganization and scalaVersion for Scala toolchain artifacts. This turns out to be a bit too aggressive because Ivy configurations can be used as an independent dependency graph that does not rely on the scalaVersion used by Compile configuration. By enforcing scalaVersion in those graph causes runtime failure. This change checks if the configuration extends Default, Compile, Provided, or Optional before enforcing scalaVersion.
Fixes sbt#2786. Ref sbt#2634. sbt 0.13.12 added Ivy mediator that enforces scalaOrganization and scalaVersion for Scala toolchain artifacts. This turns out to be a bit too aggressive because Ivy configurations can be used as an independent dependency graph that does not rely on the scalaVersion used by Compile configuration. By enforcing scalaVersion in those graph causes runtime failure. This change checks if the configuration extends Default, Compile, Provided, or Optional before enforcing scalaVersion.
Ref sbt#2634 updateSbtClassifiers uses an artificially created dependency graph set in classifiersModule. The problem is that ivyScala instance is reused from the outer scope that has the user project's scalaVersion as demonstrated as follows: scala> val is = (ivyScala in updateSbtClassifiers).eval is: Option[sbt.IvyScala] = Some(IvyScala(2.9.3,2.9.3,List(),true,false,true,org.scala-lang)) This change fixes sbt#2686 by redefining ivyScala with scalaVersion and scalaBinaryVersion scoped to updateSbtClassifiers task. The existing scripted test was modified to reproduce the bug.
There are a couple of settings / configs that affect this, summary below. The change in this PR seems to be the most narrow. `scalaModuleInfo.value.overrideScalaVersion` in sbt - affects how sbt to sets coursier's `forceScalaVersion` (see below) - used by librarymanagement.ivy. If true, add a OverrideScalaMediator See sbt#2634. Probably not relevant when using coursier. `autoScalaLibrary` setting in sbt - automatically add `scala-library` (or `scala3-library`) as a project dependency - also used for `forceScalaVersion` (see below) `CoursierConfiguration.autoScalaLibrary` - if `true` then Coursier `ResolutionParams.forceScalaVersion` is set to to `true` - initialized by sbt to `autoScalaLibrary.value && !ScalaArtifacts.isScala3(sv) && !Classpaths.isScala213(sv) && // added in this commit scalaModuleInfo.forall(_.overrideScalaVersion)` coursier `ResolutionParams.forceScalaVersion` - if true, `scala-library` / `scala-reflect` / `scala-compiler` / `scalap` are forced to the scala version, not actually resolved - for Scala 3, the `scala3-library` and `scala3-compiler` versions are forced
There are a couple of settings / configs that affect this, summary below. The change in this PR seems to be the most narrow. `scalaModuleInfo.value.overrideScalaVersion` in sbt - affects how sbt to sets coursier's `forceScalaVersion` (see below) - used by librarymanagement.ivy. If true, add a OverrideScalaMediator See sbt#2634. Probably not relevant when using coursier. `autoScalaLibrary` setting in sbt - automatically add `scala-library` (or `scala3-library`) as a project dependency - also used for `forceScalaVersion` (see below) `CoursierConfiguration.autoScalaLibrary` - if `true` then Coursier `ResolutionParams.forceScalaVersion` is set to to `true` - initialized by sbt to `autoScalaLibrary.value && !ScalaArtifacts.isScala3(sv) && !Classpaths.isScala213(sv) && // added in this commit scalaModuleInfo.forall(_.overrideScalaVersion)` coursier `ResolutionParams.forceScalaVersion` - if true, `scala-library` / `scala-reflect` / `scala-compiler` / `scalap` are forced to the scala version, not actually resolved - for Scala 3, the `scala3-library` and `scala3-compiler` versions are forced
There are a couple of settings / configs that affect this, summary below. The change in this PR seems to be the most narrow. `scalaModuleInfo.value.overrideScalaVersion` in sbt - affects how sbt to sets coursier's `forceScalaVersion` (see below) - used by librarymanagement.ivy. If true, add a OverrideScalaMediator See sbt#2634. Probably not relevant when using coursier. `autoScalaLibrary` setting in sbt - automatically add `scala-library` (or `scala3-library`) as a project dependency - also used for `forceScalaVersion` (see below) `CoursierConfiguration.autoScalaLibrary` - if `true` then Coursier `ResolutionParams.forceScalaVersion` is set to to `true` - initialized by sbt to `autoScalaLibrary.value && !ScalaArtifacts.isScala3(sv) && !Classpaths.isScala213(sv) && // added in this commit scalaModuleInfo.forall(_.overrideScalaVersion)` coursier `ResolutionParams.forceScalaVersion` - if true, `scala-library` / `scala-reflect` / `scala-compiler` / `scalap` are forced to the scala version, not actually resolved - for Scala 3, the `scala3-library` and `scala3-compiler` versions are forced
There are a couple of settings / configs that affect this, summary below. The change in this PR seems to be the most narrow. `scalaModuleInfo.value.overrideScalaVersion` in sbt - affects how sbt to sets coursier's `forceScalaVersion` (see below) - used by librarymanagement.ivy. If true, add a OverrideScalaMediator See sbt#2634. Probably not relevant when using coursier. `autoScalaLibrary` setting in sbt - automatically add `scala-library` (or `scala3-library`) as a project dependency - also used for `forceScalaVersion` (see below) `CoursierConfiguration.autoScalaLibrary` - if `true` then Coursier `ResolutionParams.forceScalaVersion` is set to to `true` - initialized by sbt to `autoScalaLibrary.value && !ScalaArtifacts.isScala3(sv) && !Classpaths.isScala213(sv) && // added in this commit scalaModuleInfo.forall(_.overrideScalaVersion)` coursier `ResolutionParams.forceScalaVersion` - if true, `scala-library` / `scala-reflect` / `scala-compiler` / `scalap` are forced to the scala version, not actually resolved - for Scala 3, the `scala3-library` and `scala3-compiler` versions are forced
Fixes sbt#2786. Ref sbt#2634. sbt 0.13.12 added Ivy mediator that enforces scalaOrganization and scalaVersion for Scala toolchain artifacts. This turns out to be a bit too aggressive because Ivy configurations can be used as an independent dependency graph that does not rely on the scalaVersion used by Compile configuration. By enforcing scalaVersion in those graph causes runtime failure. This change checks if the configuration extends Default, Compile, Provided, or Optional before enforcing scalaVersion.
This is a first cut at a PR intended to elicit feedback.
The primary motivation for this change is to allow an alternative Scala organization to be used in builds which have dependencies which themselves have direct dependencies on Scala distribution artefacts (eg. scala-reflect, scala-compiler). As well as supporting alternative Scala organizations I believe that this change also addresses #2286 and some of the concerns raised in the discussion on that ticket.
The change is to use an Ivy
DependencyDescriptorMediator
to rewrite all direct and transitive dependencies on scala-library, scala-compiler, scala-reflect, scala-actors, and scalap to have the same organization (either specified by thescalaOrganization
setting or defaulting toorg.scala-lang
) and version (as specified byscalaVersion
).Issues to be resolved before merging,
UpdateOptions
, how would that be made available toIvyScala
?