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

Unnecessary-looking dependencies #2824

Closed
OrangeDog opened this issue May 23, 2017 · 27 comments
Closed

Unnecessary-looking dependencies #2824

OrangeDog opened this issue May 23, 2017 · 27 comments

Comments

@OrangeDog
Copy link

@OrangeDog OrangeDog commented May 23, 2017

New in Release 22 are a bunch of Maven dependencies that look like they should be compile-only, but are now required at runtime.

[INFO] +- com.google.guava:guava:jar:22.0:compile
[INFO] |  +- com.google.code.findbugs:jsr305:jar:1.3.9:compile
[INFO] |  +- com.google.errorprone:error_prone_annotations:jar:2.0.18:compile
[INFO] |  +- com.google.j2objc:j2objc-annotations:jar:1.1:compile
[INFO] |  \- org.codehaus.mojo:animal-sniffer-annotations:jar:1.14:compile
@cpovirk

This comment has been minimized.

Copy link
Member

@cpovirk cpovirk commented May 23, 2017

The goal here was to fix #2721. Evidently "compile-only" doesn't actually make the annotations available at compile time to projects that compile against Guava, and that causes problems for people who compile with all warnings as errors. But if making the dependencies required at runtime causes problems, too, we'll have to figure out what to do. Are they causing problems for you, or is it just surprising that we made this change?

@OrangeDog

This comment has been minimized.

Copy link
Author

@OrangeDog OrangeDog commented May 23, 2017

In case you didn't know "compile-only" is the provided scope. The compile scope is compile-time and later (i.e. including runtime and test).

Sounds like they should be either <optional>true</optional>, <scope>provided</scope>, or moved into a new "build tools" pom that people can depend on separately.

@cpovirk

This comment has been minimized.

Copy link
Member

@cpovirk cpovirk commented May 23, 2017

They used to be <optional>true</optional>, and that's what caused #2721. It's possible that <scope>provided</scope> would fare better; I don't know.

@OrangeDog

This comment has been minimized.

Copy link
Author

@OrangeDog OrangeDog commented May 23, 2017

I believe <scope>provided</scope> matches your intent, so is probably the correct solution.

@efenderbosch

This comment has been minimized.

Copy link

@efenderbosch efenderbosch commented May 23, 2017

Any chance of a quick 22.1 release to resolve this? Otherwise we'll be adding a ton of exclusions for dependency convergence.

@ghost

This comment has been minimized.

Copy link

@ghost ghost commented May 24, 2017

I believe #2721 (0e29934) is a reasonable change, but since it affects backward compatibility, I think it should be clarified at least in the release notes. Please be aware that changes to pom.xml do not appear in the Full JDiff Report.

@cpovirk

This comment has been minimized.

Copy link
Member

@cpovirk cpovirk commented May 25, 2017

I just tested with <scope>provided</scope>, and it reintroduces #2721 :(

I'll put something in the release notes. We should think more about whether dependency version bumps should show up in the release notes in general.

@cpovirk

This comment has been minimized.

Copy link
Member

@cpovirk cpovirk commented May 25, 2017

https://github.com/google/guava/wiki/Release22#additional-changes

That may be all we can do, but if anyone has other ideas, I'm all ears. Sorry for the trouble.

@cpovirk cpovirk closed this May 25, 2017
@OrangeDog

This comment has been minimized.

Copy link
Author

@OrangeDog OrangeDog commented May 25, 2017

Then my third suggestion still remains - move them to be dependencies of a new artifact.
Then people who just want to use the library don't have to add exclusions, and anyone who wants to build against it avoiding #2721 adds the extra dependency.

<dependency>
  <groupId>com.google.guava<groupId>
  <artifactId>guava-annotations</artifactId>  <!-- first name I thought of -->
  <type>pom</type>
  <scope>provided</scope>
</dependency>
@cpovirk

This comment has been minimized.

Copy link
Member

@cpovirk cpovirk commented May 25, 2017

