Incorporate more review feedback

- default to SSLContext.getDefault() instead of built-in certificates in HttpClient
- simplify Kotlin code using `with()`
- add missing semicolon in documented User-Agent header
- shorten comment (topic was discussed in PR, stops silly ktfmt formatting)
- move built-in certificate file to new pkl-certs subproject
- use java.net.URI instead of java.net.URL for certificate files (safer)
- only allow jar: and file: certificate URIs

Author: odenix

pkl-certs/pkl-certs.gradle.kts

@@ -0,0 +1,4 @@
+plugins {
+  pklAllProjects
+  pklJavaLibrary
+}

pkl-commons-cli/src/main/resources/org/pkl/commons/cli/commands/IncludedCARoots.pem renamed to pkl-certs/src/main/resources/org/pkl/certs/PklCARoots.pem

@@ -156,7 +156,7 @@ fun Exec.configureExecutable(isEnabled: Boolean, outputFile: File, extraArgs: Li
         ,"--no-fallback"
         ,"-H:IncludeResources=org/pkl/core/stdlib/.*\\.pkl"
         ,"-H:IncludeResources=org/jline/utils/.*"
-        ,"-H:IncludeResources=org/pkl/core/http/IncludedCARoots.pem"
+        ,"-H:IncludeResources=org/pkl/certs/PklCARoots.pem"
         //,"-H:IncludeResources=org/pkl/core/Release.properties"
         ,"-H:IncludeResourceBundles=org.pkl.core.errorMessages"
         ,"--macro:truffle"

pkl-cli/pkl-cli.gradle.kts

@@ -14,6 +14,7 @@ dependencies {
 
   implementation(projects.pklCommons)
   testImplementation(projects.pklCommonsTest)
+  runtimeOnly(projects.pklCerts)
 }
 
 publishing {

pkl-commons-cli/pkl-commons-cli.gradle.kts

@@ -178,16 +178,16 @@ data class CliBaseOptions(
    * To release the resources held by the HTTP client in a timely manner, call its `close()` method.
    */
   val httpClient: HttpClient by lazy {
-    val builder = HttpClient.builder()
-    if (normalizedCaCertificates.isEmpty()) {
-      builder.addDefaultCliCertificates()
-    } else {
-      for (file in normalizedCaCertificates) builder.addCertificates(file)
+    with(HttpClient.builder()) {
+      if (normalizedCaCertificates.isEmpty()) {
+        addDefaultCliCertificates()
+      } else {
+        for (file in normalizedCaCertificates) addCertificates(file)
+      }
+      // Lazy building significantly reduces execution time of commands that do minimal work.
+      // However, it means that HTTP client initialization errors won't surface until an HTTP
+      // request is made.
+      buildLazily()
     }
-    // Lazy building significantly reduces execution time of commands that do minimal work.
-    // However, it means that HTTP client initialization errors won't surface until an HTTP request
-    // is
-    // made. A middleground would be to only build lazily if built-in certificates are used.
-    builder.buildLazily()
   }
 }

pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/CliBaseOptions.kt

@@ -13,6 +13,7 @@ dependencies {
   api(libs.junitParams)
   api(projects.pklCommons) // for convenience
   implementation(libs.assertj)
+  runtimeOnly(projects.pklCerts)
 }
 
 

pkl-commons-test/pkl-commons-test.gradle.kts

@@ -40,8 +40,8 @@ object FileTestUtils {
   }
 
   fun writePklBuiltInCertificates(dir: Path): Path {
-    val text = javaClass.getResource("/org/pkl/core/http/IncludedCARoots.pem")!!.readText()
-    return dir.resolve("IncludedCARoots.pem").apply { writeText(text) }
+    val text = javaClass.getResource("/org/pkl/certs/PklCARoots.pem")!!.readText()
+    return dir.resolve("PklCARoots.pem").apply { writeText(text) }
   }
 }
 

pkl-commons-test/src/main/kotlin/org/pkl/commons/test/FileTestUtils.kt

@@ -58,7 +58,7 @@ dependencies {
   implementation(libs.snakeYaml)
 
   testImplementation(projects.pklCommonsTest)
-
+  
   add("generatorImplementation", libs.javaPoet)
   add("generatorImplementation", libs.truffleApi)
   add("generatorImplementation", libs.kotlinStdLib)

pkl-core/pkl-core.gradle.kts

@@ -23,4 +23,8 @@ public PklException(String message, Throwable cause) {
   public PklException(String message) {
     super(message);
   }
+
+  public PklException(Throwable cause) {
+    super(cause);
+  }
 }

pkl-core/src/main/java/org/pkl/core/PklException.java

@@ -16,7 +16,7 @@
 package org.pkl.core.http;
 
 import java.io.IOException;
-import java.net.URL;
+import java.net.URI;
 import java.net.http.HttpRequest;
 import java.net.http.HttpResponse;
 import java.net.http.HttpTimeoutException;
@@ -40,7 +40,7 @@ interface Builder {
     /**
      * Sets the {@code User-Agent} header.
      *
-     * <p>Defaults to {@code "Pkl/$version ($os $flavor)"}.
+     * <p>Defaults to {@code "Pkl/$version ($os; $flavor)"}.
      */
     Builder setUserAgent(String userAgent);
 
@@ -73,10 +73,13 @@ interface Builder {
      * <p>The given file must contain <a href="https://en.wikipedia.org/wiki/X.509">X.509</a>
      * certificates in PEM format.
      *
-     * <p>This method can be used to add certificate files located on the JVM class path. To add
-     * certificate files located on the file system, use {@link #addCertificates(Path)}.
+     * <p>This method is intended to be used for adding certificate files located on the class path.
+     * To add certificate files located on the file system, use {@link #addCertificates(Path)}.
+     *
+     * @throws HttpClientInitException if the given URI has a scheme other than {@code jar:} or
+     *     {@code file:}
      */
-    Builder addCertificates(URL file);
+    Builder addCertificates(URI file);
 
     /**
      * Adds the CA certificate files in {@code ~/.pkl/cacerts/} to the client's trust store.
@@ -86,10 +89,23 @@ interface Builder {
      * {@link #addBuiltInCertificates() built-in certificates} are added instead.
      *
      * <p>This method implements the default behavior of Pkl CLIs.
+     *
+     * <p>NOTE: This method requires the optional {@code pkl-certs} JAR to be present on the class
+     * path.
+     *
+     * @throws HttpClientInitException if an I/O error occurs while scanning {@code ~/.pkl/cacerts/}
+     *     or the {@code pkl-certs} JAR is not found on the class path
      */
-    Builder addDefaultCliCertificates() throws IOException;
+    Builder addDefaultCliCertificates();
 
-    /** Adds Pkl's built-in CA certificates to the client's trust store. */
+    /**
+     * Adds Pkl's built-in CA certificates to the client's trust store.
+     *
+     * <p>NOTE: This method requires the optional {@code pkl-certs} JAR to be present on the class
+     * path.
+     *
+     * @throws HttpClientInitException if the {@code pkl-certs} JAR is not found on the class path
+     */
     Builder addBuiltInCertificates();
 
     /**
@@ -117,7 +133,8 @@ interface Builder {
    * <ul>
    *   <li>Connect timeout: 60 seconds
    *   <li>Request timeout: 60 seconds
-   *   <li>CA certificates: none (falls back to {@link Builder#addBuiltInCertificates})
+   *   <li>CA certificates: none (falls back to the JVM's {@linkplain SSLContext#getDefault()
+   *       default SSL context})
    * </ul>
    */
   static Builder builder() {