pantsbuild / pants

The Pants Build System
https://www.pantsbuild.org
Apache License 2.0
3.32k stars 636 forks source link

File paths for isort lint run are not scrubbed/stripped properly with workunits. #14189

Open asherf opened 2 years ago

asherf commented 2 years ago
Screen Shot 2022-01-18 at 6 04 02 PM

@Eric-Arellano FYI

Eric-Arellano commented 2 years ago

This happens because isort uses absolute paths. We post-process it to strip from the console: https://github.com/pantsbuild/pants/blob/33e341a9814cbc82fd390779eb7e6055f9a4bd13/src/python/pants/backend/python/lint/isort/rules.py#L154-L157

That works for the console, but this workunit is showing ~ProcessResult, before the post-processing.

I believe there are two fixes we could do:

  1. Move post-processing into the Rust engine. Have Process() have something like a strip_chroot_prefix argument, and then Rust will strip it rather than Python.
    • This means no changes needed for streaming workunit handlers like BuildSense.
    • Maybe smells wrong for Rust to be modifying the std{out,err} of processes? It means we can't be 100% certain what the original value was.
  2. Start using artifacts() on LintResults.
    • Streaming workunit handlers like BuildSense would need to consume LintResults rather than the `ProcessResult.
    • Maybe smells wrong to be storing nearly same std{out,err} in two WUs, but not sure.

@stuhood thoughts?

stuhood commented 2 years ago
  1. Move post-processing into the Rust engine. Have Process() have something like a strip_chroot_prefix argument, and then Rust will strip it rather than Python.

This sounds reasonable to me.

Similar to the existing support, this would need to be on a best-effort basis though: if we run a Process remotely, then we can't know what the sandbox will look like, and can't strip it (same as today). So the support would probably be best implemented inside of the local::CommandRunner as the result is created.