Remove AutoPlugin triggers from plugins. #389

fiadliel · 2014-10-26T12:36:48Z

With the current triggers, all plugins are enabled by enabling any other
(since all end up triggering on UniversalPlugin). After removing the
triggers, we can still include all plugins via JavaAppPackaging.

With the current triggers, all plugins are enabled by enabling any other (since all end up triggering on UniversalPlugin). After removing the triggers, we can still include all plugins via JavaAppPackaging.

fiadliel · 2014-10-26T13:36:11Z

Not yet complete; universal/publish test fails because deploymentSettings assumes that all plugins are initialised, and removing that causes Repository for publishing is not specified.

muuki88 · 2014-10-26T14:51:30Z

I see this could be a problem if you rely on enabling only a single plugin. I will look into more detail this week

fiadliel · 2014-10-26T17:27:16Z

This is one potential method. However, there is no "enable deployment settings for all plugins" option any more. The downside of creating a plugin that depends on all xxxDeployPlugin plugins, is that one can then (unexpectedly) enable xxxPlugin transitively.

Making these extra plugins auto-trigger means it is harder to disable them all (have to specify each one manually), but it is an option...

Deployment/publishing settings should be provided by a plugin which depends on the appropriate parent plugin. Replaces deploymentSettings which requires all plugins to be configured.

jsuereth · 2014-10-28T00:24:48Z

I do like the notion that having the plugins auto-trigger off of dependencies.

How dangerous is including them with a trigger in practice? Currently native packager includes almost all settings in one single sequence or another. The difference now is that you can manually enable/disable plugins (by hierarchy).

I do like the new tests that only test specific plugins. That should be the case no matter what we do.

Again, would love to hear more about what issues you're facing.

fiadliel · 2014-10-28T10:22:20Z

It's not so much an issue, as what I would see as a "principle of least astonishment" failure:

enablePlugins(SbtNativePackager)

> plugins
In file:/Users/gary/snp-test/
    sbt.plugins.IvyPlugin: enabled in snp-test
    sbt.plugins.JvmPlugin: enabled in snp-test
    sbt.plugins.CorePlugin: enabled in snp-test
    sbt.plugins.JUnitXmlReportPlugin: enabled in snp-test
    com.typesafe.sbt.SbtNativePackager: enabled in snp-test
    com.typesafe.sbt.packager.archetypes.AkkaAppPackaging
    com.typesafe.sbt.packager.archetypes.JavaAppPackaging
    com.typesafe.sbt.packager.archetypes.JavaServerAppPackaging
    com.typesafe.sbt.packager.debian.DebianPlugin: enabled in snp-test
    com.typesafe.sbt.packager.debian.JDebPackaging
    com.typesafe.sbt.packager.docker.DockerPlugin: enabled in snp-test
    com.typesafe.sbt.packager.linux.LinuxPlugin: enabled in snp-test
    com.typesafe.sbt.packager.rpm.RpmPlugin: enabled in snp-test
    com.typesafe.sbt.packager.universal.UniversalPlugin: enabled in snp-test
    com.typesafe.sbt.packager.windows.WindowsPlugin: enabled in snp-test

All well and good. But I would expect that enablePlugins(DockerPlugin) would behave differently:

enablePlugins(DockerPlugin)

> plugins
In file:/Users/gary/snp-test/
    sbt.plugins.IvyPlugin: enabled in snp-test
    sbt.plugins.JvmPlugin: enabled in snp-test
    sbt.plugins.CorePlugin: enabled in snp-test
    sbt.plugins.JUnitXmlReportPlugin: enabled in snp-test
    com.typesafe.sbt.SbtNativePackager: enabled in snp-test
    com.typesafe.sbt.packager.archetypes.AkkaAppPackaging
    com.typesafe.sbt.packager.archetypes.JavaAppPackaging
    com.typesafe.sbt.packager.archetypes.JavaServerAppPackaging
    com.typesafe.sbt.packager.debian.DebianPlugin: enabled in snp-test
    com.typesafe.sbt.packager.debian.JDebPackaging
    com.typesafe.sbt.packager.docker.DockerPlugin: enabled in snp-test
    com.typesafe.sbt.packager.linux.LinuxPlugin: enabled in snp-test
    com.typesafe.sbt.packager.rpm.RpmPlugin: enabled in snp-test
    com.typesafe.sbt.packager.universal.UniversalPlugin: enabled in snp-test
    com.typesafe.sbt.packager.windows.WindowsPlugin: enabled in snp-test

