Show gpg output in server/client in publish (fix #387) #2059
Conversation
Avoid using `os.Inherit` which is known to break the output in server/client mode.
| @@ -174,8 +175,7 @@ class SonatypePublisher( | |||
| val fileName = file.toString | |||
| val command = "gpg" +: args :+ fileName | |||
|
|
|||
| os.proc(command.map(v => v: Shellable)) | |||
| .call(stdin = os.Inherit, stdout = os.Inherit, stderr = os.Inherit) | |||
| Jvm.runSubprocess(command, Map.empty[String, String], workingDir = null) | |||
There was a problem hiding this comment.
Why are you setting workingDir explicitly to null? (The current implementation is handling it as os.pwd.) Besides being ugly and non-idiomatic Scala, it may not always be the right location. I think using T.workspace is the better option here.
There was a problem hiding this comment.
Are you sure the current implementation is handling as os.pwd?
My understanding is that we are calling os.proc() which by default uses null as cwd
https://github.com/com-lihaoyi/os-lib/blob/main/os/src-jvm/ProcessOps.scala#L53
To better match the current behavior I need to pass sys.env as the env instead of an empty Map.
There was a problem hiding this comment.
Yeah, I think so.
But it's an undocumented (I think) implementation detail, so I'd prefer to make it explicit.
There was a problem hiding this comment.
Also, the current env should come via T.env, as sys.env might be stale in server-mode, and this whole PR is about improving server-mode, right?
There was a problem hiding this comment.
Yes, you are right!
When cwd is null it fallbacks to os.pwd:
@ os.proc("pwd").call(cwd = null).out.text
res1: String = """/Users/lorenzo
"""
@ os.proc("pwd").call(cwd = os.pwd).out.text
res2: String = """/Users/lorenzo
"""There was a problem hiding this comment.
Now, SonatypePublisher doesn't know Ctx and can't access T.workspace or T.env. Should I deprecate the APIs and define new ones that receive implicit ctx: Ctx? It's a bit of work though
There was a problem hiding this comment.
Since the class was already receiving a Logger explicitly via T.log, I added another two parameters, workspace and env and I'm passing T.workspace and T.env. In the deprecated constructor, I'm using sys.env and os.pwd instead.
There was a problem hiding this comment.
The only issue I see with this approach is, that there is a chance the created instance is used when the context is no longer valid. Should be no issue for Ctx.workspace or Ctx.log (although misleading) , but Ctx.dest of Ctx.env could be already invalid.
As a compromise, we can keep it this way for now, but schedule a rework of the Publisher API to the next major release. I already have a tracker ticket for that:
There was a problem hiding this comment.
I understand the issue.
Currently, we create a fresh instance of SonatypePublisher every time so it's not an issue internally. But we can change the API when we are in the next breaking window (0.11.x)
- Deprecate old constructor and provide previous behavior via sys.env and os.pwd
lefou
changed the title
Avoid using
os.Inheritwhich is known to break the output in server/client mode.Fixes #387
Checked manually and the errors now appear with and without
-i.