prestodb / presto-maven-plugin

Maven packaging and lifecycle for Presto plugins
Apache License 2.0
7 stars 27 forks source link

plugin should not flag all 'provided' scope dependencies as errors #9

Open hgschmie opened 6 years ago

hgschmie commented 6 years ago

When using google auto (https://github.com/google/auto/tree/master/value), the presto maven plugin flags it with:

[ERROR] Failed to execute goal com.facebook.presto:presto-maven-plugin:0.3:check-spi-dependencies (default-check-spi-dependencies) on project owl-presto-connector:
[ERROR]
[ERROR] Presto plugin dependency com.google.auto.value:auto-value must not have scope 'provided'. It is not part of the SPI and will not be available at runtime.
[ERROR] -> [Help 1]
[ERROR]

Google auto value is an annotation processor that executes at compile time and is not needed at run time. Presto should not flag this dependency as erroneous.

Right now, I need to use version 0.4 of the plugin and add

<configuration
    <allowedProvidedDependencies>com.google.auto.value:auto-value</allowedProvidedDependencies>
</configuration>

to the plugin configuration.

electrum commented 6 years ago

The purpose of the checker is to detect a common error: adding a provided dependency on something in the Presto engine and then being surprised when it’s not available at runtime due to class loader isolation.

Needing to add a non-SPI provided dependency to a plugin seems to be uncommon, as we only added that allowedProvidedDependencies option last week. None of the plugins in the main Presto tree nor our internal extensions at Facebook need it.

For AutoValue and the use case for which the option was added, it seems like a Maven deficiency of not having a compile-only scope. (Provided scope is not correct here since we don’t want it in the runtime or the test class paths.)

Do you have a suggestion on how we can improve this?

hgschmie commented 6 years ago

I see that the purpose of this plugin seems to be "the other way around" than I thought: Not to ensure that SPI dependencies are scoped as "provided" and not "compile" but to circumvent the "cut'n'paste dependency, oops I got one that is provided scoped and did not notice). So you want to enforce to only ever the SPI deps are in provide scope, nothing else.

How about introducing a whitelist and add a couple of well known deps? The google auto family seems to be prime candidates. E.g., we are starting to put pressure on teams at $dayjob to not write their own value classes anymore but use AutoValue exclusively. I am planning to use it for all the handle classes in my connector and while the 'allowedProvidedDependencies' config is a workable compromise, it feels kludgy.

electrum commented 5 years ago

The maven-compiler-plugin now has <annotationProcessorPaths> which might be what you want for the AutoValue.