Clean up http-client changes (#295)

* pkl-excutor tests: symlink 0.25.0 distribution into pkl-executable/build
* Use `IoUtils.getPklHomeDir` in HttpClientBuilder
* Simplify Exceptions.java
* Enable testing for older pkl-executor distribution

pkl-core/src/main/java/org/pkl/core/http/HttpClientBuilder.java

@@ -26,24 +26,26 @@ import java.util.List;
 import java.util.function.Supplier;
 import org.pkl.core.Release;
 import org.pkl.core.util.ErrorMessages;
+import org.pkl.core.util.IoUtils;
 
 final class HttpClientBuilder implements HttpClient.Builder {
-  private final Path userHome;
   private String userAgent;
   private Duration connectTimeout = Duration.ofSeconds(60);
   private Duration requestTimeout = Duration.ofSeconds(60);
+  private final Path caCertsDir;
   private final List<Path> certificateFiles = new ArrayList<>();
   private final List<URI> certificateUris = new ArrayList<>();
 
   HttpClientBuilder() {
-    this(Path.of(System.getProperty("user.home")));
+    this(IoUtils.getPklHomeDir().resolve("cacerts"));
   }
 
   // only exists for testing
-  HttpClientBuilder(Path userHome) {
-    this.userHome = userHome;
+  HttpClientBuilder(Path caCertsDir) {
     var release = Release.current();
-    userAgent = "Pkl/" + release.version() + " (" + release.os() + "; " + release.flavor() + ")";
+    this.caCertsDir = caCertsDir;
+    this.userAgent =
+        "Pkl/" + release.version() + " (" + release.os() + "; " + release.flavor() + ")";
   }
 
   public HttpClient.Builder setUserAgent(String userAgent) {
@@ -80,10 +82,9 @@ final class HttpClientBuilder implements HttpClient.Builder {
   }
 
   public HttpClient.Builder addDefaultCliCertificates() {
-    var directory = userHome.resolve(".pkl").resolve("cacerts");
     var fileCount = certificateFiles.size();
-    if (Files.isDirectory(directory)) {
-      try (var files = Files.list(directory)) {
+    if (Files.isDirectory(caCertsDir)) {
+      try (var files = Files.list(caCertsDir)) {
         files.filter(Files::isRegularFile).forEach(certificateFiles::add);
       } catch (IOException e) {
         throw new HttpClientInitException(e);

pkl-core/src/main/java/org/pkl/core/service/ExecutorSpiImpl.java

@@ -132,11 +132,18 @@ public class ExecutorSpiImpl implements ExecutorSpi {
   private HttpClient getOrCreateHttpClient(ExecutorSpiOptions options) {
     List<Path> certificateFiles;
     List<URI> certificateUris;
-    if (options instanceof ExecutorSpiOptions2) {
-      var options2 = (ExecutorSpiOptions2) options;
-      certificateFiles = options2.getCertificateFiles();
-      certificateUris = options2.getCertificateUris();
-    } else {
+    try {
+      if (options instanceof ExecutorSpiOptions2) {
+        var options2 = (ExecutorSpiOptions2) options;
+        certificateFiles = options2.getCertificateFiles();
+        certificateUris = options2.getCertificateUris();
+      } else {
+        certificateFiles = List.of();
+        certificateUris = List.of();
+      }
+      // host pkl-executor does not have class ExecutorOptions2 defined.
+      // this will happen if the pkl-executor distribution is too old.
+    } catch (NoClassDefFoundError e) {
       certificateFiles = List.of();
       certificateUris = List.of();
     }

pkl-core/src/main/java/org/pkl/core/util/Exceptions.java

@@ -20,10 +20,8 @@ public final class Exceptions {
 
   public static Throwable getRootCause(Throwable t) {
     var result = t;
-    var cause = result.getCause();
-    while (cause != null) {
-      result = cause;
-      cause = cause.getCause();
+    while (result.getCause() != null) {
+      result = result.getCause();
     }
     return result;
   }

pkl-core/src/test/kotlin/org/pkl/core/http/HttpClientTest.kt

@@ -22,42 +22,42 @@ class HttpClientTest {
     val client = assertDoesNotThrow {
       HttpClient.builder().build()
     }
-    
+
     assertThat(client).isInstanceOf(RequestRewritingClient::class.java)
     client as RequestRewritingClient
-    
+
     val release = Release.current()
     assertThat(client.userAgent).isEqualTo("Pkl/${release.version()} (${release.os()}; ${release.flavor()})")
     assertThat(client.requestTimeout).isEqualTo(Duration.ofSeconds(60))
-    
+
     assertThat(client.delegate).isInstanceOf(JdkHttpClient::class.java)
     val delegate = client.delegate as JdkHttpClient
-    
+
     assertThat(delegate.underlying.connectTimeout()).hasValue(Duration.ofSeconds(60))
   }
-  
+
   @Test
   fun `can build custom client`() {
     val client = HttpClient.builder()
       .setUserAgent("Agent 1")
       .setRequestTimeout(Duration.ofHours(86))
-      .setConnectTimeout(Duration.ofMinutes(42))  
+      .setConnectTimeout(Duration.ofMinutes(42))
       .build() as RequestRewritingClient
-    
+
     assertThat(client.userAgent).isEqualTo("Agent 1")
     assertThat(client.requestTimeout).isEqualTo(Duration.ofHours(86))
 
     val delegate = client.delegate as JdkHttpClient
     assertThat(delegate.underlying.connectTimeout()).hasValue(Duration.ofMinutes(42))
   }
-  
+
   @Test
   fun `can load certificates from file system`() {
     assertDoesNotThrow {
       HttpClient.builder().addCertificates(FileTestUtils.selfSignedCertificate).build()
     }
   }
-  
+
   @Test
   fun `certificate file located on file system cannot be empty`(@TempDir tempDir: Path) {
     val file = tempDir.resolve("certs.pem").createFile()
@@ -75,7 +75,7 @@ class HttpClientTest {
       HttpClient.builder().addCertificates(javaClass.getResource("/org/pkl/certs/PklCARoots.pem")!!.toURI()).build()
     }
   }
-  
+
   @Test
   fun `only allows loading jar and file certificate URIs`() {
     assertThrows<HttpClientInitException> {
@@ -96,46 +96,47 @@ class HttpClientTest {
 
   @Test
   fun `can load built-in certificates`() {
-    assertDoesNotThrow {  
+    assertDoesNotThrow {
       HttpClient.builder().addBuiltInCertificates().build()
     }
   }
-  
+
   @Test
   fun `can load certificates from Pkl user home cacerts directory`(@TempDir tempDir: Path) {
-    val certFile = tempDir.resolve(".pkl")
+    val certsDir = tempDir.resolve(".pkl")
       .resolve("cacerts")
       .createDirectories()
-      .resolve("certs.pem")
-    FileTestUtils.selfSignedCertificate.copyTo(certFile)
-    
+      .also { dir ->
+        FileTestUtils.selfSignedCertificate.copyTo(dir.resolve("certs.pem"))
+      }
+
     assertDoesNotThrow {
-      HttpClientBuilder(tempDir).addDefaultCliCertificates().build()
+      HttpClientBuilder(certsDir).addDefaultCliCertificates().build()
     }
   }
-  
+
   @Test
-  fun `loading certificates from cacerts directory falls back to built-in certificates`(@TempDir userHome: Path) {
+  fun `loading certificates from cacerts directory falls back to built-in certificates`(@TempDir certsDir: Path) {
     assertDoesNotThrow {
-      HttpClientBuilder(userHome).addDefaultCliCertificates().build()
+      HttpClientBuilder(certsDir).addDefaultCliCertificates().build()
     }
   }
-  
+
   @Test
   fun `can be closed multiple times`() {
     val client = HttpClient.builder().build()
-    
+
     assertDoesNotThrow { 
       client.close() 
       client.close() 
     }
   }
-  
+
   @Test
   fun `refuses to send messages once closed`() {
     val client = HttpClient.builder().build()
     val request = HttpRequest.newBuilder(URI("https://example.com")).build()
-    
+
     client.close()
 
     assertThrows<IllegalStateException> {

pkl-core/src/test/kotlin/org/pkl/core/util/ExceptionsTest.kt

@@ -11,7 +11,7 @@ class ExceptionsTest {
     val e = IOException("io")
     assertThat(Exceptions.getRootCause(e)).isSameAs(e)
   }
-  
+
   @Test
   fun `get root cause of nested exception`() {
     val e = IOException("io")
@@ -29,7 +29,7 @@ class ExceptionsTest {
     e.initCause(e2)
     assertThat(Exceptions.getRootReason(e)).isEqualTo("the root reason")
   }
-  
+
   @Test
   fun `get root reason if null`() {
     val e = IOException("io")

pkl-executor/pkl-executor.gradle.kts

@@ -1,3 +1,5 @@
+import java.nio.file.Files
+
 plugins {
   pklAllProjects
   pklJavaLibrary
@@ -6,7 +8,7 @@ plugins {
 }
 
 val pklDistributionCurrent: Configuration by configurations.creating
-val pklDistribution025: Configuration by configurations.creating
+val pklHistoricalDistributions: Configuration by configurations.creating
 
 // Because pkl-executor doesn't depend on other Pkl modules
 // (nor has overlapping dependencies that could cause a version conflict),
@@ -15,7 +17,7 @@ val pklDistribution025: Configuration by configurations.creating
 dependencies {
   pklDistributionCurrent(project(":pkl-config-java", "fatJar"))
   @Suppress("UnstableApiUsage")
-  pklDistribution025(libs.pklConfigJavaAll025)
+  pklHistoricalDistributions(libs.pklConfigJavaAll025)
 
   implementation(libs.slf4jApi)
 
@@ -52,14 +54,29 @@ sourceSets {
   }
 }
 
-// this task could be folded into tasks.test by switching to IntelliJ's Gradle test runner
+val prepareHistoricalDistributions by tasks.registering {
+  val outputDir = layout.buildDirectory.dir("pklHistoricalDistributions")
+  inputs.files(pklHistoricalDistributions.files())
+  outputs.dir(outputDir)
+  doLast {
+    val distributionDir = outputDir.get().asFile.toPath()
+      .also(Files::createDirectories)
+    for (file in pklHistoricalDistributions.files) {
+      val link = distributionDir.resolve(file.name)
+      if (!Files.isSymbolicLink(link)) {
+        if (Files.exists(link)) {
+          Files.delete(link)
+        }
+        Files.createSymbolicLink(link, file.toPath())
+      }
+    }
+  }
+}
+
 val prepareTest by tasks.registering {
-  // used by EmbeddedExecutorTest
-  dependsOn(pklDistributionCurrent, pklDistribution025)
+  dependsOn(pklDistributionCurrent, prepareHistoricalDistributions)
 }
 
 tasks.test {
   dependsOn(prepareTest)
-  systemProperty("pklDistributionCurrent", pklDistributionCurrent.singleFile)
-  systemProperty("pklDistribution025", pklDistribution025.singleFile)
 }

pkl-executor/src/test/kotlin/org/pkl/executor/EmbeddedExecutorTest.kt

@@ -39,7 +39,7 @@ class EmbeddedExecutorTest {
 
         // This context has a pkl-executor version that is lower than the distribution version.
         // It can be enabled once there is a distribution that includes pkl-executor.
-        //ExecutionContext(executor1_2.value, ::convertToOptions1, "Options1, Executor1, Distribution2"),
+        ExecutionContext(executor1_2.value, ::convertToOptions1, "Options1, Executor1, Distribution2"),
 
         ExecutionContext(executor2_1.value, ::convertToOptions1, "Options1, Executor2, Distribution1"),
         ExecutionContext(executor2_1.value, ::convertToOptions2, "Options2, Executor2, Distribution1"),
@@ -70,7 +70,7 @@ class EmbeddedExecutorTest {
     }
 
     // A pkl-executor library that supports ExecutorSpiOptions up to v2
-    // and a Pkl distribution that supports ExecutorSpiOptions up to v.
+    // and a Pkl distribution that supports ExecutorSpiOptions up to v2.
     private val executor2_2: Lazy<Executor> = lazy {
       EmbeddedExecutor(listOf(pklDistribution2), pklExecutorClassLoader2)
     }
@@ -98,28 +98,22 @@ class EmbeddedExecutorTest {
         if (executor.isInitialized()) executor.value.close()
       }
     }
-    
+
     // a Pkl distribution that supports ExecutorSpiOptions up to v1
     private val pklDistribution1: Path by lazy {
-      val path = System.getProperty("pklDistribution025")?.toPath() ?:
-        // can get rid of this path by switching to IntelliJ's Gradle test runner
-        System.getProperty("user.home").toPath()
-          .resolve(".gradle/caches/modules-2/files-2.1/org.pkl-lang/pkl-config-java-all/" +
-            "0.25.0/e9451dda554f1659e49ff5bdd30accd26be7bf0f/pkl-config-java-all-0.25.0.jar")
-      path.apply {
-          if (!exists()) throw AssertionError("Missing test fixture. " +
+      FileTestUtils.rootProjectDir.resolve("pkl-executor/build/pklHistoricalDistributions/pkl-config-java-all-0.25.0.jar").apply {
+        if (!exists()) {
+          throw AssertionError("Missing test fixture. " +
             "To fix this problem, run `./gradlew :pkl-executor:prepareTest`.")
         }
+      }
     }
 
     // a Pkl distribution that supports ExecutorSpiOptions up to v2
     private val pklDistribution2: Path by lazy {
-      val path = System.getProperty("pklDistributionCurrent")?.toPath() ?:
-        // can get rid of this path by switching to IntelliJ's Gradle test runner
-        FileTestUtils.rootProjectDir
-          .resolve("pkl-config-java/build/libs/pkl-config-java-all-" +
-            "${Release.current().version().withBuild(null).toString().replaceFirst("dev", "SNAPSHOT")}.jar")
-      path.apply {
+      FileTestUtils.rootProjectDir
+        .resolve("pkl-config-java/build/libs/pkl-config-java-all-" +
+          "${Release.current().version().withBuild(null).toString().replaceFirst("dev", "SNAPSHOT")}.jar").apply {
         if (!exists()) throw AssertionError("Missing test fixture. " +
           "To fix this problem, run `./gradlew :pkl-executor:prepareTest`.")
       }