From a7f4531bfaf4e7ca93fea518b5c1734cd6ffdc1a Mon Sep 17 00:00:00 2001
From: Elango Cheran <elango@unicode.org>
Date: Tue, 17 Jan 2023 19:13:01 +0000
Subject: [PATCH] ICU-12811 Fix localespi tests when run by Maven on Java 8

See #2283
---
 .github/workflows/icu_ci.yml                  | 22 ++++--
 .../maven-build/maven-icu4j-localespi/pom.xml | 77 +++++++++++++++++--
 icu4j/maven-build/pom.xml                     | 31 ++++++++
 3 files changed, 116 insertions(+), 14 deletions(-)

diff --git a/.github/workflows/icu_ci.yml b/.github/workflows/icu_ci.yml
index f83c9b7e486..1f16614d39e 100644
--- a/.github/workflows/icu_ci.yml
+++ b/.github/workflows/icu_ci.yml
@@ -60,13 +60,17 @@ jobs:
 
   # Run `test` to execute unit tests and ensure it is possible for tests to run even without packaging submodule
   # dependencies as jar files
-  java8-icu4j-test-maven:
-    name: Run unit tests with Maven using JDK 8
+  icu4j-test-maven:
+    name: Run unit tests with Maven for Java version
     runs-on: ubuntu-latest
     # Make this unit test target job depend on a later phase target job to prevent race condition when
     # trying to persist the Maven cache to the Github cache, knowing that artifacts needed for
     # the later phase `verify` are a superset of the artifacts needed for the earlier phase `test`.
-    needs: java8-icu4j-verify-maven
+    needs: icu4j-verify-maven
+    strategy:
+      fail-fast: false
+      matrix:
+        java-version: [ '8', '11' ]
     steps:
       - name: Checkout and setup
         uses: actions/checkout@v2
@@ -77,7 +81,7 @@ jobs:
       - uses: actions/setup-java@v3
         with:
           distribution: 'temurin'
-          java-version: '8'
+          java-version: ${{ matrix.java-version }}
           cache: maven
       - name: Run Maven test
         run: |
@@ -85,9 +89,13 @@ jobs:
           mvn --batch-mode test
 
   # Run `verify` to ensure that `package` (creating .jar files) and `integration-test` (special setup for localespi tests) work
-  java8-icu4j-verify-maven:
-    name: Run integration tests with Maven using JDK 8
+  icu4j-verify-maven:
+    name: Run integration tests with Maven for Java version
     runs-on: ubuntu-latest
+    strategy:
+      fail-fast: false
+      matrix:
+        java-version: [ '8', '11' ]
     steps:
       - name: Checkout and setup
         uses: actions/checkout@v2
@@ -98,7 +106,7 @@ jobs:
       - uses: actions/setup-java@v3
         with:
           distribution: 'temurin'
-          java-version: '8'
+          java-version: ${{ matrix.java-version }}
           cache: maven
       # The Maven `verify` phase causes the following to happen first, and in order:
       # build/compile (`compile`), unit tests (`test`), Jar building (`package`),
diff --git a/icu4j/maven-build/maven-icu4j-localespi/pom.xml b/icu4j/maven-build/maven-icu4j-localespi/pom.xml
index bccecd08527..70b62fdd494 100644
--- a/icu4j/maven-build/maven-icu4j-localespi/pom.xml
+++ b/icu4j/maven-build/maven-icu4j-localespi/pom.xml
@@ -130,6 +130,49 @@
         </configuration>
       </plugin>
 
