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

Exclude guava's annotations dependencies #230

Conversation

findepi
Copy link
Contributor

@findepi findepi commented Jul 3, 2020

Reference https://github.com/prestosql/presto/blob/03aa1b6f9d9ced2807a33d578384c6e05a6f939a/presto-jdbc/pom.xml#L62-L83:

<dependency>
    <groupId>com.google.guava</groupId>
    <artifactId>guava</artifactId>
    <exclusions>
        <exclusion>
            <groupId>org.checkerframework</groupId>
            <artifactId>checker-qual</artifactId>
        </exclusion>
        <exclusion>
            <groupId>com.google.errorprone</groupId>
            <artifactId>error_prone_annotations</artifactId>
        </exclusion>
        <exclusion>
            <groupId>com.google.j2objc</groupId>
            <artifactId>j2objc-annotations</artifactId>
        </exclusion>
        <exclusion>
            <groupId>org.codehaus.mojo</groupId>
            <artifactId>animal-sniffer-annotations</artifactId>
        </exclusion>
    </exclusions>
</dependency>

Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

I don’t think this is the right approach. These annotations need to be available to tools like Error Prone that process the annotations. It also causes warnings from javac.

The real goal, as I understand, is to prevent these from being packaged for use at runtime. It seems better to attack that problem directly.

@electrum
Copy link
Member

electrum commented Jul 4, 2020

More discussion here: google/guava#2824

@findepi
Copy link
Contributor Author

findepi commented Jul 4, 2020

The real goal, as I understand, is to prevent these from being packaged for use at runtime. It seems better to attack that problem directly.

Sure. I do not think we should copy https://github.com/prestosql/presto/blob/03aa1b6f9d9ced2807a33d578384c6e05a6f939a/presto-jdbc/pom.xml#L62-L83 into each of the connectors, but I do think I know the proper way. How should this look like?

@findepi findepi closed this Jul 4, 2020
@findepi findepi deleted the findepi/master/exclude-guava-s-annotations-dependencies-80468b branch July 4, 2020 19:26
@electrum
Copy link
Member

electrum commented Jul 4, 2020

This seems to be a low priority concern. While it's true that the JARs are not needed at runtime, excluding the would only save a couple hundred kilobytes in total (since they are physically shared across plugins). The reason we exclude them from JDBC is that it's easier to exclude unnecessary dependencies than it is to shade them (plus size is more of a concern there).

@findepi
Copy link
Contributor Author

findepi commented Jul 4, 2020

I am not concerned about size. I know these are small.
Maybe I just incorrectly assumed this would be an improvement.
-- let's continue discussion in trinodb/trino#4336.

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.

None yet

2 participants