Ah, sorry, I'd missed that one. I think I feel better with the current version, which requires action from people who use dependency convergence (and who presumably understand the problem and how to fix it), than with the proposed alternative, which requires action from people who are turning on warnings or using annotation processors (who might see a variety of different errors and might not know how to debug them). I know that that's still a bad experience for you; it just seems like it has to be a bad experience for someone for all the reasons to diamond dependencies normally are a bad experience :(

@guidomedina

This comment has been minimized.

Copy link

@guidomedina guidomedina commented Aug 1, 2017

Will Guava version 23.0 include that extra 4 dependencies?
I can still see them been dragged on our Maven project by 23.0-rc1

@kluever

This comment has been minimized.

Copy link
Member

@kluever kluever commented Aug 1, 2017

/cc @cgdecker re. 23.0-rc1 concern

@cgdecker

This comment has been minimized.

Copy link
Member

@cgdecker cgdecker commented Aug 2, 2017

@cpovirk's conclusion above seems to have been that of the two options we have, both of which cause issues for some people in some way, we should stick with what we're currently doing. So 23.0 will continue to have those dependencies as non-optional.

@guai

This comment has been minimized.

Copy link

@guai guai commented Jan 15, 2018

This is madness
Just make them provided. Its how they should be declared
If this declaration causes bugs then these bugs are somewhere else

@jbduncan

This comment has been minimized.

Copy link
Contributor

@jbduncan jbduncan commented Jan 15, 2018

The JUnit 5 team have gone and made their dependency on opentest4j (another annotations library) compile scope now, for reasons given in junit-team/junit5#1105.

Maybe it goes to show that annotations aren't really as optional as we all might think? 🤔

@guidomedina

This comment has been minimized.

Copy link

@guidomedina guidomedina commented Jan 15, 2018

I'm dealing with it by using exclusion on its dependencies, ugly as hell but it works.

@cpovirk

This comment has been minimized.

Copy link
Member

@cpovirk cpovirk commented Jan 16, 2018

@jbduncan , thanks for the link to the JUnit 5 change. I mentioned it on the Maven feature request for a true compile-time scope.

vruusmann added a commit to jpmml/jpmml-evaluator that referenced this issue Feb 18, 2018
@cpovirk cpovirk mentioned this issue Mar 27, 2018
@jsichi jsichi mentioned this issue Apr 3, 2018
fdlk added a commit to fdlk/molgenis that referenced this issue May 16, 2018
fdlk added a commit to fdlk/molgenis that referenced this issue May 17, 2018
dennishendriksen added a commit to molgenis/molgenis that referenced this issue May 17, 2018
fdlk added a commit to fdlk/molgenis that referenced this issue Jun 29, 2018
…loading and revered to WEB-INF/classes/molgenis.properties

feat(Jenkinsfile): added for building development war for Docker production

fix(MolGEnisWebappSecurity.java) disabled hsts in webapp, you can set this enabled when you do not run this application on apache and offload ssl on it

chore(travis.yml): disabled travis build

fix(Jenkinsfile): add Jenkins build

fix(molgenis.home): get this setting from the system environment instead of contained JVM environment

chore(Jenkinsfile): updated scp flow to download server

feat(Jenkinsfile): updated to run tests and publish to sonatype

fix(Jenkinsfile): removed failure block

fix(Jenkinsfile): removed publish block

fix(Jenkinsfile): removed post block

fix(Jenkinsfile): set JAVA_HOME and PATH

fix(Jenkinsfile): set JAVA_HOME and PATH

fix(Jenkinsfile): set JAVA_HOME and PATH

fix(Jenkinsfile): set JAVA_HOME and PATH

fix(Jenkinsfile): set JAVA_HOME and PATH

fix(agent): updated agent to maven

fix(jdk): upgraded to Oracle

fix(agent): fix syntax jenkinsfile

fix(agent): can not address node directly

updated build config

fix(codecov) bash script

fix(codecov): you need to do a different call to codecov in jenkinsfile

Updated build config according to sonaytype jenkins plugin

import github

comment github out

upgraded yarn for efficiency reasons

add sonar token in credentials plugin

update node version for performace reasons

updated sonar build

updated build config for build trigger

updated version number

Added codecov token

fix(jenkinsfile): closure of unit block

fix(jenkinsfile): closure of unit block

feat(Dockerfile): add Dockerfile

fix(Jenkinsfile): added build stage

fix(Jenkinsfile): ignore docker

validate date before saving WIP

fix autosave; pass state and data as payload and determine updatedField before committing the update

fix(actions): updated next tick configuration

fix(actions.spec.js): test in actions

fix eslint warning

ignore unused expression eslint warning in e2e spec

Only validate changed field before auto save

+

split auto save action into validate action and save action

fix(index.js): removed proxytable because of failing e2e tests

url-encode api calls

Exclude compile scope annotation dependencies from guava. See google/guava#2824 (molgenis#7362)

Fix molgenis#7365 Range type mismatch unclear error

feat(docker): add building a docker image

feat(docker): add building a docker image

feat(docker): add building a docker image

fix(sonar): quiet mode off

fix(sonar): quiet mode off

updated docker config and module deletion for sonar

Try to sign in release

Move pgp and nexus plugins out of release profile

deploy before publish

Fix sonar build step, run differently for PRs.

Fiddle fiddle.

Use different oauth token for github PR comments.

Merge build/test/deploy steps, doesn't look like they're saving time.

Provide settings xml to deploy task

Move Dockerfile to molgenis/app dir

Deploy to sonatype snapshot and docker hub.

Add milestone

Pick up versioning scheme from jx

Use kubernetes agent.

Try a complete build in the docker maven image, to see what happens

Don't skip js build

Redirect test output to file

Don't build docker image in the molgenis-app project yet.

Mount host docker socket and executable in maven container as a volume.

Build docker image and push it.

Switch to custom molgenis-maven docker image that already contains the docker executable

Try different registry

Move pod template to the Jenkins configuration.

Change container label.

Publish to registry.molgenis.org

Run tests as well

Fiddle with the jenkinsfile

Run codecov in alpine container

Deploy only the image, leave jar files that noone depends upon out of the maven repo

Docker build, tag, push, all in the package phase

Try to fix codecov script

Remove leftover dockerfile target from parent pom

Ditch the codecov debug flag

Prefix PR tag with PR-

Reinstate Prepare step, codecov needs to know GIT_COMMIT.

Update pod and container names

Add integration tests

Explicitly push docker snapshot, and remove the push from package phase.

rename pod label default -> molgenis

skip unit test result collection

add sonar support

added docker-compose

set right Dockerfile options

updated documentation

Fix syntax of Jenkinsfile
lrozenblyum added a commit to lrozenblyum/chess that referenced this issue Jan 9, 2019
compile time additional artifact and (sic!) a weird empty artifact.

Excluding compile-time only dependencies as per
google/guava#2824
cpovirk added a commit to google/jimfs that referenced this issue Oct 4, 2019
I was going to say that this also paves the way for including the annotation as a non-optional dependency, should we wish to follow our Guava precedent for annotations:
- google/guava#2824
- google/guava#2721

But I see that it's retention=SOURCE anyway, so there isn't much reason to do that -- except maybe consistency with other annotation packages someday. (Maybe it's still a negative then, as it might still let people rely on our transitive dependency?)

I think the relationship of all this to Java 11 was that I might have to set an Automatic-Module-Name on AutoService, and it makes more sense to set it after we've done the processor-vs.-annotation artifact split. Once I was upgrading, it made sense to set up the annotation processor the Right Away, now that we're using a version in which that works. (Or maybe it always worked but now it's nice that it gets the processor off the classpath?) Or maybe there was some other reason for the change to the annotation-processor setup; once again, I forget. It looks like it might have been that AutoService stops running when I switch how we run Error Prone. Hopefully this was the solution :) But it's probably a good idea in any case.

This CL is basically following the "alternatively" instructions in https://github.com/google/auto/blob/master/value/userguide/index.md#in-pomxml

...even though the AutoService instructions haven't been similarly updated yet: https://github.com/google/auto/tree/master/service#download

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=272720556
@cpovirk cpovirk mentioned this issue Oct 4, 2019
cpovirk added a commit to google/jimfs that referenced this issue Oct 4, 2019
I was going to say that this also paves the way for including the annotation as a non-optional dependency, should we wish to follow our Guava precedent for annotations:
- google/guava#2824
- google/guava#2721

But I see that it's retention=SOURCE anyway, so there isn't much reason to do that -- except maybe consistency with other annotation packages someday. (Maybe it's still a negative then, as it might still let people rely on our transitive dependency?)

I think the relationship of all this to Java 11 was that I might have to set an Automatic-Module-Name on AutoService, and it makes more sense to set it after we've done the processor-vs.-annotation artifact split. Once I was upgrading, it made sense to set up the annotation processor the Right Away, now that we're using a version in which that works. (Or maybe it always worked but now it's nice that it gets the processor off the classpath?) Or maybe there was some other reason for the change to the annotation-processor setup; once again, I forget. It looks like it might have been that AutoService stops running when I switch how we run Error Prone. Hopefully this was the solution :) But it's probably a good idea in any case.

This CL is basically following the "alternatively" instructions in https://github.com/google/auto/blob/master/value/userguide/index.md#in-pomxml

...even though the AutoService instructions haven't been similarly updated yet: https://github.com/google/auto/tree/master/service#download

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=272720556
@davidljuba15031979

This comment has been minimized.

Copy link

@davidljuba15031979 davidljuba15031979 commented Oct 19, 2019

[INFO] |  |  +- com.google.guava:guava:jar:28.1-jre:compile
[INFO] |  |  |  +- com.google.guava:failureaccess:jar:1.0.1:compile
[INFO] |  |  |  +- com.google.guava:listenablefuture:jar:9999.0-empty-to-avoid-conflict-with-guava:compile
[INFO] |  |  |  +- com.google.code.findbugs:jsr305:jar:3.0.2:compile
[INFO] |  |  |  +- org.checkerframework:checker-qual:jar:2.8.1:compile
[INFO] |  |  |  +- com.google.errorprone:error_prone_annotations:jar:2.3.2:compile
[INFO] |  |  |  +- com.google.j2objc:j2objc-annotations:jar:1.3:compile
[INFO] |  |  |  \- org.codehaus.mojo:animal-sniffer-annotations:jar:1.18:compile

Copy-paste exclusions for future readers...

            <groupId>com.google.guava</groupId>
            <artifactId>guava</artifactId>
            <version>28.1-jre</version>
            <exclusions>
                <!--https://github.com/google/guava/issues/2824-->
                <exclusion>
                    <groupId>com.google.guava</groupId>
                    <artifactId>failureaccess</artifactId>
                </exclusion>
                <exclusion>
                    <groupId>com.google.guava</groupId>
                    <artifactId>listenablefuture</artifactId>
                </exclusion>
                <exclusion>
                    <groupId>com.google.code.findbugs</groupId>
                    <artifactId>jsr305</artifactId>
                </exclusion>
                <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>
@guidomedina

This comment has been minimized.

Copy link

@guidomedina guidomedina commented Oct 19, 2019

@davidljuba15031979 failureaccess is needed in half of the cases and in few cases you could need listenablefuture if you use that concurrent facility.

@talios

This comment has been minimized.

Copy link

@talios talios commented Oct 21, 2019

@guidomedina maybe they should be <optional>true</optional> then?

@guidomedina

This comment has been minimized.

Copy link

@guidomedina guidomedina commented Oct 21, 2019

@guidomedina maybe they should be <optional>true</optional> then?

I don't think they are going to change it, we will have to live with the long exclusion list.

@milosonator

This comment has been minimized.

Copy link

@milosonator milosonator commented Oct 21, 2019

I am surprised this hasn't been dealt with properly at this time. We use Guava in a lot of our projects and have a bunch of custom build code just dealing with excluding all the compile-time stuff from our runtime and distributions. It's a huge hassle to deal with this and there are no other projects doing something like this. Really hope this will all be reverted.

@guidomedina

This comment has been minimized.

Copy link

@guidomedina guidomedina commented Oct 22, 2019

There is a very old comment regarding splitting and moving the annotations classes to guava-annotations #2824 (comment)

Then probably making such optional or provided which is the one dragging most of the extra dependencies, please guys go back to that comment and re-read previous comments which makes me believe it would be a better solution to this problem.

Initially there were only 4 extra dependencies but now there are 6 already which has been becoming more and more of a hassle.

It would make the dependencies tree look like this (notice the com.google.guava:annotations I added to the original tree):

|  |  +- com.google.guava:guava:jar:28.1-jre:compile
|  |  |  +- com.google.guava:failureaccess:jar:1.0.1:compile
|  |  |  +- com.google.guava:listenablefuture:jar:9999.0-empty-to-avoid-conflict-with-guava:compile
|  |  |  +- com.google.guava:annotations:jar:1.0.0:compile
|  |  |  | +- com.google.code.findbugs:jsr305:jar:3.0.2:compile
|  |  |  | +- org.checkerframework:checker-qual:jar:2.8.1:compile
|  |  |  | +- com.google.errorprone:error_prone_annotations:jar:2.3.2:compile
|  |  |  | +- com.google.j2objc:j2objc-annotations:jar:1.3:compile
|  |  |  | \- org.codehaus.mojo:animal-sniffer-annotations:jar:1.18:compile
@cpovirk

This comment has been minimized.

Copy link
Member

@cpovirk cpovirk commented Oct 22, 2019

I see. The previous post had suggested that the guava-annotations artifact would be provided or optional (and maybe you're suggesting that, too)? I still believe that provided/optional is the wrong solution, since it's not a true compile-time scope (and Maven doesn't provide such a scope).

The idea of a non-provided, non-optional guava-annotations artifact is a little more appealing. It would (I think?) give users an easy way to exclude all the annotation artifacts. It does still have downsides:

  • We'll have to find a name that makes clear that it doesn't contain com.google.common.annotations.
  • I don't know if tools will be upset that Guava will then be relying on a transitive dependency for its annotations.
  • People may depend on guava-annotations directly, at which point they might see conflicts with the version that Guava itself is using.

For that reason, I'm still not sure that a change is a net win.

In better news:

  • In the medium term, we expect to replace jsr305 and checker-qual with a single artifact of nullness annotations.
  • animal-sniffer-annotations was just removed in #3652 (though we haven't made a release with that yet).
  • I'd be happy to take a similar pull request that replaces the j2objc-annotations dependency with package-private custom @J2ObjCIncompatible annotations.
@guidomedina

This comment has been minimized.

Copy link

@guidomedina guidomedina commented Oct 22, 2019

@cpovirk I would say it is fine as a normal dependency (guava-annotations), it will be very easy to exclude and can be re-worked/re-factored separately in the future for people looking to use it.

@cpovirk

This comment has been minimized.

Copy link
Member

@cpovirk cpovirk commented Oct 23, 2019

Still unsure, and keeping this on the back burner, but that's the best alternative I've heard so far. Call it "guava-optional-at-runtime-deps" or something.

Also, I was confused about j2objc: We use more annotations than just J2ObjCIncompatible. I had briefly hoped that we could continue to use the j2objc artifact but make it provided or optional, since J2ObjCIncompatible has source retention. But other annotations that we use from the artifact have class retention.

@guidomedina

This comment has been minimized.

Copy link

@guidomedina guidomedina commented Oct 23, 2019

@cpovirk this is a well known used pattern by other projects, when an API starts getting cluttered by "non-related" things or cover several groups then each group is put into a smaller artifact specialized in doing a single group of things, annotations is a good example, for example the famous Jackson dependencies have:

  • jackson-core
  • jackson-databind
  • jackson-annotations

This also applies to many other projects which do exactly this same split process; cluttering an API without clear division/delegation of responsibilities has a negative effect; I will tell you what I think before I decide to include Guava in my projects:

  • Week 1: I only need one function that I can implement myself, won't drag it yet.
  • Week 2: I only need two functions that I can implement myself, won't drag it yet.
  • Week 3: I only need three functions that I can implement myself, won't drag it yet; still not worth dragging 2mb jar + a train of unrelated dependencies just for this.
  • Week 4: I'm screwed, I have no other choice than to suffer the painful process of including Guava and excluding its train of dependencies.

Please don't take my thought process wrong; this is what the mind does and denying it would be hypocritical on my side, a good example of this would be to split Guava into:

  • guava-core
  • guava-collections
  • guava-concurrent
  • guava-annotations

Of course; in due time, guava-annotations can be a good start for this pattern and then slowly divide in between releases of major versions; another benefit in doing is that you won't be limited to add things to each group because such dependency will be added by people interested in such particular area, say; guava-graphs or guava-geometry, the possibilities are endless with this approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.