Pom refactoring #253

honnix · 2023-09-28T21:30:37Z

TL;DR

pom.xml files refactoring

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

It's a bit hard to list what is being done so I tried adding comments in the PR where applicable.

Note that in this PR we are not trying to upgrade any deps or plugin versions unless necessary.

A further upgrade and cleanup can be applied later, with the possibility introducing maven-dependency analyse.

Tracking Issue

_NA

Follow-up issue

NA

honnix · 2023-09-28T21:34:24Z

jflyte-aws/src/test/java/org/flyte/jflyte/aws/S3FileSystemIT.java


 @Testcontainers
 public class S3FileSystemIT {

  @Container
-  public final LocalStackContainer localStack = new LocalStackContainer().withServices(S3);


Deprecation.

honnix · 2023-09-28T21:34:47Z

pom.xml

@@ -98,10 +98,8 @@
    <!-- also remember to bump the version in flytekit-bom module -->
    <spotless.version>2.21.0</spotless.version>
    <spotbugs.excludeFilterFile>spotbugs-exclude.xml</spotbugs.excludeFilterFile>
-    <error_prone.version>2.19.1</error_prone.version>
-    <!-- Using the error-prone-javac is required when running on JDK 8 -->
-    <error_prone.javac.version>9+181-r4173-1</error_prone.javac.version>


This is not used anymore and we are no longer on Java 8.

honnix · 2023-09-28T21:35:16Z

pom.xml

@@ -111,6 +109,9 @@
    <maven.compiler.source>${maven.compiler.release}</maven.compiler.source>
    <maven.compiler.target>${maven.compiler.release}</maven.compiler.target>
    <mockito.version>3.3.3</mockito.version>
+    <scala212.version>2.12.17</scala212.version>


Manage these in parent so it is clearer to see what versions we are using.

honnix · 2023-09-28T21:36:30Z

pom.xml

@@ -388,23 +459,6 @@
              <!-- see https://stackoverflow.com/a/57004351 -->
              <arg>-Xpkginfo:always</arg>
            </compilerArgs>
-            <annotationProcessorPaths>


I had tons of troubles with errorprone in IntelliJ when using annotationProcessorPaths.

honnix · 2023-09-28T21:36:51Z

pom.xml

+    <dependency>
+      <groupId>com.google.errorprone</groupId>
+      <artifactId>error_prone_core</artifactId>
+      <scope>provided</scope>


This is effectively the same as using annotationProcessorPaths.

honnix · 2023-09-28T21:37:19Z

pom.xml

+            <executions>
+              <execution>
+                <goals>
+                  <goal>apply</goal>


When compiling locally, we apply.

honnix · 2023-09-28T21:37:30Z

pom.xml

+            <executions>
+              <execution>
+                <goals>
+                  <goal>check</goal>


When building in CI, we check.

honnix · 2023-09-28T21:37:49Z

jflyte-aws/pom.xml

-  <dependencyManagement>
-    <dependencies>
-      <dependency>
-        <groupId>org.testcontainers</groupId>


Now managed centrally in parent.

honnix · 2023-09-28T21:38:33Z

integration-tests/pom.xml

    </plugins>
  </build>
+  <profiles>
+    <profile>
+      <id>it</id>


IT can be very slow when running on Apple silicon, we can disable that by mvn clean verify -P \!it.

honnix · 2023-09-28T21:39:16Z

integration-tests/pom.xml

@@ -33,24 +33,6 @@
    <enforcer.skip>true</enforcer.skip>
  </properties>

-  <dependencyManagement>
-    <dependencies>
-      <dependency>


Now managed centrally in parent.

honnix · 2023-09-28T21:39:39Z

flytekit-local-engine/src/test/java/org/flyte/localengine/LocalEngineTest.java

@@ -185,6 +185,7 @@ public void testStructWorkflow() {
    assertEquals(expectedOutput, outputs.get("outputStructData"));
  }*/

+  @SuppressWarnings("LockOnNonEnclosingClassLiteral")


New errorprone rule.

honnix · 2023-09-28T21:39:56Z

flytekit-examples-scala/pom.xml

@@ -24,23 +24,47 @@
    <version>0.4.25-SNAPSHOT</version>
  </parent>

-  <artifactId>flytekit-examples-scala</artifactId>
+  <artifactId>flytekit-examples-scala_2.13</artifactId>


Renamed to reflect Scala version, as best practice.

honnix · 2023-09-29T07:26:16Z

flyteidl-protos/pom.xml

@@ -70,53 +69,11 @@
      </plugin>
      <plugin>
        <artifactId>maven-compiler-plugin</artifactId>
-        <version>3.8.1</version>
        <configuration>


This is already configured by parent.

honnix · 2023-09-29T07:26:31Z

flyteidl-protos/pom.xml

        <configuration>
-          <protocArtifact>com.google.protobuf:protoc:3.21.1:exe:${os.detected.classifier}</protocArtifact>
+          <protocArtifact>com.google.protobuf:protoc:${protobuf.version}:exe:${os.detected.classifier}</protocArtifact>


Use the version defined by parent.

honnix · 2023-09-29T07:27:52Z

.github/workflows/build.yaml

@@ -38,7 +38,7 @@ jobs:

      - name: Verify with Maven
        if: ${{ github.ref != 'refs/heads/master' }}
-        run: mvn --batch-mode verify
+        run: mvn --batch-mode verify -Pci


Enable this profile for spotless check.

honnix · 2023-09-29T07:28:19Z

.github/workflows/build.yaml

        env:
+          CI: true


Set this to true so spotless:apply will not be done during CI.

honnix · 2023-09-29T07:28:35Z

flyteidl-protos/pom.xml

        </configuration>
      </plugin>
-      <plugin>


This plugin is now globally enabled in parent.

honnix · 2023-09-29T07:29:16Z

flytekit-api/pom.xml

@@ -44,6 +44,7 @@
    <dependency>
      <groupId>com.google.errorprone</groupId>
      <artifactId>error_prone_annotations</artifactId>
+      <scope>provided</scope>


Not necessary but following other places.

honnix · 2023-09-29T07:29:46Z

flytekit-examples/pom.xml

@@ -100,9 +101,6 @@
          </execution>
        </executions>
      </plugin>
-      <plugin>


This plugin is globally enabled in the parent.

honnix · 2023-09-29T07:32:12Z

flytekit-scala-tests/pom.xml

        </executions>
      </plugin>
+      <!-- Disabling the default javadoc plugin - we use scala-maven-plugin instead -->


We have been doing this for another scala module, so we should follow.

honnix · 2023-09-29T07:32:47Z

flytekit-scala_2.12/pom.xml

      </plugin>
    </plugins>
-    <sourceDirectory>../flytekit-scala_2.13/src/main/scala</sourceDirectory>


This is unnecessary.

honnix · 2023-09-29T07:33:06Z

flytekit-scala_2.12/pom.xml

@@ -74,7 +87,8 @@
        <artifactId>scala-maven-plugin</artifactId>
        <configuration>
          <sourceDir>../flytekit-scala_2.13/src/main/scala</sourceDir>
-          <scalaVersion>${scala.version}</scalaVersion>
+          <testSourceDir>../flytekit-scala_2.13/src/test/scala</testSourceDir>


Not needed now but might save us some headache later when needed.

honnix · 2023-09-29T07:34:16Z

flytekit-scala_2.13/pom.xml

-      </plugin>
-      <plugin>
-        <artifactId>maven-source-plugin</artifactId>
+        <groupId>org.apache.maven.plugins</groupId>


honnix · 2023-09-29T07:35:00Z

integration-tests/pom.xml

    <dependency>
      <groupId>org.flyte</groupId>
      <artifactId>flytekit-examples</artifactId>
      <scope>test</scope>
    </dependency>
-    <dependency>


We are currently not using scala examples for IT.

honnix · 2023-09-29T07:35:58Z

integration-tests/pom.xml

    <dependency>
      <groupId>org.flyte</groupId>
      <artifactId>jflyte</artifactId>
      <scope>test</scope>
-      <optional>true</optional>


Not exactly sure why it is here. test scope plus optional doesn't make much sense.

honnix · 2023-09-29T07:37:19Z

jflyte-utils/pom.xml

@@ -47,20 +47,6 @@
      <groupId>org.flyte</groupId>
      <artifactId>jflyte-api</artifactId>
    </dependency>
-    <dependency>


Minior cleanup because this is not needed by jflyte-util.

honnix · 2023-09-29T07:37:29Z

jflyte-utils/pom.xml

@@ -90,13 +76,6 @@
      <artifactId>netty-tcnative-boringssl-static</artifactId>
    </dependency>

-    <!-- runtime -->
-    <dependency>


honnix · 2023-09-29T07:37:52Z

jflyte/pom.xml

@@ -171,20 +171,10 @@
      <artifactId>memoryfilesystem</artifactId>
      <scope>test</scope>
    </dependency>
-    <!-- ensure there is at least one slf4j implementation in test classpath so that we can test
-         org/slf4j/impl/StaticLoggerBinder.class is only discovered in child class loader -->
-    <dependency>


This is only needed by jflyte-utils.

honnix · 2023-09-29T07:38:13Z

pom.xml

@@ -366,7 +442,7 @@
      <plugins>
        <plugin>
          <artifactId>maven-compiler-plugin</artifactId>
-          <version>3.8.1</version>


3.8.1 is very old.

honnix · 2023-09-29T07:38:53Z

flyteidl-protos/pom.xml

@@ -52,9 +52,8 @@
      <plugin>
        <groupId>org.xolstice.maven.plugins</groupId>
        <artifactId>protobuf-maven-plugin</artifactId>
-        <version>0.6.1</version>


Now managed by parent.

honnix · 2023-09-29T07:41:02Z

pom.xml

@@ -788,23 +897,6 @@
              </execution>
            </executions>
          </plugin>
-          <plugin>


It doesn't seem to be needed to reconfigure this plugin for release profile.

honnix · 2023-09-29T09:29:42Z

pom.xml

+    module that uses @AutoService, otherwise we get warning like "The following options were not recognized by any processor: '[verify]'",
+    which again will fail the build because we have failOnWarning.
+     -->
+    <auto-service.version>1.0.2</auto-service.version>


This is the most recently version that has compatible guava deps with auto-value and error-prone. For some reason, maven could pick up a more recently guava for java compiler to use, but when building in IntelliJ we get errors that is clearly due to incompatible guava version.

Signed-off-by: Hongxin Liang <honnix@users.noreply.github.com>

honnix commented Sep 28, 2023

View reviewed changes

pom.xml

<executions>

<execution>

<goals>

<goal>apply</goal>

Copy link

Member Author

honnix Sep 28, 2023

Choose a reason for hiding this comment

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

When compiling locally, we apply.

honnix commented Sep 28, 2023

View reviewed changes

pom.xml

<executions>

<execution>

<goals>

<goal>check</goal>

Copy link

Member Author

honnix Sep 28, 2023

Choose a reason for hiding this comment

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

When building in CI, we check.

honnix commented Sep 28, 2023

View reviewed changes

honnix force-pushed the pom-refactoring branch from 6b46135 to ea36901 Compare September 29, 2023 06:27

honnix commented Sep 29, 2023

View reviewed changes

.github/workflows/build.yaml

env:

CI: true

Copy link

Member Author

honnix Sep 29, 2023

Choose a reason for hiding this comment

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

Set this to true so spotless:apply will not be done during CI.

honnix commented Sep 29, 2023

View reviewed changes

Refactor pom

f6249e9

Signed-off-by: Hongxin Liang <honnix@users.noreply.github.com>

honnix force-pushed the pom-refactoring branch from f369d0f to f6249e9 Compare September 29, 2023 09:33

honnix marked this pull request as ready for review September 29, 2023 10:03

andresgomezfrr approved these changes Sep 29, 2023

View reviewed changes

andresgomezfrr merged commit c925d99 into master Sep 29, 2023
4 checks passed

andresgomezfrr deleted the pom-refactoring branch September 29, 2023 10:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pom refactoring #253

Pom refactoring #253

honnix commented Sep 28, 2023 •

edited

Loading

honnix Sep 28, 2023

honnix Sep 28, 2023

honnix Sep 28, 2023

honnix Sep 28, 2023

honnix Sep 28, 2023

honnix Sep 28, 2023

honnix Sep 28, 2023

honnix Sep 28, 2023

honnix Sep 28, 2023

honnix Sep 28, 2023

honnix Sep 28, 2023

honnix Sep 28, 2023

honnix Sep 29, 2023

honnix Sep 29, 2023

honnix Sep 29, 2023

honnix Sep 29, 2023

honnix Sep 29, 2023 •

edited

Loading

honnix Sep 29, 2023

honnix Sep 29, 2023

honnix Sep 29, 2023

honnix Sep 29, 2023

honnix Sep 29, 2023

honnix Sep 29, 2023

honnix Sep 29, 2023

honnix Sep 29, 2023

honnix Sep 29, 2023

honnix Sep 29, 2023

honnix Sep 29, 2023

honnix Sep 29, 2023

honnix Sep 29, 2023

honnix Sep 29, 2023

honnix Sep 29, 2023

Pom refactoring #253

Pom refactoring #253

Conversation

honnix commented Sep 28, 2023 • edited Loading

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honnix Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honnix commented Sep 28, 2023 •

edited

Loading

honnix Sep 29, 2023 •

edited

Loading