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

Moe Sync #606

Merged
merged 1 commit into from Mar 12, 2018
Merged

Moe Sync #606

merged 1 commit into from Mar 12, 2018

Conversation

ronshapiro
Copy link
Contributor

This code has been reviewed and submitted internally. Feel free to discuss on the PR and we can submit follow-up changes as necessary.

Commits:

Create an auto-value-annotations artifact separate from the processor

Fixes #268

I followed the instructions in https://maven.apache.org/guides/mini/guide-using-one-source-directory.html because it seemed like the easiest way forward without moving around any files.

RELNOTES=@AutoValue, @AutoAnnotation, @AutoOneOf, and @Memoized are now in a separate artifact, auto-value-annotations. This allows users to specify the annotations in compile scope and the processor in an annotation processing scope, without leaking the processor to a release binary. To upgrade to this version of auto-value, you'll need to add this new artifact as a dependency.

eb7b4ae

Fixes #268

I followed the instructions in https://maven.apache.org/guides/mini/guide-using-one-source-directory.html because it seemed like the easiest way forward without moving around any files.

RELNOTES=`@AutoValue`, `@AutoAnnotation`, `@AutoOneOf`, and `@Memoized` are now in a separate artifact, `auto-value-annotations`. This allows users to specify the annotations in compile scope and the processor in an annotation processing scope, without leaking the processor to a release binary. To upgrade to this version of auto-value, you'll need to add this new artifact as a dependency.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=188505001
@ronshapiro ronshapiro merged commit 0c488d7 into master Mar 12, 2018
@ronshapiro ronshapiro deleted the sync-master-2018/03/12 branch March 12, 2018 20:29
@@ -74,21 +74,31 @@ examples short and simple.
Maven users should add the following to the project's `pom.xml` file:

```xml
<dependency>
<groupId>com.google.auto.value</groupId>
<artifactId>auto-value-annotations</artifactId>

Choose a reason for hiding this comment

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

unless the annotations are now runtime retention... is it really necessary to add this as a strict dep to maven projects?

Copy link
Member

Choose a reason for hiding this comment

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

The annotations (at least most of them) now have CLASS retention, so it isn't completely pointless to have them at runtime. I suppose we could tell people that they can give this dependency provided scope if they want, but is it worthwhile for such a tiny dependency?

Copy link
Member

Choose a reason for hiding this comment

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

FYI: Guava (and, I recently learned from jbduncan, JUnit 5) have stopped using provided scope for annotation dependencies after various discussions of problems. In short, provided doesn't mean "compile-time" as you'd probably hope; instead, it means "compile-time for this project and missing entirely in downstream compilations." This causes problems for people who enable all javac warnings or use some annotation processors.

Copy link
Member

Choose a reason for hiding this comment

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

We should probably add a note in index.md suggesting that people not use provided scope, then, with your link.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, then the Gradle snippet should probably use api (for java-library and Android projects) or compile (for all others).

Though this actually depends whether your autovalues are part of your public API or not.

BTW, apt should be updated to annotationProcessor, and with Gradle 4.6 the net.ltgt.apt plugin is no longer required (though it still provides useful features).

So how about:

Gradle users should use the following in their build.gradle script:

dependencies {
  // Use 'api' rather than 'compile' for Android or java-library projects.
  compile             "com.google.auto.value:auto-value-annotations:1.6"
  annotationProcessor "com.google.auto.value:auto-value:1.6"
}

You may want, or have to, install the net.ltgt.apt plugin in non-Android projects.

Note that I updated the link to my plugin to point to the GitHub repo where the README provides documentation, including about companion plugins for better IDE support, whereas the Gradle Plugin Portal only has code snippets (I've had a couple reports already from people who hadn't seen the companion plugins, coming from the Plugin Portal).

And in the note you suggest to add about <scope>provided</scope> (or <optional>true</optional>), add a note about the compileOnly and implementation Gradle configurations (that's the technical name in Gradle, "configurations", not "scopes").

Choose a reason for hiding this comment

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

Hm, shouldn't be compileOnly recommended for Gradle users then?

This seems like best configuration: annotations will be available to javac and annotation processors (including 3rd-party "extension" processors) of the module, but won't be included into jar nor will be transitively available to other modules.

Afaik philosophy of Auto* was to be hidden from runtime introspection by default #290 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

@artem-zinnatullin See comments above, specifically #606 (comment) and #606 (comment). The issue is about being present at compile-time (or static analysis time) of downstream projects.

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.

None yet

7 participants