The result is exactly the same, and I think this may surprise people.

In addition, the SbtNativePackager plugin seems to be treated as the "entrypoint" to the hierarchy, but it's not clear that the archetypes are an alternative entrypoint, not a mixin, e.g.:

enablePlugins(SbtNativePackager)
enablePlugins(JavaAppPackaging)
disablePlugins(WindowsPlugin)

[error] sbt.AutoPluginException: Error determining plugins for project 'snp-test' in /Users/gary/snp-test:
[error] Failed to sort List(sbt.plugins.CorePlugin, com.typesafe.sbt.packager.archetypes.JavaAppPackaging, sbt.plugins.IvyPlugin, com.typesafe.sbt.packager.rpm.RpmPlugin, com.typesafe.sbt.packager.debian.DebianPlugin, com.typesafe.sbt.packager.docker.DockerPlugin, sbt.plugins.JvmPlugin, com.typesafe.sbt.packager.universal.UniversalPlugin, com.typesafe.sbt.packager.linux.LinuxPlugin, sbt.plugins.JUnitXmlReportPlugin, com.typesafe.sbt.SbtNativePackager) topologically

This might be fixable with renaming and updated documentation, but I'm not sure whether this is sufficient, or if the hierarchy should be updated.

But perhaps I should be filing issues instead of pull requests... (but the attached code was interesting so far as it showed some assumptions in the deployment settings when it was implemented).

jsuereth · 2014-10-28T12:24:47Z

src/main/scala/com/typesafe/sbt/packager/rpm/RpmPlugin.scala

@@ -109,3 +108,12 @@ object RpmPlugin extends AutoPlugin {
    ))
 }