+      <!-- 
+        Mainly for running localespi tests in Java 8:
+        The easiest way to get the integration tests to pass (in particular, to get the JVM that is
+        spawned with the `-Djava.ext.dirs` system property value to be interpreted properly in order 
+        for the extensions jars therein to be loaded correctly) was to copy the jars needed to a
+        separate directory. (The alternative of pointing to the build directories of the respective
+        submodules didn't seem to work.)
+
+        This isn't of much use for Java 9+, in which extensions are deprecated and the regular
+        classpath is to be used instead.
+      -->
+      <plugin>
+        <groupId>org.apache.maven.plugins</groupId>
+        <artifactId>maven-dependency-plugin</artifactId>
+        <executions>
+          <execution>
+            <id>it-test-copy-localespi-extension-jars</id>
+            <phase>integration-test</phase>
+            <goals>
+              <goal>copy</goal>
+            </goals>
+            <configuration>
+              <artifactItems>
+                <artifactItem>
+                  <groupId>${project.groupId}</groupId>
+                  <artifactId>icu4j</artifactId>
+                  <version>${project.version}</version>
+                  <type>${project.packaging}</type>
+                </artifactItem>
+                <artifactItem>
+                  <groupId>${project.groupId}</groupId>
+                  <artifactId>icu4j-localespi</artifactId>
+                  <version>${project.version}</version>
+                  <type>${project.packaging}</type>
+                </artifactItem>
+              </artifactItems>
+              <!-- https://stackoverflow.com/questions/36181371/how-can-i-get-the-temp-folder-of-a-machine-running-maven -->
+              <outputDirectory>${java.io.tmpdir}/ext-test-jars</outputDirectory>
+            </configuration>
+          </execution>
+        </executions>
+      </plugin>
+
       <plugin>
         <groupId>org.apache.maven.plugins</groupId>
         <artifactId>maven-failsafe-plugin</artifactId>
@@ -150,9 +193,17 @@
             <include>**/*ITCase.java</include>
           </includes>
 
-          <!-- Set up the locale service provider using the .jar file of `icu4j-localespi` main code from the `package` phase -->
-          <systemPropertyVariables>
-            <!-- 
+          <!-- 
+            For Java 8:
+            Set up the locale service provider using the .jar file of `icu4j-localespi` main code from the `package` phase
+
+            Leave empty for Java 9+.
+            Using the `java.ext.dirs` special Java system property on Java 9+ runtimes triggers an
+             error that tells the user to place those jars on the classpath instead.
+          -->
+          <!-- 
+              Notes:
+
               https://stackoverflow.com/a/5039973/2077918
               The `java.ext.dirs` is a special Java system property that activates the Java extension mechanism.
               The Java extension mechanism was deprecated in Java 8 (users are recommended to use `-classpath` instead),
@@ -160,12 +211,24 @@
               For backwards compatibility testing for users of the Java extension mechansim,
               this configuration achieves the effect of having the localespi code in a .jar file that gets loaded
               by running this test in a phase following the `package` phase in which the .jar file is created.
-            -->
-            <java.ext.dirs>${project.build.directory}</java.ext.dirs>
-            <!--
+
               https://stackoverflow.com/questions/45223908/why-does-the-java-extension-mechanism-not-check-the-classpath-for-an-optional-pa
+
+              For some reason, the plugin was effectively not honoring the values as expected that were set in
+              <systemPropertyVariables>, so instead, the <argLine> value was used to set the JVM options. This
+              approach was inspired by: https://stackoverflow.com/a/48213614/2077918
+          -->
+          <argLine>${localespi-tests.jvm.args}</argLine>
+
+          <!-- 
+              For Java 9+:
+              Configure to use the locale service provider using the .jar file of `icu4j-localespi` main code from the `package` phase
+
+              Leave empty for Java 8 to take defaults ("SPI,JRE").
+              See: https://stackoverflow.com/questions/45223908/why-does-the-java-extension-mechanism-not-check-the-classpath-for-an-optional-pa
             -->
-            <java.locale.providers>SPI,JRE</java.locale.providers>
+          <systemPropertyVariables>
+            <java.locale.providers>${localespi-tests.locale-providers}</java.locale.providers>
           </systemPropertyVariables>
 
         </configuration>
diff --git a/icu4j/maven-build/pom.xml b/icu4j/maven-build/pom.xml
index 3c15b16926f..f1f0496de0a 100644
--- a/icu4j/maven-build/pom.xml
+++ b/icu4j/maven-build/pom.xml
@@ -148,6 +148,37 @@
     </snapshotRepository>
   </distributionManagement>
 
+  <profiles>
+    <!-- 
+      For testing localespi using the Java extensions feature, which last existed in Java 8.
+      ICU4J currently has a minimum supported version of 8.
+    -->
+    <profile>
+      <id>localespi-tests-java8</id>
+      <activation>
+        <jdk>(,8]</jdk>
+      </activation>
+      <properties>
+        <localespi-tests.jvm.args>-Djava.ext.dirs="${java.io.tmpdir}/ext-test-jars"</localespi-tests.jvm.args>
+        <localespi-tests.locale-providers></localespi-tests.locale-providers>
+      </properties>
+    </profile>
+    <!--
+      For testing localespi using the classpath for Java 9+, since Java extensions were
+      deprecated after Java 8.
+    -->
+    <profile>
+      <id>localespi-tests-java9</id>
+      <activation>
+        <jdk>[9,)</jdk>
+      </activation>
+      <properties>
+        <localespi-tests.jvm.args></localespi-tests.jvm.args>
+        <localespi-tests.locale-providers>CLDR,COMPAT,SPI</localespi-tests.locale-providers>
+      </properties>
+    </profile>
+  </profiles>
+
   <properties>
     <!-- 
       Main ICU4J version number.