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

Write adhoc_tool(stdout/stderr="...") relative to workdir, support absolute paths #18814

Merged
merged 3 commits into from
Apr 26, 2023

Conversation

huonw
Copy link
Contributor

@huonw huonw commented Apr 24, 2023

This fixes #18810 by ensuring adhoc_tool(stdout="...", stderr="...") are written relative to the working directory, and before the root_output_directory filtering happens.

This also fixes a second bug I noticed while working on this: if the path is absolute, e.g. stdout="/out.txt", pants crashes: panic at 'called `Result::unwrap()` on an `Err` value: "Absolute paths are not allowed: \"/out.txt\""', src/intrinsics.rs:420. After this PR, they are supported, and taken to start at the build root.

@huonw huonw added the category:bugfix Bug fixes for released features label Apr 24, 2023
@huonw huonw requested review from kaos, chrisjrn and thejcannon April 24, 2023 12:08
@chrisjrn
Copy link
Contributor

Good catch!

Given that this is now more frequently writing into subdirs, there should probably be an extra test to make sure that ../-type relatives paths work for the value of stdout and stderr fields.

@chrisjrn
Copy link
Contributor

(nb. Cherry-pick required back to 2.16.x)

@kaos kaos added this to the 2.16.x milestone Apr 24, 2023
@huonw huonw changed the title Write adhoc_tool(stdout/stderr="...") relative to workdir Write adhoc_tool(stdout/stderr="...") relative to workdir, support absolute paths Apr 25, 2023
@huonw
Copy link
Contributor Author

huonw commented Apr 25, 2023

Done! I've made a few additional changes:

  • now that the test is parametrised and so runs many more, I converted it to use system_binary rather than running a Python source. On my machine, this means each test takes 0.7-0.9s, whereas it was previously ~2.8s.
  • I noticed that absolute paths crashed pants, and so added explicit support for them (updated the PR description)

@huonw huonw merged commit bcfd4f3 into pantsbuild:main Apr 26, 2023
@huonw huonw deleted the bugfix/18810-adhoc-stdout-stderr branch April 26, 2023 20:02
huonw added a commit to huonw/pants that referenced this pull request Apr 26, 2023
…solute paths (pantsbuild#18814)

This fixes pantsbuild#18810 by ensuring `adhoc_tool(stdout="...", stderr="...")`
are written relative to the working directory, and before the
`root_output_directory` filtering happens.

This also fixes a second bug I noticed while working on this: if the
path is absolute, e.g. `stdout="/out.txt"`, pants crashes: ```panic at
'called `Result::unwrap()` on an `Err` value: "Absolute paths are not
allowed: \"/out.txt\""', src/intrinsics.rs:420```. After this PR, they
are supported, and taken to start at the build root.
huonw added a commit that referenced this pull request Apr 27, 2023
…solute paths (Cherry-pick of #18814) (#18839)

This fixes #18810 by ensuring `adhoc_tool(stdout="...", stderr="...")`
are written relative to the working directory, and before the
`root_output_directory` filtering happens.

This also fixes a second bug I noticed while working on this: if the
path is absolute, e.g. `stdout="/out.txt"`, pants crashes: ```panic at
'called `Result::unwrap()` on an `Err` value: "Absolute paths are not
allowed: \"/out.txt\""', src/intrinsics.rs:420```. After this PR, they
are supported, and taken to start at the build root.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:bugfix Bug fixes for released features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

adhoc_tool stdout and stderr are relative to build root, not workdir
3 participants