+object RpmDeployPlugin extends AutoPlugin {


This should probably just be part of the RPM plugin (similarly for Debian/Universal/Windows etc.). The reason we didn't do this initially was because these settings were more experimental. Having used them in several projects, we can just embed them in the raw plugins now, in my opinion.

jsuereth · 2014-10-28T12:32:37Z

OK, so I wrote a huge wall of text that got deleted. However, let me throw my thoughts down again, more coherently.

User stories we should support:

Most users will just select an archetype for their project and do minor tweaks.
- All packaging types should be included (batteries loaded)
- They should only have to enable the archetype plugin, not all other plugins.
Advanced users will just select one packaging type and only want to configure it by hand
- They do not want all the other packaging types showing up
- They will be manually encoding things they need.
Third-Party packaging plugins would like to provide a new plugin for native packager users
- They want their packaging type to be offered w/ all the archetype offerings
- They do not want users to have to manually enable their packaging, but seamlessly integrate once included via addSbtPlugin

SO, my hierrachy proposal is the following (assuming deployment settings get unified with underlying plugins):

UniversalPlugin
Linux
Debian -> Linux (no trigger)
JDeb -> Debian (no trigger)
Rpm -> Linux (no trigger)
Windows
Docker
NativePackager -> Universal, Debian, Rpm, Windows, Docker (no trigger)
*XYZ*Archetype -> NativePackager (no trigger)

<THIRD PARTY GENERIC  PLUGIN (e.g. OSX bundle)> -> NativePackager (auto trigger)
<THIRD PARTY SPECIFIC PLUGIN (e.g. Play) -> *XYZ*Archetype

I think this may offer the best of all worlds in the sense of:

Limiting dependencies when needed
"Batteries included" feel
Open/Easy extension

WDYT?

fiadliel · 2014-10-28T13:38:36Z

And I assume (Linux, Windows, Docker) -> UniversalPlugin (no trigger)?

That seems reasonable to me. Without triggers, (Universal, Linux, Debian, JDeb, Rpm, Windows, Docker) are a directed tree with plugins enabled from the leaves, with Universal at the base of everything. And (NativePackager, _XYZ_Archetype) depend on the leaves of the first tree.

Also, this means that NativePackager should probably have its actual (non-dependency) functionality moved to UniversalPlugin.

As the AutoPlugin trigger feature currently stands, excessive use of triggers causes a large community of plugins which get enabled as a body, no matter which individual in the community was actually explicitly enabled. I wonder if there's room for another feature, override def trigger = explicitTrigger(plugins: Plugin*) (the plugin(s) should also be in the dependency list), causing the trigger to occur only if a particular plugin was explicitly called by enablePlugins. Or is it too complex?

fiadliel · 2014-10-28T20:38:11Z

@jsuereth The one downside of your proposal is that one cannot depend on the Java App archetype without also enabling all the plugins. It is quite minor, for a problem which most people probably won't care about.

But an example of a plugin which might do this: https://github.com/gilt/sbt-newrelic/pull/10

I see though that one cannot infinitely split functionality into different plugins, without fragmenting users' knowledge of how to use the project.

muuki88 · 2014-10-28T21:45:34Z

I'm with @jsuereth suggestion. The requires for an archetype are like a documentation: you can use all these packages now. Choose If a user is bothered by any plugin, disabling is always an option.

So the only thing left to do for this PR would be to move the RpmDeployPlugin into the RpmPlugin.
BTW @fiadliel , I like discussing around pull requests a lot more than just ideas.

muuki88 · 2014-11-06T21:56:44Z

Okay. I'll merge this one as I would like to release another milestone.
We won't document the DeployPlugins and merge them in another pr.

Remove AutoPlugin triggers from plugins.

Gary Coady added 2 commits October 26, 2014 12:32

Remove AutoPlugin triggers from plugins.

f969da4

With the current triggers, all plugins are enabled by enabling any other (since all end up triggering on UniversalPlugin). After removing the triggers, we can still include all plugins via JavaAppPackaging.

Update scripted tests to enable required plugin.

e870547

Create xxxDeployPlugin which provides deploy settings for xxxPlugin

edf70c0

Deployment/publishing settings should be provided by a plugin which depends on the appropriate parent plugin. Replaces deploymentSettings which requires all plugins to be configured.

jsuereth reviewed Oct 28, 2014
View reviewed changes

muuki88 mentioned this pull request Nov 6, 2014

Play backward compatibility issue #403

Merged

muuki88 added a commit that referenced this pull request Nov 6, 2014

Merge pull request #389 from fiadliel/feature/less_autoplugin_triggers

62fde8f

Remove AutoPlugin triggers from plugins.

muuki88 merged commit 62fde8f into sbt:master Nov 6, 2014

fiadliel deleted the feature/less_autoplugin_triggers branch November 26, 2014 11:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove AutoPlugin triggers from plugins. #389

Remove AutoPlugin triggers from plugins. #389

fiadliel commented Oct 26, 2014

fiadliel commented Oct 26, 2014

muuki88 commented Oct 26, 2014

fiadliel commented Oct 26, 2014

jsuereth commented Oct 28, 2014

fiadliel commented Oct 28, 2014

jsuereth Oct 28, 2014

jsuereth commented Oct 28, 2014

fiadliel commented Oct 28, 2014

fiadliel commented Oct 28, 2014

muuki88 commented Oct 28, 2014

muuki88 commented Nov 6, 2014

Remove AutoPlugin triggers from plugins. #389

Remove AutoPlugin triggers from plugins. #389

Conversation

fiadliel commented Oct 26, 2014

fiadliel commented Oct 26, 2014

muuki88 commented Oct 26, 2014

fiadliel commented Oct 26, 2014

jsuereth commented Oct 28, 2014

fiadliel commented Oct 28, 2014

jsuereth Oct 28, 2014

Choose a reason for hiding this comment

jsuereth commented Oct 28, 2014

fiadliel commented Oct 28, 2014

fiadliel commented Oct 28, 2014

muuki88 commented Oct 28, 2014

muuki88 commented Nov 6, 2014