Skip to content
This repository was archived by the owner on Dec 3, 2023. It is now read-only.

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

Merged
chingor13 merged 3 commits into
masterfrom
optional
Oct 8, 2019
Merged

fix: try to keep autovalue out of the runtime time classpath#48
chingor13 merged 3 commits into
masterfrom
optional

Conversation

@elharo

Copy link
Copy Markdown
Contributor

@elharo
elharo requested a review from chingor13October 2, 2019 14:16
@googlebotgooglebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 2, 2019
@elharoelharo changed the title try to keep autovalue out of the compile time classpathtry to keep autovalue out of the runtime time classpathOct 2, 2019
@codecov

codecovBot commented Oct 2, 2019

Copy link
Copy Markdown

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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

/cc @rfscholte ? ;)

@saturnism

Copy link
Copy Markdown
Contributor

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

elharo commented Oct 2, 2019 via email

Copy link
Copy Markdown
ContributorAuthor

@elharo

elharo commented Oct 2, 2019 via email

Copy link
Copy Markdown
ContributorAuthor

@chingor13

Copy link
Copy Markdown
Contributor

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

@elharo

Copy link
Copy Markdown
ContributorAuthor

@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
Copy Markdown
Contributor

@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
Copy Markdown
ContributorAuthor

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

@chingor13

Copy link
Copy Markdown
Contributor

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

@elharo

Copy link
Copy Markdown
ContributorAuthor

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

@chingor13

Copy link
Copy Markdown
Contributor

Do we know that this fixes the linkage check?

@elharo

Copy link
Copy Markdown
ContributorAuthor

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

elharo commented Oct 7, 2019

Copy link
Copy Markdown
ContributorAuthor

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

@chingor13chingor13 changed the title try to keep autovalue out of the runtime time classpathfix: try to keep autovalue out of the runtime time classpathOct 8, 2019
@chingor13
chingor13 merged commit 0988c27 into masterOct 8, 2019
@chingor13
chingor13 deleted the optional branch October 8, 2019 17:35
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yesThis human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@elharo@saturnism@eamonnmcmanus@chingor13@googlebot