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

non-triggered scripted plugin: SbtPlugin #3875

Merged
merged 6 commits into from Jan 24, 2018

Conversation

Projects
None yet
4 participants
@eed3si9n
Member

eed3si9n commented Jan 13, 2018

This PR takes care of a few scripted plugin related issues at one fell swoop.

  1. Scripted plugin is no longer triggered. (#3514 suggested by @alexarchambault)
  2. New SbtPlugin is added to the sbt mothership that depends on sbt.ScriptedPlugin. (#3538 suggested by @jvican)
  3. Some scripted default settings are now scoped globally. (#3656 suggested by @jvican)

The net effect is that a plugin author will be able to use scripted with

lazy val fooPlugin = (project in file("plugin"))
  .enablePlugins(SbtPlugin)

lazy val app = project

and it no longer requires disabling in other subprojects (like app above).

notes about name conflict and bincompat

There is a name conflict between sbt.test package and sbt.Keys.test.

[error] /tmp/sbt_ea607a5c/build.sbt:5: error: reference to test is ambiguous;
[error] it is imported twice in the same scope by
[error] import _root_.sbt.Keys._
[error] and import _root_.sbt._
[error]     parallelExecution in test := false
[error]                          ^

Here are the workaround steps that I took.

  1. sbt.test package is renamed to sbt.scriptedtest. This allows 1.0 plugins and builds to use test to mean Keys.test.
  2. To keep binary compatibility for sbt 0.13 scripted, I am adding sbt.test.ScriptedRunner and sbt.test.ScriptedTests in scripted-plugin artifact.
  3. Another affected user is Giter8 plugin that uses ScriptedPlugin. Since the intereactions are limited to sbt.ScriptedPlugin.*, we should be fine here. - https://github.com/foundweekends/giter8/blob/v0.11.0-M2/plugin/src/main/scala-sbt-1.0/giter8/SBTCompat.scala

@eed3si9n eed3si9n added this to the 1.2.0 milestone Jan 13, 2018

@eed3si9n eed3si9n requested a review from dwijnand Jan 13, 2018

@eed3si9n

This comment has been minimized.

Member

eed3si9n commented Jan 13, 2018

I half expected this was gonna happen, but including scripted library is causing major name conflict problems because it uses sbt.test package.

sbt.test collides with sbt.Keys.test, which is very commonly used in plugins and build.sbt that contains import sbt._. The workaround would be to move scripted into sbt.scriptedtest or something, but that would of break the bincompat with 0.13.16's scripted - https://github.com/sbt/sbt/blob/v0.13.16/scripted/plugin/src/main/scala/sbt/ScriptedPlugin.scala#L34

      ModuleUtilities.getObject("sbt.test.ScriptedTests", loader)
@jvican

This comment has been minimized.

Member

jvican commented Jan 13, 2018

Can you not add import sbt.Keys._ manually for every build.sbt evaluation? That should make the right test have preference over the sbt.test package.

@eed3si9n

This comment has been minimized.

Member

eed3si9n commented Jan 13, 2018

@jvican Keys._ already is imported. The error message on Travis looks like this:

[error] /tmp/sbt_ea607a5c/build.sbt:5: error: reference to test is ambiguous;
[error] it is imported twice in the same scope by
[error] import _root_.sbt.Keys._
[error] and import _root_.sbt._
[error]     parallelExecution in test := false
[error]                          ^
@eed3si9n

This comment has been minimized.

Member

eed3si9n commented Jan 13, 2018

I wonder if I can use the dark arts technique of using package protected to maintain bincompat.
https://twitter.com/eed3si9n/status/835750967949697024

@dwijnand

LGTM. 1 concern

override lazy val globalSettings = Seq(
scriptedBufferLog := true,
scriptedSbt := (sbtVersion in pluginCrossBuild).value,

This comment has been minimized.

@dwijnand

dwijnand Jan 15, 2018

Member

This is a breaking semantic change because most users don't set sbtVersion in pluginCrossBuild in Global scope, they set it in some project scope (like the root project scope).`

(If you think this is wrong and should be fixed, please vote for #2899)

This comment has been minimized.

@eed3si9n

eed3si9n Jan 17, 2018

Member

See https://github.com/sbt/sbt/blob/v1.1.0/main/src/main/scala/sbt/PluginCross.scala#L46-L47:

        state.log.info(s"Setting `sbtVersion in pluginCrossBuild` to $version")
        val add = List(sbtVersion in GlobalScope in pluginCrossBuild :== version) ++

This comment has been minimized.

@dwijnand

dwijnand Jan 17, 2018

Member

There are 78 other users of sbtVersion in pluginCrossBuild (on GitHub) that probably aren't doing that though...

https://github.com/search?l=Scala&q=%22sbtVersion+in+pluginCrossBuild%22&type=Code&utf8=%E2%9C%93

This comment has been minimized.

@eed3si9n

eed3si9n Jan 17, 2018

Member

The majority of the users I think are just reading the value, so they should be ok, but I am happy to move scriptedSbt back into project scoping.

package sbt
// ScriptedPlugin has moved to main.

This comment has been minimized.

@dwijnand

dwijnand Jan 15, 2018

Member

Why does this file exist?

This comment has been minimized.

@eed3si9n

eed3si9n Jan 17, 2018

Member

This is sort of a memo to ourselves.
ScriptedPlugin itself has moved to the mothership, but I am leaving the module as is for backward compatibility of the build. In general having a subproject for the plugin, but not having the plugin defined in it is highly unusual, so I'm leaving the trail blaze here.

This comment has been minimized.

@dwijnand

dwijnand Jan 17, 2018

Member

I see what you mean.

scriptedBufferLog := true,
scriptedLaunchOpts := Seq(),
)
override lazy val projectSettings = Seq(
ivyConfigurations ++= Seq(ScriptedConf, ScriptedLaunchConf),
scriptedSbt := (sbtVersion in pluginCrossBuild).value,

This comment has been minimized.

@eed3si9n

eed3si9n Jan 17, 2018

Member

Fixed scriptedSbt.

@dwijnand dwijnand referenced this pull request Jan 18, 2018

Closed

ParseKey failures on CI #3893

@dwijnand

This comment has been minimized.

Member

dwijnand commented Jan 18, 2018

I think the project / scripted-skip-incompatible failure is legit.

@sjrd

This comment has been minimized.

sjrd commented Jan 22, 2018

Although this is clearly better than what I was afraid of in #3538, I would suggest one improvement: switch the dependencies between ScriptedPlugin and SbtPlugin.

To me it's clear that any project enabling ScriptedPlugin also needs to be an sbt plugin, so ScriptedPlugin-depends-on-SbtPlugin makes sense. On the other hand, not all sbt plugins need to be tested with scripted, so the other direction is not necessary/desirable.

To further argue my point: given your current definition, can you imagine a scenario where a project would enablePlugins(ScriptedPlugin) but not SbtPlugin? Personally I can't.

@eed3si9n

This comment has been minimized.

Member

eed3si9n commented Jan 22, 2018

It's currently not coded using requires because of cross building, but Giter8Plugin depends on ScriptedPlugin to reuse launch-sbt-for-testing behavior:
https://github.com/foundweekends/giter8/blob/c65f9f41ed18c8fc757c352a02c298059f8bb77d/plugin/src/main/scala/Giter8Plugin.scala
https://github.com/foundweekends/giter8/blob/c65f9f41ed18c8fc757c352a02c298059f8bb77d/plugin/src/main/scala-sbt-1.0/giter8/SBTCompat.scala

So we have a Venn diagram of large overlap between scripted and sbt plugins, with Giter8 and Scala.JS (and possibly others of course) as some holdouts.

eed3si9n added some commits Jan 13, 2018

Add SbtPlugin
Fixes #3538

This brings in `sbt.ScriptedPlugin` as `sbt.plugins.ScriptedPlugin` into sbt mothership.
In addition, `sbt.plugins.SbtPlugin` is added that enables the scripted plugin and `sbtPlugin := true`.

This allows plugin authors to bring in scripted plugin by writing:

```scala
lazy val root = (project in file("."))
  .enablePlugins(SbtPlugin)
```
Work around package name confusion
This works around the name conflict between sbt.test package and sbt.Keys.test.

1. sbt.test package is renamed to sbt.scriptedtest. This allows 1.0 plugins and builds to use `test` to mean `Keys.test`.
2. To keep binary compatibility for sbt 0.13 scripted, I am adding `sbt.test.ScriptedRunner` and `sbt.test.ScriptedTests` in `scripted-plugin` artifact.
3. Another affected user is Giter8 plugin that uses ScriptedPlugin. Since the intereactions are limited to `sbt.ScriptedPlugin.*`, we should be fine here. - https://github.com/foundweekends/giter8/blob/v0.11.0-M2/plugin/src/main/scala-sbt-1.0/giter8/SBTCompat.scala
@@ -0,0 +1,2 @@
lazy val root = (project in file("."))
.enablePlugins(SbtPlugin)

This comment has been minimized.

@eed3si9n

eed3si9n Jan 22, 2018

Member

@dwijnand The Travis failure was legit. I had to migrate to the new way that explicitly enables the use of scripted at subproject definition.

@sjrd

This comment has been minimized.

sjrd commented Jan 22, 2018

OK. I do find this odd, but since you have at least one use case, that's good enough for me.

@dwijnand

This comment has been minimized.

Member

dwijnand commented Jan 23, 2018

Looks like AppVeyor won't pass. @eed3si9n want to look into that?

@eed3si9n

This comment has been minimized.

Member

eed3si9n commented Jan 24, 2018

Yea. I'll ignore AppVeyor for now and merge, and I'll take a look at it.

@eed3si9n eed3si9n merged commit be6bbcd into sbt:1.x Jan 24, 2018

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Codacy/PR Quality Review Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@eed3si9n eed3si9n deleted the eed3si9n:wip/scripted branch Jan 24, 2018

@eed3si9n eed3si9n referenced this pull request Jul 2, 2018

Closed

[sbt 1.2.0-RC1] reference to test is ambiguous #4242

1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment