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

Maven coordinates for shaded javac seem incorrect #169

Closed
ZacSweers opened this issue Aug 2, 2017 · 12 comments
Closed

Maven coordinates for shaded javac seem incorrect #169

ZacSweers opened this issue Aug 2, 2017 · 12 comments
Assignees

Comments

@ZacSweers
Copy link

ZacSweers commented Aug 2, 2017

In looking at the 1.3 release pom, it seems like the javac configuration is incorrect.

From: https://search.maven.org/remotecontent?filepath=com/google/googlejavaformat/google-java-format-parent/1.3/google-java-format-parent-1.3.pom

<properties>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<java.version>1.8</java.version>
<guava.version>19.0</guava.version>
<javac.version>9-dev-r3297-1-shaded</javac.version>
</properties>
<dependencyManagement>
<dependencies>
<!--  Required runtime dependencies  -->
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
<version>${guava.version}</version>
</dependency>
<dependency>
<groupId>com.google.errorprone</groupId>
<artifactId>javac</artifactId>
<version>${javac.version}</version>
</dependency>

The maven coordinates for the shaded javac are still the same as the normal javac, which means it will compete with any existing javac jars on the path even though the sources used by GJF are shaded, so if the normal javac jar is on the path and wins, GJF will fail to find the classes because they won't be shaded. It seems to me that the artifactId should also be different.

This is to say: If someone has another version of the error-prone javac jar on the path, it could be used instead of this shaded one, and subsequently break GJF since it depends on shaded package names that would no longer be present.

@msridhar
Copy link
Contributor

msridhar commented Aug 2, 2017

In particular, if there are Error Prone plugin checkers on the processor path, that could result in a different javac version also getting pulled in there.

@cushon cushon self-assigned this Aug 3, 2017
@ronshapiro
Copy link
Contributor

ronshapiro commented Aug 3, 2017

I think that what you're saying is that another com:google:errorprone:javac:not-shaded-javac could be in the dependency tree, causing some build systems to not place the shaded version on the classpath at all (I initially read it as saying that both may be on the classpath, but one might win, which isn't true). Is that correct? If so it may be worth updating what you wrote, as I find it confusing.

If that's the case, that sounds reasonable to me. What you'd like is for one library's error-prone javac code to not conflict with the version that GJF needs shaded.

@msridhar
Copy link
Contributor

msridhar commented Aug 3, 2017

@ronshapiro yes, that's exactly right, we were only seeing the not-shaded version in the processorpath, which broke GJF.

@ZacSweers
Copy link
Author

Updated the wording

@ZacSweers
Copy link
Author

Any thoughts on this? Seems like a fairly serious issue with the release in its current state, as no one can use custom error prone checkers and GJF-using tools like dagger in the same project

@cushon
Copy link
Collaborator

cushon commented Aug 17, 2017

I'm going to fix this soonish.

cushon added a commit that referenced this issue Aug 18, 2017
See #169

MOE_MIGRATED_REVID=165753177
@cushon
Copy link
Collaborator

cushon commented Aug 18, 2017

I published a new javac version 9-dev-r4023-3 with separate javac and javac-shaded artifacts, and updated GJF to use it in fee1ed2.

@ZacSweers
Copy link
Author

Great. I hate to be that person, but any chance of a (patch) release soon? Otherwise we can just do a fork of this internally for now

@sormuras
Copy link
Contributor

@hzsweers You can also use jitpack to compile and provide the current master snapshot version. See https://jitpack.io/#google/google-java-format/master-SNAPSHOT for details how to declare a dependency in Maven, Gradle, etc.

@ZacSweers
Copy link
Author

We run on buck, so unfortunately jitpack isn't a straightforward option. It's easy enough for us to take a cut of master and stick it up on our internal artifactory though, was just asking in case there was a release planned for like, tomorrow or something before going through the trouble :)

@cushon
Copy link
Collaborator

cushon commented Aug 29, 2017

I released it: https://github.com/google/google-java-format/releases/tag/google-java-format-1.4

@cushon cushon closed this as completed Aug 29, 2017
@ZacSweers
Copy link
Author

Thanks Liam!

ronshapiro pushed a commit to google/dagger that referenced this issue Aug 30, 2017
ronshapiro pushed a commit to google/dagger that referenced this issue Aug 31, 2017
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

No branches or pull requests

5 participants