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

Separate auto-factory compiler and annotations #1814

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

CoatedMoose
Copy link

No code changes, just moving files, adjusting POM files, and updating the README.

See issue #268.

I opened a previous version of this PR in #352, there is some discussion there as well.

No code changes, just moving files, adjusting POM files, and updating
the README.

See issue google#268.
@CoatedMoose
Copy link
Author

CoatedMoose commented Aug 1, 2024

@cpovirk commented:

...

It seems entirely likely that we will continue to drag on doing anything about it—I wonder if we were concerned that users would need to update their build configuration when they upgrade?—but it's a highly legitimate request, and it would obviously be the right approach if we were starting from scratch.

I do not think build configuration updates are required, just recommended. The name of the "processor" package uses the same artifact identifier as the (pre-PR) "mono-package", and the new "annotations" package is a dependency of the "processor" package. None of the (java) package names have changed, so updating to the latest should be invisible.

If we look at the configuration recommended in the README "Getting Started" section for the service package (which I mirrored to this PR), there is an alternative configuration.

Alternatively, you can include the processor itself (which transitively depends on the annotation) in your compile-time classpath. (However, note that doing so may pull unnecessary classes into your runtime classpath.)

<dependencies>
  <dependency>
    <groupId>com.google.auto.factory</groupId>
    <artifactId>auto-factory</artifactId>
    <version>${version}</version>
    <optional>true</optional>
  </dependency>
</dependencies>

Which is the same as the current recommended install method for this (factory) sub-project.

In a Maven project, one would include the auto-factory artifact as an "optional" dependency:

<dependencies>
  <dependency>
    <groupId>com.google.auto.factory</groupId>
    <artifactId>auto-factory</artifactId>
    <version>${version}</version>
    <optional>true</optional>
  </dependency>
</dependencies>

Copy link
Member

@cpovirk cpovirk left a comment

Choose a reason for hiding this comment

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

Thank you!


Alternatively, you can include the processor itself (which transitively depends
on the annotation) in your compile-time classpath. (However, note that doing so
may pull unnecessary classes into your runtime classpath.)
Copy link
Member

Choose a reason for hiding this comment

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

I see that this matches https://github.com/google/auto/blob/main/value/userguide/index.md#with-maven, so that's fine for this PR. I'm just noting for the record that we should probably revisit both in the future.

My reasoning: This approach will also not work if you already have something else in annotationProcessorPaths, and it won't work in upcoming JDKs unless you pass more flags (It's possible that Maven specifically will pass those options if it doesn't already, but I also note that Gradle has been pushing users away from the classpath approach, too.)

@@ -19,19 +18,24 @@
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd">
<modelVersion>4.0.0</modelVersion>

<parent>
<groupId>org.sonatype.oss</groupId>
<artifactId>oss-parent</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that factory uses this yet except in its integration tests. Are we able to avoid doing so here, even if it requires pulling more plugin configuration into the aggregator or annotations build (like what you already have in processor—and see my comment about whether processor should have aggregator as its parent)?

I say this because we've had trouble with oss-parent in the past. (It's deprecated, too, probably to reflect that it's not being kept up to date with plugin versions.)

<configuration>
<!-- disable processing because the definition in META-INF/services breaks javac -->
<compilerArgument>-proc:none</compilerArgument>
<source>1.8</source>
Copy link
Member

Choose a reason for hiding this comment

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

Do we need source and target here, or do we inherit them from the aggregator parent pom? If we need them, maybe we can still use java.version?

<module>processor</module>
</modules>

<dependencyManagement>
Copy link
Member

Choose a reason for hiding this comment

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

It looks like probably few if any of these are used by annotations—which looks like it might be the only pom.xml that uses auto-factory-aggregator as its parent.

  • Should the processor also use auto-factory-aggregator as its parent?
  • If not, do we need any of these dependencies here? (And could the plugin configuration be pushed down into the annotations build?)
  • If so, might it still make sense to push the dependencies down into processor if that's the only place that they're used?

Basically, I can understand emptying the aggregator our (as was done in https://github.com/google/auto/pull/889/files, which I see we've also neglected...) and putting everything that each subproject needs in that subproject, and I can understand putting the intersection of what the two subprojects need into the aggregator, and I can probably understand putting the union of what the two subprojects need into the aggregator. Currently, though, we use the aggregator to configure only for annotations, and we probably configure more than we need.

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.

3 participants