-
-
Notifications
You must be signed in to change notification settings - Fork 359
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
Fix BSP regressions and improved resource file handling #2420
Conversation
|
bsp/src/mill/bsp/BSP.scala
Outdated
repositoriesTask() | ||
) | ||
private def bspWorkerLibs: T[Agg[PathRef]] = { | ||
sys.props.getOrElse("MILL_UNDER_TEST", "0") match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this change for? AFAICT, it doesn't modify the security characteristics at all: the existing millProjectModule
already uses sys.props
to control things, so anyone who has control over the MILL_BSP_WORKER
property would also have control over the MILL_UNDER_TEST
property
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this seems to be left over. I searched for the issues which later turned out to come from #2419. I'll revert it.
val out: PrintStream, | ||
val err: PrintStream, | ||
val in: InputStream, | ||
val bspLog: Option[PrintStream] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is the right place to put this, since it is really only used by the BSP logic and not the rest of the code in the repo. Furthermore, it kind of breaks the encapsulation here, which is that SystemStreams
is meant to be the stdout
/stdin
/stderr
streams that are provided by the operating system and not include any application-specific values
IMO we should pass bspLog
around separately, or put it on our existing Logger
class, or use an environment variable or JVM property to pass around the log file path. Looking at the code, the last option (environment variable) seems the most appropriate, since this is really only relevant to one single worker and not the rest of the Mill codebase, so putting it on one of the "global" context objects doesn't make sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is the right place to put this, since it is really only used by the BSP logic and not the rest of the code in the repo.
Yeah, makes sense. Let me find a better solution.
Furthermore, it kind of breaks the encapsulation here, which is that
SystemStreams
is meant to be thestdout
/stdin
/stderr
streams that are provided by the operating system and not include any application-specific values
Here I disagree. Initially, SystemStreams
was created to hold the OS provided streams, but after your refactoring, it's rather a class to pass the three typical streams around. The point in the BSP mode is, that we very early redirect and run the rest of Mill with these customizations. If we want SystemStreams
to hold the original OS values, we can't use this class further down to pass customized streams around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I don't mean that it's only meant to pass the oringal OS streams around, more that it's meant to pass some abstraction that looks like them and expose the same interface. So they can get substituted or redirected, but they're still meant to have 3 streams. So any code that receives it can use those three streams as if it's the OS-provided ones, even if they got replaced somewhere along the way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood. Let me work out some alternative for bspLog
.
@lihaoyi I addressed your review comments and am currently in the process of rebasing/merging to latest main, which is a pita. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that familiar with the BSP code, but I think this looks ok
I needed to fix various parts. For example the format of the embedded BSP server impl dependency in the BuildInfo had changed, and some other things, but this should work now. Tested with vscode. |
Fix some BSP regression and some improvements
mill --bsp
is dumping all output onSTDERR
which is very noisy