Exclude non file-based modules from synthesized *GatherImports task (#821)

This fixes an issue where certain modules tasks fail due to the plugin attempting to analyze their imports, but the arguments may not actually be Pkl modules. For example, the pkldoc task accepts entire packages in its "sourceMoules" property. This changes the gather imports logic to only analyze file-based modules. This is also a performance improvement; non file-based modules are unlikely to import files due to insufficient trust levels. Also: fix a bug when generating pkldoc on Windows
2026-01-11 22:30:54 +01:00 · 2024-11-18 11:14:17 -08:00
parent 7c1604b264
commit b93cb9b322
7 changed files with 216 additions and 134 deletions
--- a/pkl-gradle/src/main/java/org/pkl/gradle/PklPlugin.java
+++ b/pkl-gradle/src/main/java/org/pkl/gradle/PklPlugin.java
@@ -18,6 +18,7 @@ package org.pkl.gradle;
 import java.io.File;
 import java.lang.reflect.InvocationTargetException;
 import java.nio.file.Files;
+import java.util.Collections;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Optional;
@@ -27,6 +28,7 @@ import org.gradle.api.GradleException;
 import org.gradle.api.NamedDomainObjectContainer;
 import org.gradle.api.Plugin;
 import org.gradle.api.Project;
+import org.gradle.api.Transformer;
 import org.gradle.api.file.SourceDirectorySet;
 import org.gradle.api.plugins.Convention;
 import org.gradle.api.provider.Provider;
@@ -64,6 +66,7 @@ import org.pkl.gradle.task.PkldocTask;
 import org.pkl.gradle.task.ProjectPackageTask;
 import org.pkl.gradle.task.ProjectResolveTask;
 import org.pkl.gradle.task.TestTask;
+import org.pkl.gradle.utils.PluginUtils;

@SuppressWarnings("unused")
 public class PklPlugin implements Plugin<Project> {
@@ -456,6 +459,9 @@ public class PklPlugin implements Plugin<Project> {

  private List<File> getTransitiveModules(AnalyzeImportsTask analyzeTask) {
    var outputFile = analyzeTask.getOutputFile().get().getAsFile().toPath();
+    if (!analyzeTask.getOnlyIf().isSatisfiedBy(analyzeTask)) {
+      return Collections.emptyList();
+    }
    try {
      var contents = Files.readString(outputFile);
      ImportGraph importGraph = ImportGraph.parseFromJson(contents);
@@ -470,9 +476,16 @@ public class PklPlugin implements Plugin<Project> {
  }

  private <T extends ModulesTask, S extends ModulesSpec> void configureModulesTask(
-      T task, S spec, @Nullable TaskProvider<AnalyzeImportsTask> analyzeImportsTask) {
+      T task,
+      S spec,
+      @Nullable TaskProvider<AnalyzeImportsTask> analyzeImportsTask,
+      @Nullable Transformer<List<?>, List<?>> mapSourceModules) {
    configureBaseTask(task, spec);
-    task.getSourceModules().set(spec.getSourceModules());
+    if (mapSourceModules != null) {
+      task.getSourceModules().set(spec.getSourceModules().map(mapSourceModules));
+    } else {
+      task.getSourceModules().set(spec.getSourceModules());
+    }
    task.getNoProject().set(spec.getNoProject());
    task.getProjectDir().set(spec.getProjectDir());
    task.getOmitProjectSettings().set(spec.getOmitProjectSettings());
@@ -484,6 +497,11 @@ public class PklPlugin implements Plugin<Project> {
    }
  }

+  private <T extends ModulesTask, S extends ModulesSpec> void configureModulesTask(
+      T task, S spec, @Nullable TaskProvider<AnalyzeImportsTask> analyzeImportsTask) {
+    configureModulesTask(task, spec, analyzeImportsTask, null);
+  }
+
  private TaskProvider<AnalyzeImportsTask> createAnalyzeImportsTask(ModulesSpec spec) {
    var outputFile =
        project
@@ -496,11 +514,26 @@ public class PklPlugin implements Plugin<Project> {
            spec.getName() + "GatherImports",
            AnalyzeImportsTask.class,
            task -> {
-              configureModulesTask(task, spec, null);
+              configureModulesTask(
+                  task,
+                  spec,
+                  null,
+                  (modules) ->
+                      // only need to analyze imports of file-based modules; it's unlikely that a
+                      // non-file-based module will import a file-based module due to security
+                      // manager trust levels (see
+                      // org.pkl.core.SecurityManagers.getDefaultTrustLevel).
+                      modules.stream()
+                          .map(PluginUtils::parseModuleNotationToUri)
+                          .filter(
+                              (it) ->
+                                  it.getScheme() == null || it.getScheme().equalsIgnoreCase("file"))
+                          .toList());
              task.setDescription("Compute the set of imports declared by input modules");
              task.setGroup("build");
              task.getOutputFormat().set(OutputFormat.JSON.toString());
              task.getOutputFile().set(outputFile);
+              task.onlyIf(ignored -> !task.getSourceModules().get().isEmpty());
            });
  }

--- a/pkl-gradle/src/main/java/org/pkl/gradle/task/BasePklTask.java
+++ b/pkl-gradle/src/main/java/org/pkl/gradle/task/BasePklTask.java
@@ -17,9 +17,6 @@ package org.pkl.gradle.task;

 import java.io.File;
 import java.net.URI;
-import java.net.URISyntaxException;
-import java.net.URL;
-import java.nio.file.InvalidPathException;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.time.Duration;
@@ -31,10 +28,8 @@ import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 import javax.inject.Inject;
 import org.gradle.api.DefaultTask;
-import org.gradle.api.InvalidUserDataException;
 import org.gradle.api.file.ConfigurableFileCollection;
 import org.gradle.api.file.DirectoryProperty;
-import org.gradle.api.file.FileSystemLocation;
 import org.gradle.api.model.ObjectFactory;
 import org.gradle.api.provider.ListProperty;
 import org.gradle.api.provider.MapProperty;
@@ -49,9 +44,9 @@ import org.gradle.api.tasks.Optional;
 import org.gradle.api.tasks.TaskAction;
 import org.pkl.commons.cli.CliBaseOptions;
 import org.pkl.core.evaluatorSettings.Color;
-import org.pkl.core.util.IoUtils;
 import org.pkl.core.util.LateInit;
 import org.pkl.core.util.Nullable;
+import org.pkl.gradle.utils.PluginUtils;

 public abstract class BasePklTask extends DefaultTask {
  @Input
@@ -74,7 +69,7 @@ public abstract class BasePklTask extends DefaultTask {

  @Internal
  public Provider<Object> getParsedSettingsModule() {
-    return getSettingsModule().map(this::parseModuleNotation);
+    return getSettingsModule().map(PluginUtils::parseModuleNotation);
  }

  @InputFile
@@ -165,7 +160,7 @@ public abstract class BasePklTask extends DefaultTask {
              parseModulePath(),
              getProject().getProjectDir().toPath(),
              mapAndGetOrNull(getEvalRootDirPath(), Paths::get),
-              mapAndGetOrNull(getSettingsModule(), this::parseModuleNotationToUri),
+              mapAndGetOrNull(getSettingsModule(), PluginUtils::parseModuleNotationToUri),
              null,
              getEvalTimeout().getOrNull(),
              mapAndGetOrNull(getModuleCacheDir(), it1 -> it1.getAsFile().toPath()),
@@ -199,100 +194,6 @@ public abstract class BasePklTask extends DefaultTask {
    return getModulePath().getFiles().stream().map(File::toPath).collect(Collectors.toList());
  }

-  /**
-   * Parses the specified source module notation into a "parsed" notation which is then used for
-   * input path tracking and as an argument for the CLI API.
-   *
-   * <p>This method accepts the following input types:
-   *
-   * <ul>
-   *   <li>{@link URI} - used as is.
-   *   <li>{@link File} - used as is.
-   *   <li>{@link Path} - converted to a {@link File}. This conversion may fail because not all
-   *       {@link Path}s point to the local file system.
-   *   <li>{@link URL} - converted to a {@link URI}. This conversion may fail because {@link URL}
-   *       allows for URLs which are not compliant URIs.
-   *   <li>{@link CharSequence} - first, converted to a string. If this string is "URI-like" (see
-   *       {@link IoUtils#isUriLike(String)}), then we attempt to parse it as a {@link URI}, which
-   *       may fail. Otherwise, we attempt to parse it as a {@link Path}, which is then converted to
-   *       a {@link File} (both of these operations may fail).
-   *   <li>{@link FileSystemLocation} - converted to a {@link File} via the {@link
-   *       FileSystemLocation#getAsFile()} method.
-   * </ul>
-   *
-   * In case the returned value is determined to be a {@link URI}, then this URI is first checked
-   * for whether its scheme is {@code file}, like {@code file:///example/path}. In such case, this
-   * method returns a {@link File} corresponding to the file path in the URI. Otherwise, a {@link
-   * URI} instance is returned.
-   *
-   * @throws InvalidUserDataException In case the input is none of the types described above, or
-   *     when the underlying value cannot be parsed correctly.
-   */
-  protected Object parseModuleNotation(Object notation) {
-    if (notation instanceof URI uri) {
-      if ("file".equals(uri.getScheme())) {
-        return new File(uri.getPath());
-      }
-      return uri;
-    } else if (notation instanceof File) {
-      return notation;
-    } else if (notation instanceof Path path) {
-      try {
-        return path.toFile();
-      } catch (UnsupportedOperationException e) {
-        throw new InvalidUserDataException("Failed to parse Pkl module file path: " + notation, e);
-      }
-    } else if (notation instanceof URL url) {
-      try {
-        return parseModuleNotation(url.toURI());
-      } catch (URISyntaxException e) {
-        throw new InvalidUserDataException("Failed to parse Pkl module URI: " + notation, e);
-      }
-    } else if (notation instanceof CharSequence) {
-      var s = notation.toString();
-      if (IoUtils.isUriLike(s)) {
-        try {
-          return parseModuleNotation(IoUtils.toUri(s));
-        } catch (URISyntaxException e) {
-          throw new InvalidUserDataException("Failed to parse Pkl module URI: " + s, e);
-        }
-      } else {
-        try {
-          return Paths.get(s).toFile();
-        } catch (InvalidPathException | UnsupportedOperationException e) {
-          throw new InvalidUserDataException("Failed to parse Pkl module file path: " + s, e);
-        }
-      }
-    } else if (notation instanceof FileSystemLocation location) {
-      return location.getAsFile();
-    } else {
-      throw new InvalidUserDataException(
-          "Unsupported value of type "
-              + notation.getClass()
-              + " used as a module path: "
-              + notation);
-    }
-  }
-
-  protected URI parseModuleNotationToUri(Object m) {
-    var parsed1 = parseModuleNotation(m);
-    return parsedModuleNotationToUri(parsed1);
-  }
-
-  /**
-   * Converts either a file or a URI to a URI. We convert a file to a URI via the {@link
-   * IoUtils#createUri(String)} because other ways of conversion can make relative paths into
-   * absolute URIs, which may break module loading.
-   */
-  private URI parsedModuleNotationToUri(Object notation) {
-    if (notation instanceof File file) {
-      return IoUtils.createUri(file.getPath());
-    } else if (notation instanceof URI uri) {
-      return uri;
-    }
-    throw new IllegalArgumentException("Invalid parsed module notation: " + notation);
-  }
-
  protected List<Pattern> patternsFromStrings(List<String> patterns) {
    return patterns.stream().map(Pattern::compile).collect(Collectors.toList());
  }
--- a/pkl-gradle/src/main/java/org/pkl/gradle/task/ModulesTask.java
+++ b/pkl-gradle/src/main/java/org/pkl/gradle/task/ModulesTask.java
@@ -37,8 +37,8 @@ import org.gradle.api.tasks.Optional;
 import org.gradle.api.tasks.TaskAction;
 import org.pkl.commons.cli.CliBaseOptions;
 import org.pkl.core.evaluatorSettings.Color;
-import org.pkl.core.util.IoUtils;
 import org.pkl.core.util.Pair;
+import org.pkl.gradle.utils.PluginUtils;

 public abstract class ModulesTask extends BasePklTask {
  // We expose the contents of this property as task inputs via the sourceModuleFiles
@@ -84,7 +84,7 @@ public abstract class ModulesTask extends BasePklTask {
  @Override
  protected List<URI> getSourceModulesAsUris() {
    return getSourceModules().get().stream()
-        .map(this::parseModuleNotationToUri)
+        .map(PluginUtils::parseModuleNotationToUri)
        .collect(Collectors.toList());
  }

@@ -117,7 +117,7 @@ public abstract class ModulesTask extends BasePklTask {
    var files = new ArrayList<File>();
    var uris = new ArrayList<URI>();
    for (var m : modules) {
-      var parsed = parseModuleNotation(m);
+      var parsed = PluginUtils.parseModuleNotation(m);
      if (parsed instanceof File file) {
        files.add(file);
      } else if (parsed instanceof URI uri) {
@@ -127,28 +127,6 @@ public abstract class ModulesTask extends BasePklTask {
    return Pair.of(files, uris);
  }

-  /**
-   * Converts either a file or a URI to a URI. We convert a relative file to a URI via the {@link
-   * IoUtils#createUri(String)} because other ways of conversion can make relative paths into
-   * absolute URIs, which may break module loading.
-   */
-  private URI parsedModuleNotationToUri(Object notation) {
-    if (notation instanceof File file) {
-      if (file.isAbsolute()) {
-        return file.toPath().toUri();
-      }
-      return IoUtils.createUri(IoUtils.toNormalizedPathString(file.toPath()));
-    } else if (notation instanceof URI uri) {
-      return uri;
-    }
-    throw new IllegalArgumentException("Invalid parsed module notation: " + notation);
-  }
-
-  protected URI parseModuleNotationToUri(Object m) {
-    var parsed1 = parseModuleNotation(m);
-    return parsedModuleNotationToUri(parsed1);
-  }
-
  @TaskAction
  @Override
  public void runTask() {
@@ -172,7 +150,7 @@ public abstract class ModulesTask extends BasePklTask {
              parseModulePath(),
              getProject().getProjectDir().toPath(),
              mapAndGetOrNull(getEvalRootDirPath(), Paths::get),
-              mapAndGetOrNull(getSettingsModule(), this::parseModuleNotationToUri),
+              mapAndGetOrNull(getSettingsModule(), PluginUtils::parseModuleNotationToUri),
              getProjectDir().isPresent() ? getProjectDir().get().getAsFile().toPath() : null,
              getEvalTimeout().getOrNull(),
              mapAndGetOrNull(getModuleCacheDir(), it1 -> it1.getAsFile().toPath()),
--- a/pkl-gradle/src/main/java/org/pkl/gradle/utils/PluginUtils.java
+++ b/pkl-gradle/src/main/java/org/pkl/gradle/utils/PluginUtils.java
@@ -0,0 +1,128 @@
+/*
+ * Copyright © 2024 Apple Inc. and the Pkl project authors. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.pkl.gradle.utils;
+
+import java.io.File;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.net.URL;
+import java.nio.file.InvalidPathException;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import org.gradle.api.InvalidUserDataException;
+import org.gradle.api.file.FileSystemLocation;
+import org.pkl.core.util.IoUtils;
+
+public class PluginUtils {
+  private PluginUtils() {}
+
+  /**
+   * Parses the specified source module notation into a "parsed" notation which is then used for
+   * input path tracking and as an argument for the CLI API.
+   *
+   * <p>This method accepts the following input types:
+   *
+   * <ul>
+   *   <li>{@link URI} - used as is.
+   *   <li>{@link File} - used as is.
+   *   <li>{@link Path} - converted to a {@link File}. This conversion may fail because not all
+   *       {@link Path}s point to the local file system.
+   *   <li>{@link URL} - converted to a {@link URI}. This conversion may fail because {@link URL}
+   *       allows for URLs which are not compliant URIs.
+   *   <li>{@link CharSequence} - first, converted to a string. If this string is "URI-like" (see
+   *       {@link IoUtils#isUriLike(String)}), then we attempt to parse it as a {@link URI}, which
+   *       may fail. Otherwise, we attempt to parse it as a {@link Path}, which is then converted to
+   *       a {@link File} (both of these operations may fail).
+   *   <li>{@link FileSystemLocation} - converted to a {@link File} via the {@link
+   *       FileSystemLocation#getAsFile()} method.
+   * </ul>
+   *
+   * In case the returned value is determined to be a {@link URI}, then this URI is first checked
+   * for whether its scheme is {@code file}, like {@code file:///example/path}. In such case, this
+   * method returns a {@link File} corresponding to the file path in the URI. Otherwise, a {@link
+   * URI} instance is returned.
+   *
+   * @throws InvalidUserDataException In case the input is none of the types described above, or
+   *     when the underlying value cannot be parsed correctly.
+   */
+  public static Object parseModuleNotation(Object notation) {
+    if (notation instanceof URI uri) {
+      if ("file".equals(uri.getScheme())) {
+        return new File(uri.getPath());
+      }
+      return uri;
+    } else if (notation instanceof File) {
+      return notation;
+    } else if (notation instanceof Path path) {
+      try {
+        return path.toFile();
+      } catch (UnsupportedOperationException e) {
+        throw new InvalidUserDataException("Failed to parse Pkl module file path: " + notation, e);
+      }
+    } else if (notation instanceof URL url) {
+      try {
+        return parseModuleNotation(url.toURI());
+      } catch (URISyntaxException e) {
+        throw new InvalidUserDataException("Failed to parse Pkl module URI: " + notation, e);
+      }
+    } else if (notation instanceof CharSequence) {
+      var s = notation.toString();
+      if (IoUtils.isUriLike(s)) {
+        try {
+          return parseModuleNotation(IoUtils.toUri(s));
+        } catch (URISyntaxException e) {
+          throw new InvalidUserDataException("Failed to parse Pkl module URI: " + s, e);
+        }
+      } else {
+        try {
+          return Paths.get(s).toFile();
+        } catch (InvalidPathException | UnsupportedOperationException e) {
+          throw new InvalidUserDataException("Failed to parse Pkl module file path: " + s, e);
+        }
+      }
+    } else if (notation instanceof FileSystemLocation location) {
+      return location.getAsFile();
+    } else {
+      throw new InvalidUserDataException(
+          "Unsupported value of type "
+              + notation.getClass()
+              + " used as a module path: "
+              + notation);
+    }
+  }
+
+  /**
+   * Converts either a file or a URI to a URI. We convert a relative file to a URI via the {@link
+   * IoUtils#createUri(String)} because other ways of conversion can make relative paths into
+   * absolute URIs, which may break module loading.
+   */
+  public static URI parsedModuleNotationToUri(Object notation) {
+    if (notation instanceof File file) {
+      if (file.isAbsolute()) {
+        return file.toPath().toUri();
+      }
+      return IoUtils.createUri(IoUtils.toNormalizedPathString(file.toPath()));
+    } else if (notation instanceof URI uri) {
+      return uri;
+    }
+    throw new IllegalArgumentException("Invalid parsed module notation: " + notation);
+  }
+
+  public static URI parseModuleNotationToUri(Object m) {
+    var parsed1 = PluginUtils.parseModuleNotation(m);
+    return parsedModuleNotationToUri(parsed1);
+  }
+}
--- a/pkl-gradle/src/main/java/org/pkl/gradle/utils/package-info.java
+++ b/pkl-gradle/src/main/java/org/pkl/gradle/utils/package-info.java
@@ -0,0 +1,4 @@
+@NonnullByDefault
+package org.pkl.gradle.utils;
+
+import org.pkl.core.util.NonnullByDefault;
--- a/pkl-gradle/src/test/kotlin/org/pkl/gradle/PkldocGeneratorsTest.kt
+++ b/pkl-gradle/src/test/kotlin/org/pkl/gradle/PkldocGeneratorsTest.kt
@@ -15,13 +15,17 @@
 */
 package org.pkl.gradle

+import java.nio.file.Path
 import kotlin.io.path.readText
 import org.assertj.core.api.Assertions.assertThat
 import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.io.TempDir
+import org.pkl.commons.test.PackageServer

 class PkldocGeneratorsTest : AbstractTest() {
  @Test
-  fun `generate docs`() {
+  fun `generate docs`(@TempDir tempDir: Path) {
+    PackageServer.populateCacheDir(tempDir)
    writeFile(
      "build.gradle",
      """
@@ -32,7 +36,8 @@ class PkldocGeneratorsTest : AbstractTest() {
      pkl {
        pkldocGenerators {
          pkldoc {
-            sourceModules = ["person.pkl", "doc-package-info.pkl"]
+            moduleCacheDir = file("${tempDir.toUri()}")
+            sourceModules = ["package://localhost:0/birds@0.5.0", "person.pkl", "doc-package-info.pkl"]
            outputDir = file("build/pkldoc")
            settingsModule = "pkl:settings"
          }
@@ -94,6 +99,39 @@ class PkldocGeneratorsTest : AbstractTest() {
    checkTextContains(moduleFile.readText(), "<html>", "Person", "Address", "other")
    checkTextContains(personFile.readText(), "<html>", "name", "addresses")
    checkTextContains(addressFile.readText(), "<html>", "street", "zip")
+
+    val birdsPackageFile = baseDir.resolve("localhost(3a)0/birds/0.5.0/index.html")
+    assertThat(birdsPackageFile).exists()
+  }
+
+  @Test
+  fun `generate docs only for package`(@TempDir tempDir: Path) {
+    PackageServer.populateCacheDir(tempDir)
+    writeFile(
+      "build.gradle",
+      """
+      plugins {
+        id "org.pkl-lang"
+      }
+
+      pkl {
+        pkldocGenerators {
+          pkldoc {
+            moduleCacheDir = file("${tempDir.toUri()}")
+            sourceModules = ["package://localhost:0/birds@0.5.0"]
+            outputDir = file("build/pkldoc")
+            settingsModule = "pkl:settings"
+          }
+        }
+      }
+    """
+    )
+
+    runTask("pkldoc")
+
+    val baseDir = testProjectDir.resolve("build/pkldoc")
+    val birdsPackageFile = baseDir.resolve("localhost(3a)0/birds/0.5.0/index.html")
+    assertThat(birdsPackageFile).exists()
  }

  @Test