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

chore: consolidate build configuration to parent #8637

Merged
merged 8 commits into from
Oct 20, 2022

Conversation

meltsufin
Copy link
Member

@meltsufin meltsufin commented Oct 19, 2022

Moving this block from jar modules to jar-parent.

  <build>
    <plugins>
      <plugin>
          <groupId>org.codehaus.mojo</groupId>
          <artifactId>flatten-maven-plugin</artifactId>
        </plugin>
    </plugins>
  </build>

Removing this block from module parents. Since dependencies.sh is not run at all, it's not clear if these exclusions are still needed.

  <build>
    <pluginManagement>
      <plugins>
        <plugin>
          <groupId>org.apache.maven.plugins</groupId>
          <artifactId>maven-dependency-plugin</artifactId>
          <configuration>
            <ignoredUnusedDeclaredDependencies>
              <ignoredUnusedDeclaredDependency>org.objenesis:objenesis</ignoredUnusedDeclaredDependency>
              <ignoredUnusedDeclaredDependency>javax.annotation:javax.annotation-api</ignoredUnusedDeclaredDependency>
            </ignoredUnusedDeclaredDependencies>
          </configuration>
        </plugin>
      </plugins>
    </pluginManagement>
  </build>

Removing javax.annotation declaration for Java 9+, since the dependency is already present transitively through api-common.

  <profiles>
    <profile>
      <id>java9</id>
      <activation>
        <jdk>[9,)</jdk>
      </activation>
      <dependencies>
        <dependency>
          <groupId>javax.annotation</groupId>
          <artifactId>javax.annotation-api</artifactId>
        </dependency>
      </dependencies>
    </profile>
  </profiles>

@meltsufin meltsufin requested a review from suztomo October 19, 2022 03:22
@suztomo
Copy link
Member

suztomo commented Oct 19, 2022

Removing javax.annotation declaration for Java 9+, since the dependency is already present transitively through api-common.

It's a good practice to declare the dependency if a project touches a class from it, without relying on transitive dependencies ("Undeclared but used"). Can you move the javax.annotation part to google-cloud-jar-parent?

@meltsufin
Copy link
Member Author

Removing javax.annotation declaration for Java 9+, since the dependency is already present transitively through api-common.

It's a good practice to declare the dependency if a project touches a class from it, without relying on transitive dependencies ("Undeclared but used"). Can you move the javax.annotation part to google-cloud-jar-parent?

Since we flatten the poms, I guess it will be declared. Declaring and not using a dependency is also a bad practice. So, I'm torn on this one.

Co-authored-by: Tomo Suzuki <[email protected]>
@meltsufin meltsufin requested a review from suztomo October 19, 2022 23:24
@suztomo
Copy link
Member

suztomo commented Oct 20, 2022

Since we flatten the poms, I guess it will be declared.

That's correct, it will be declared anyway. However, skipping dependency declaration (for the flatten plugin's effect) means we can/should remove lots of other dependencies as well. The reasoning would become complex. For example, you had to reason "javax-annotation dependency is already present transitively through api-common". I don't want people spending time on such dependency analysis to decide whether a dependency should be declared. Therefore, I think we should reason about dependency declaration without considering the effect of the flatten plugin.

<jdk>[9,)</jdk>
</activation>
<dependencies>
<!-- needed for @Generated annotation at compile-time -->
Copy link
Member

Choose a reason for hiding this comment

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

Nice survey.

suztomo@suztomo2:~/google-cloud-java$ grep -ir 'import javax.annotation' . |grep -v Generated
./README.md:import javax.annotation.Nullable;
suztomo@suztomo2:~/google-cloud-java$ 

@meltsufin meltsufin added the automerge Merge the pull request once unit tests and other checks pass. label Oct 20, 2022
@gcf-merge-on-green gcf-merge-on-green bot merged commit c62d6d6 into main Oct 20, 2022
@gcf-merge-on-green gcf-merge-on-green bot deleted the more-consolidation branch October 20, 2022 14:58
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Oct 20, 2022
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

Successfully merging this pull request may close these issues.

2 participants