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

Sends verbosity from the worker protocol to the worker and enable Java toolchain multiplex sandboxing #55

Merged
merged 3 commits into from
Sep 9, 2024

Conversation

jjudd
Copy link

@jjudd jjudd commented Sep 4, 2024

No description provided.

rules_java supports it, and we support it, so we might as well use it.
None of the workers use it yet, but it is an update to the interface.
Want to get it merged, so we can stop making changes to the interface
for a bit.
@jjudd jjudd requested a review from jadenPete September 4, 2024 21:29
@@ -14,7 +14,7 @@ abstract class WorkerMain[S](stdin: InputStream = System.in, stdout: PrintStream

protected[this] def init(args: Option[Array[String]]): S

protected[this] def work(ctx: S, args: Array[String], out: PrintStream, workDir: Path): Unit
protected[this] def work(ctx: S, args: Array[String], out: PrintStream, workDir: Path, verbosity: Int): Unit

Choose a reason for hiding this comment

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

Since we're getting the arguments and verbosity directly from the work request, should we just pass that to the worker directly instead of copying so many of its fields?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure we should because this does create a nice interface all the workers must adhere to. It does sometimes mean updates when the protocol changes. Good news is that tends to happen rather infrequently.

Choose a reason for hiding this comment

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

Yeah, I can see how this forces worker implementers to think about certain things. Nevermind, then.

@jjudd jjudd merged commit f23c160 into lucid-master Sep 9, 2024
1 check passed
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.

2 participants