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

fix: try to keep autovalue out of the runtime time classpath #48

Merged
merged 3 commits into from Oct 8, 2019

Conversation

elharo
Copy link
Contributor

@elharo elharo commented Oct 2, 2019

@elharo elharo requested a review from chingor13 Oct 2, 2019
@googlebot googlebot added the cla: yes label Oct 2, 2019
@elharo elharo changed the title try to keep autovalue out of the compile time classpath try to keep autovalue out of the runtime time classpath Oct 2, 2019
@codecov
Copy link

@codecov codecov bot commented Oct 2, 2019

Codecov Report

Merging #48 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #48   +/-   ##
=========================================
  Coverage     66.43%   66.43%           
  Complexity      367      367           
=========================================
  Files            34       34           
  Lines          1865     1865           
  Branches        236      236           
=========================================
  Hits           1239     1239           
  Misses          524      524           
  Partials        102      102

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65646ed...22796ea. Read the comment docs.

@saturnism
Copy link
Contributor

@saturnism saturnism commented Oct 2, 2019

i initially suggested optional; i just double checked auto value doc but it suggests provided scope. https://github.com/google/auto/blob/master/value/userguide/index.md#in-pomxml

/cc @eamonnmcmanus :)

@eamonnmcmanus
Copy link
Contributor

@eamonnmcmanus eamonnmcmanus commented Oct 2, 2019

Maven dependencies make my head hurt. I think <optional>true</> might actually be slightly more appropriate than <scope>provided</>. But I'm not sure it makes much difference in practice.

@saturnism
Copy link
Contributor

@saturnism saturnism commented Oct 2, 2019

/cc @rfscholte ? ;)

@saturnism
Copy link
Contributor

@saturnism saturnism commented Oct 2, 2019

The behavior is similar for both choices (i.e. consumer won't get this transitive dependency).

Both are documented here: https://maven.apache.org/guides/introduction/introduction-to-dependency-mechanism.html#Dependency_Scope

Provided:

This is much like compile, but indicates you expect the JDK or a container to provide the dependency at runtime. For example, when building a web application for the Java Enterprise Edition, you would set the dependency on the Servlet API and related Java EE APIs to scope provided because the web container provides those classes. This scope is only available on the compilation and test classpath, and is not transitive

Optional:

If project Y depends on project Z, the owner of project Y can mark project Z as an optional dependency, using the "optional" element. When project X depends on project Y, X will depend only on Y and not on Y's optional dependency Z. The owner of project X may then explicitly add a dependency on Z, at her option. (It may be helpful to think of optional dependencies as "excluded by default.")

In which case, optional does sound like a better choice based on semantics.

@elharo
Copy link
Contributor Author

@elharo elharo commented Oct 2, 2019

@elharo
Copy link
Contributor Author

@elharo elharo commented Oct 2, 2019

@chingor13
Copy link
Collaborator

@chingor13 chingor13 commented Oct 2, 2019

Should this actually be upstreamed to google-auth-library?

@elharo
Copy link
Contributor Author

@elharo elharo commented Oct 2, 2019

@chingor13 I'm not clear on how google-auth-library comes into play on this PR. The depedency is already declared in java-core's pom.xml.

@chingor13
Copy link
Collaborator

@chingor13 chingor13 commented Oct 2, 2019

@elharo java-core doesn't use auto-value at all, I'm not sure why we have the dependencyManagement entry. google-auth-library does declare a dependency on auto-value-annotations

@elharo
Copy link
Contributor Author

@elharo elharo commented Oct 2, 2019

Could be vestigial code then. Let me try removing it completely and see what happens.

@chingor13
Copy link
Collaborator

@chingor13 chingor13 commented Oct 2, 2019

You probably need to add the maven enforcer rule in this PR too.

@elharo
Copy link
Contributor Author

@elharo elharo commented Oct 2, 2019

Are you OK with the enforcer rule going here? You had asked for it to be moved to the shared config.

@chingor13
Copy link
Collaborator

@chingor13 chingor13 commented Oct 2, 2019

Do we know that this fixes the linkage check?

@elharo
Copy link
Contributor Author

@elharo elharo commented Oct 7, 2019

Yes, it appears to fix the linkage check. I could put the enforcer rule here to demonstrate that, but you asked for that to go elsewhere. If you want it here, please be explicit about that.

@elharo
Copy link
Contributor Author

@elharo elharo commented Oct 7, 2019

Also, whether this fixes the linkage check or not, it's still a good idea to remove unnecessary code where we can.

@chingor13 chingor13 changed the title try to keep autovalue out of the runtime time classpath fix: try to keep autovalue out of the runtime time classpath Oct 8, 2019
@chingor13 chingor13 merged commit 0988c27 into master Oct 8, 2019
12 checks passed
@chingor13 chingor13 deleted the optional branch Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants