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

Update Maven usage in README to suggest annotationProcessorPaths #293

Closed
wants to merge 1 commit into from

Conversation

tbroyer
Copy link

@tbroyer tbroyer commented Jan 21, 2016

Note that the online doc (gh-pages ) needs to be updated too; I'll do a followup pull request if this one's accepted.

@tbroyer tbroyer mentioned this pull request Jan 21, 2016
<groupId>com.google.auto.value</groupId>
<artifactId>auto-value</artifactId>
<version>${auto.value.version}</version>
<scope>provided</scope> <!-- to leave out of the all-deps jar -->
<scope>provided</scope> <!-- only needed for the annotations -->

Choose a reason for hiding this comment

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

Hopefully this will motivate google/auto#268 😀

@tbroyer
Copy link
Author

tbroyer commented Jan 24, 2016

Hmm, maybe samples should not be updated to annotationProcessorPaths, it has side-effects on the execution graph of the reactor build (i.e. mvn package -pl examples/simple -am will not build the annotation processor but will instead resolve it from the repositories).
See http://blog.ltgt.net/maven-is-broken-by-design-2/ for details.

It's also worth waiting for version 3.6 of the maven-compiler-plugin that will fix the incremental build issue introduced in 3.2 (three years ago or so).

Feel free to close this PR following those findings.

@tbroyer
Copy link
Author

tbroyer commented Mar 10, 2016

Updated to only use annotationProcessorPaths in integration tests (not in samples), and fixed documentation in README.

Use of annotationProcessorPaths (i.e. -processorpath) can be particularly useful when using GWT, given #228, to isolate dagger-compiler from your classpath and prevent clashes with GWT's own bundled Eclipse JDT (which I haven't experienced, but could still be possible; I think dagger-compiler came after gwt-dev in the classpath, and google-java-format seems to work OK with the version of Eclipse JDT shipped within GWT; the reverse might not be true though: GWT ships a modified version of JDT with a backported patch to workaround a memory consumption issue)

@@ -69,10 +63,17 @@ limitations under the License.
<plugins>
<plugin>
<artifactId>maven-compiler-plugin</artifactId>
<version>3.1</version>
<version>3.5.1</version>

Choose a reason for hiding this comment

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

Can you define a variable for this in the root pom.xml?

Choose a reason for hiding this comment

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

Or, is there a way to set the default and then have that be implicit?

@ronshapiro
Copy link

I'm in the process of switching us to use bazel, so besides the README and users guide, this won't be relevant soon (I hope). Do you still think it's useful in those two files?

@tbroyer
Copy link
Author

tbroyer commented Jan 3, 2017

Yay for bazel though I wonder how you'll deploy releases (and snapshots?) to Central.

But yes, probably not worth it. Feel free to close.

@JakeWharton
Copy link

You can still use mvn to deploy with mvn deploy:deploy-file (example here). You can also do individual HTTP PUTs or a multi-part POST into a Sonatype staging repo (which is all Maven does).

@tbroyer
Copy link
Author

tbroyer commented Jan 3, 2017

I rather meant the non/less-technical part: ensuring java_library↔pom parity, making sure things work for non-bazel users, etc.

annotationProcessorPaths has some issues (no dependencyManagement,
dependency:go-offline, or versions:display-dependency-updates), so
don't replace the current documentation using optional dependency.

Also update net.ltgt.apt plugin in Gradle usage to the latest
version.
@tbroyer
Copy link
Author

tbroyer commented Apr 30, 2017

Now that the build has moved to Bazel, this PR only updates the README (and I'll send a PR for the users-guide if/when that one's accepted)

@tbroyer tbroyer changed the title Update maven-compiler-plugin to 3.5 and take advantage of annotationProcessorPaths Update Maven usage in README to suggest annotationProcessorPaths Apr 30, 2017
@ronshapiro
Copy link

LGTM. Let's put both the README and users guide changes in the same PR and then I'll grab all the changes in?

@ronshapiro
Copy link

Btw don't worry about squashing since it effectively happens when we merge internally

@tbroyer
Copy link
Author

tbroyer commented Apr 30, 2017

Let's put both the README and users guide changes in the same PR and then I'll grab all the changes in?

users guide is in a different branch ;-)

BTW, there are Gradle instructions in the README, should those be added to the users guide too? (as we're at it)

@ronshapiro
Copy link

I've always found it a little weird that we have this duplicated. I think it's probably in the best interest of our users though to have it in both locations. SGTM.

If you create both PRs, I'll take a look at both and submit. I forgot that they were different branches on github

@tbroyer
Copy link
Author

tbroyer commented May 1, 2017

See #719

The users guide was already deferring to the README for Gradle, so I removed the Maven instructions. Let me know if you'd prefer to instead duplicate all instructions in the users guide.

ronshapiro pushed a commit that referenced this pull request May 2, 2017
Also defer to the README instructions for all build tools (rather than
just Gradle)

Fixes #293
Fixes #719

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=154857182
ronshapiro pushed a commit that referenced this pull request May 2, 2017
Also defer to the README instructions for all build tools (rather than
just Gradle)

Fixes #293
Fixes #719

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=154857182
@ronshapiro ronshapiro closed this in 7497510 May 2, 2017
ronshapiro pushed a commit that referenced this pull request May 2, 2017
Also defer to the README instructions for all build tools (rather than
just Gradle)

Fixes #293
Fixes #719

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=154857182
@tbroyer tbroyer deleted the m-compiler-p-3.5 branch May 3, 2017 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants