Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Projects loaded from arbitrary URIs #255

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HT154
Copy link
Contributor

@HT154 HT154 commented Feb 26, 2024

Currently, Pkl assumes that a PklProject file and any declared local dependencies are accessible on the filesystem (i.e. via file: URIs. For usage of the pkl CLI this is sufficient, but for usage of language bindings this presents an obstacle for evaluating Pkl code using projects from sources that are not the filesystem. One compelling use case would be consuming Pkl code using project dependencies from a git repository; using go-git and its go-billy filesystem abstraction, it should be possible to do this completely in-memory. With this change and apple/pkl-go#21 this now works end-to-end.

Brief summary of the changes here:

  • Replace usage of Path projectDir with URI projectBaseUri throughout
  • Switch reading of PklProject.deps.json to use resource readers
  • Update pkl:Project to accept local dependencies when the scheme and authority are the same, rather than checking for file: URIs specifically
  • Add a test in pkl-core for custom-defined module and resource readers
  • Fix NPE in pkl-server's handling of empty/absent pathElements in List*Responses

@@ -176,7 +183,7 @@ typealias LocalDependency = Project(
/// 1. Gather all dependencies, both direct and transitive.
/// 2. For each package's major version, determine the highest declared minor version.
/// 3. Write each resolved dependency to sibling file `PklProject.deps.json`.
dependencies: Mapping<String(!contains("/")), *RemoteDependency|LocalDependency>
dependencies: Mapping<String(!contains("/")), * RemoteDependency|Project(isValidLoadDependency)>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This works around a bug in typealias resolution that caused the above projectFileUri to return pkl:Project instead of the actual module URI

Comment on lines +71 to +76
if (response.error != null) {
completeExceptionally(IOException(response.error))
} else if (response.pathElements != null) {
complete(response.pathElements)
} else {
completeExceptionally(IOException(response.error))
complete(emptyList())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

response.pathElements may be null if the client resource reader (eg. in pkl-go) returns an empty/nil slice of PathElements

Comment on lines 95 to 103
is ListModulesResponse -> {
if (response.error != null) {
completeExceptionally(IOException(response.error))
} else if (response.pathElements != null) {
complete(response.pathElements)
} else {
complete(response.pathElements!!)
complete(emptyList())
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here: response.pathElements may be null if the client resource reader (eg. in pkl-go) returns an empty/nil slice of PathElements. This also aligns the implementation with ClientResourceReader.

Comment on lines +37 to +41

// https://github.com/apple/pkl/issues/166
// ["glob-import behind local project import"] {
// import("@project6/children.pkl")
// }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a test for #166. I suspected these changes might resolve the issue, but they do not. This change did seem to surface a clearer error though. It looks like when the ImportGlobNode is executed, currentModule has a file: URI while importUri has a projectpackage: URI. This disagreement causes ModuleKeys.ProjectPackage.listElements to eventually bark since it doesn't know about file: URIs.

Comment on lines +175 to +204
/**
* Used by ResourceReaders.ProjectPackageResource to resolve resources from projects that may not
* be on the local filesystem
*/
public List<PathElement> listElements(URI baseUri) throws IOException, SecurityManagerException {
var reader = resourceReaders.get(baseUri.getScheme());
if (reader == null) {
throw new VmExceptionBuilder()
.evalError("noResourceReaderRegistered", baseUri.getScheme())
.build();
}

return reader.listElements(securityManager, baseUri);
}

/**
* Used by ResourceReaders.ProjectPackageResource to resolve resources from projects that may not
* be on the local filesystem
*/
public boolean hasElement(URI elementUri) throws IOException, SecurityManagerException {
var reader = resourceReaders.get(elementUri.getScheme());
if (reader == null) {
throw new VmExceptionBuilder()
.evalError("noResourceReaderRegistered", elementUri.getScheme())
.build();
}

return reader.hasElement(securityManager, elementUri);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not a huge fan of needing to pipe these through ResourceManager like this. I'm more than open to alternate suggestions on how to handle this

@HT154 HT154 marked this pull request as ready for review February 26, 2024 23:41
@HT154 HT154 force-pushed the projects-from-any-uri branch 2 times, most recently from 5a12cd5 to 0aafd91 Compare March 21, 2024 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant