support setting the working directory on interactive process #22802
Conversation
| append_only_caches=process.append_only_caches, | ||
| immutable_input_digests=process.immutable_input_digests, | ||
| keep_sandboxes=keep_sandboxes, | ||
| working_directory=process.working_directory, |
There was a problem hiding this comment.
This is a subtle breaking change for plugin authors if they're currently setting working_directory but its getting ignored. I guess I could mention that in the release notes?
There was a problem hiding this comment.
Of the existing rules, ones look like they might be affected
test_shell_command_interactively via
twine_upload via
I'll see if there are failing tests when CI comes back in a few minutes, but I either can update these to drop the workingdir or leave them as is
There was a problem hiding this comment.
I think it's fine to treat this as a bugfix and mention it in the release notes.
| if !run_in_workspace { | ||
| command.current_dir(tempdir.path()); | ||
|
|
||
| let cwd = match (&process.working_directory, run_in_workspace) { |
There was a problem hiding this comment.
Maybe parse this with RelativePath to ensure that working directory is inside the build root?
There was a problem hiding this comment.
Plugin code is trusted, so more of a safety issue than a security issue.
There was a problem hiding this comment.
This actually already happens. Because interactive_process shares a lot of code with execute_process, the call to ExecuteProcess::lift ends up creating the RelativePath here
The error it produces is Exception: Absolute paths are not allowed: "/foo"
|
@chris-smith-zocdoc thanks for this! It'll need an entry in the docs/notes/2.31.x.md release notes to pass CI. |
|
I can add a note there @benjyw , but did you or @tdyas have an opinion on how I should handle the existing rules? #22802 (comment) Its unclear to me if that is going to break existing usages or not |
chris-smith-zocdoc
force-pushed
the
cs_working_dir
branch
from
b867985 to
ecaf675
Compare
(I commented there) |
|
LMK when this is ready for another look |
chris-smith-zocdoc
force-pushed
the
cs_working_dir
branch
from
028cb91 to
87eef68
Compare
|
Sorry for the delay here @benjyw I updated the note about the two existing usages I called out. Let me know if thats what you intended or not. CI seems to be failing for unrelated reasons. Rust compilation timed out in one and a race condition in another (slack thread) |
chris-smith-zocdoc
force-pushed
the
cs_working_dir
branch
from
87eef68 to
a04b86e
Compare
benjyw
left a comment
benjyw
left a comment
There was a problem hiding this comment.
Looks good! I don't think you need to address my release notes comment above. I might tweak it next time I edit them, but it's not worth a CI round in this PR.
|
|
||
| Pants no longer supports loading `pkg_resources`-style namespace packages for plugins. Instead, just use ["native namespace packages"](https://packaging.python.org/en/latest/guides/packaging-namespace-packages/#native-namespace-packages) as per [PEP 420](https://peps.python.org/pep-0420/). | ||
|
|
||
| Allow `InteractiveProcess` to set the working directory relative to the sandbox or workspace. Existing usages of `InteractiveProcess.from_process` will now respect the working directory if its set on the Process, which may be a breaking change depending on the use case. Two existing rules `twine_upload` for python package uploads and `test_shell_command_interactively` for shell command testing with the `--debug` flag will now honor the working directory if set on the Process. |
There was a problem hiding this comment.
I think this should emphasize that this is a bugfix, so while it might change existing behavior, it's not exactly a "breaking change" in the bad way. It's more like "you will now get what you asked for, and if you were previously implicitly relying on not getting it, you'll have to revisit".
|
Thanks @chris-smith-zocdoc ! If you happen to have a future PR in which you could also modify the notes per my comment, great. Otherwise don't worry about it. |
3 participants
We've run into a few cases where setting the working directory on an InteractiveProcess would be useful, in particular for rules are tests (ie TestRequest). In order to support debugging we convert the setup process to InteractiveProcess but since that drops the working directory you're unable to utilize it currently.
As a workaround we currently write a bash script into the sandbox like
This will help clean that up
This is also a pre-requisite to supporting setting the working dir in the Run goal (small thread here https://pantsbuild.slack.com/archives/C01CQHVDMMW/p1707448457657149) I'll handle that in a separate pr though