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

Add support for rules defined/compiled within the same build #100

Merged
merged 6 commits into from
May 19, 2020

Conversation

bjaglin
Copy link
Collaborator

@bjaglin bjaglin commented May 3, 2020

In our fairly big sbt build, we have decided to collocate custom rule definitions with the sources they run against, using a separate sbt project of the same root build, in order to make the process of adding/updating them as seamless as possible (since they are not meant to be distributed or used anywhere else). It's working, but requires a quite hacky use of --tool-classpath which can only be passed once, with a lot of coursier-based code from here inlined.

This is an attempt at bringing the feature upstream (that I refer to in the code as "local rules"), although I assume it's not a very common use-case.

I chose to implement it using an ivy configuration (which might have some side-effects I didn't think of!). That led me to 2 interesting thoughts:

  1. That configuration can be used to reference external dependencies as well, as an alternative to scalafixDependencies (see 72cd47c). I assume this has a tiny extra cost for sbt builds not running coursier (<1.3 or with >=1.3 with coursier explicitly disabled), since these dependencies end up being fetched/stored twice on disk (once into the ivy cache, once into the coursier one) as far as I understand. But it makes it possible to bring some rules only for some modules (not necessarily ThisBuild like scalafixDependencies). I am not sure it's worth documenting/advertizing though...
  2. At some point, I thought explicit usage of coursier could be removed by relying on sbt-librarymanagement only (helping sbt-scalafix doesn't respect sbt.override.build.repos scalafix#1049 for example), but I realized we need to retrieve dynamic JARs for dependency:*, and I wouldn't know how to do that without coursier...

@bjaglin bjaglin changed the title Add support for rules defined/compiled whthin the same build Add support for rules defined/compiled within the same build May 3, 2020
@@ -13,12 +13,13 @@ val rules = project
.disablePlugins(ScalafixPlugin)
.settings(
scalaVersion := Versions.scala212,
crossPaths := false,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That does not seem to be enough, as I see org.scala-lang:scala-library:2.13.1 in service/Scalafix/externalDependencyClasspath, which would probably cause all sorts of errors if the rule was not dummy. Investigating.

Copy link
Collaborator Author

@bjaglin bjaglin May 4, 2020

Choose a reason for hiding this comment

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

Actually, I can't write a test to show this is a problem, so I am going to assume it's not. I can find 2 explanations for that (I am not sure about them though):

  1. scala-library is binary-compatible across minor releases sorry for the mixup, I checked again since I couldn't understand how it was possible, and it's only for patch release of course...
  2. the parent classloader already has a 2.12 library (brought-in by scalafix-cli), so the 2.13 one in the bottom will effectively be ignored

@bjaglin bjaglin marked this pull request as ready for review May 4, 2020 00:15
@@ -21,6 +21,9 @@ object ScalafixPlugin extends AutoPlugin {
object autoImport {
val Scalafix = Tags.Tag("scalafix")

val ScalafixConfig = config("scalafix")
Copy link
Collaborator Author

@bjaglin bjaglin May 4, 2020

Choose a reason for hiding this comment

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

This is the third type of "scalafix config" after the function returning sbt settings applied to Compile/Test & the sbt setting referencing the actual configuration file for scalafix... I initially considered naming it simply Scalafix to match its id, and deleting/renaming the current one 2 lines above since it's not used internally since 4dbaf1e, but that would be a breaking change.

@bjaglin
Copy link
Collaborator Author

bjaglin commented May 4, 2020

I have added a questionable commit (1efa0cf) that enables and demonstrates declaration of a custom, local rule within the same project as the target code - mostly to honor/extend the description of the ScalafixConfig configuration.

I say questionable, as we don't use this but have a multi-project setup as initially tested, and most importantly this has the strong consequence of coupling the scala version of the project with the one of the rules.

In scalacenter/scalafix#1108, I have noted

change sbt-scalafix to run scalafix_2.13 by default instead of scalafix_2.12

I guess the "by default" imply that it would be possible to switch version anyway, so maybe it wouldn't be that much of a problem as sbt-scalafix could adapt to the rules scala version?

@bjaglin
Copy link
Collaborator Author

bjaglin commented May 16, 2020

Rebased and updated the first commit to clarify and improve the classloading layering.

* Handle internalDependencyClasspath & externalDependencyClasspath
  separately, so that Coursier resolves transitive dependencies of
  local & remote rules together and evicts as needed
* Tweak classloader hierarchy to keep core & global external
  dependencies warm across projects & invocations
@bjaglin
Copy link
Collaborator Author

bjaglin commented May 19, 2020

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