Bind PackageServer to ephemeral port to avoid port conflicts

[translatenix]

Here is my take on how to best solve the "flaky PackageServer tests" problem. It is ready to be merged after #217 . See commit message for details. If you prefer a different solution, perhaps this can serve as inspiration.

[translatenix]

@bioball Rebased and ready for review.

[translatenix]

@bioball Ready from my side. See latest commit message for details. Quite happy with the cleanup.

[translatenix]

We have an existing pattern of having unconfigured() and preconfigured() builders; see EvaluatorBuilder, ConfigEvaluatorBuilder, ValueMapperBuilder, etc. I think let's follow that.

According to the Javadoc, Executor "evaluates Pkl modules in a sandbox". Does preconfiguring with System.getenv() and System.getProperties() make sense here? And shouldn't granting file system and network access be an explicit decision?

Apart from adding unconfigured()/preconfigured(), we could add builder().grantFileSystemAccess(rootDir).grantNetworkAccess(). Or we could stick to a basic builder for now. Let me know how to proceed.

Let's still remove this, though; it adds some unnecessary brittleness to our tests.

Removed. Just to clarify, this validation can't possibly cause brittleness. It prevents brittleness by catching the mistake of picking a well-known port as test port. Operating systems would never pick a well-known port as ephemeral port.

both are public APIs but expose flags that are only meant for test purposes (e.g. testMode, testPort added by this PR) and we call them "internal" in the doc comments.

There is a big difference: testMode is only useful for testing Pkl itself, whereas testPort is equally useful for testing third-party code that uses CliBaseOptions or HttpClient. But I won't fight over this one.

[bioball]

According to the Javadoc, Executor "evaluates Pkl modules in a sandbox". Does preconfiguring with System.getenv() and System.getProperties() make sense here? And shouldn't granting file system and network access be an explicit decision?

I don't see any issues with the set of options provided by preconfigured(). This is an opt-in behavior, and users are expected to know what preconfigured() means. It's actually not that different from using our evaluator in pkl-core (which is also a sandbox).

[translatenix]

I don't see any issues with the set of options provided by preconfigured(). This is an opt-in behavior, and users are expected to know what preconfigured() means. It's actually not that different from using our evaluator in pkl-core (which is also a sandbox).

What is Executor used for at Apple? It seems to be intended for different use cases than (say) ConfigEvaluator, where exposing process env vars is a sensible default. What also worries me about preconfigured()/unconfigured() is that it's all or nothing, and that without adding many additional methods, preconfigured() values are impossible to customize. On the other hand, a method such as grantNetworkAccess() is naturally incremental.

[translatenix]

I've made 2 out of 3 requested changes and have squashed my commits. I'll leave any builder changes to you.

I've also rebased my Gradle 8.6 PR, which I'd love to land by the end of the week.

[bioball]

The executor API is typically meant for services that need to evaluate 3rd party Pkl code. For instance, if you are a service that accepts Pkl files as configuration. And, like you said, in those cases, you wouldn't want to pass your own env vars or system properties to Pkl.

The core thing that the executor provides that ConfigEvaluator (or just Evaluator) don't is the ability to use multiple different Pkl versions for evaluation. The rest of the sandboxing is already part of Evaluator/ConfigEvaluator (e.g. allowed modules, allowed resources).

Thought about this a little more--yeah, there's probably not much of a use-case for ExecutorOptions#preconfigured(). I'm okay merging this as-is.