wave I was hopeful that it d be easy to implement the fix f Pants #development

:wave: I was hopeful that it’d be easy to implemen...

sparse-lifeguard-95737

05/04/2022, 2:38 PM

👋 I was hopeful that it’d be easy to implement the fix for https://github.com/pantsbuild/pants/issues/15322, but after digging around the code for a bit I might be stumped. we’re setting the

PATH

to the

.shims

dir in the

DockerBinary

setup here - it’s not obvious to me if there’s a way to inject the absolute path of the final work-dir into that value. I thought about hacking in a

$(pwd)

or something, but looks like the values we set in the process env get escaped before being written to

__run.sh

sparse-lifeguard-95737

05/04/2022, 2:39 PM

Within the

export

logic there’s support for a magic

{digest_root}

placeholder in

argv

- is there anything similar for process envs that I could use here?

sparse-lifeguard-95737

05/04/2022, 2:40 PM

cc @curved-television-6568 since I think you’re most familiar with this area

sparse-lifeguard-95737

05/04/2022, 4:43 PM

it looks like

.shims

is just a symlink to another tmp dir - could we drop the symlink entirely, and instead set

PATH=<path-to-tmpdir>

👍 1

hundreds-father-404

05/04/2022, 4:46 PM

That's definitely worth trying!

hundreds-father-404

05/04/2022, 4:47 PM

We have some test for BinaryShims in src/python/pants/core/util_rules/system_binaries_test.py which you can use when making edits to verify things still work edit: actually might need to adjust a bit since API contract is changing

hundreds-father-404

05/04/2022, 4:49 PM

Ah, src/python/pants/backend/codegen/protobuf/lint/buf/format_rules.py uses BinaryShims so that's a good integration test

sparse-lifeguard-95737

05/04/2022, 4:50 PM

🤔 I was hoping that I would only need to change the logic in the docker subsystem, not the binary-shims code itself

sparse-lifeguard-95737

05/04/2022, 4:50 PM

but still investigating to see if that’s feasible

sparse-lifeguard-95737

05/04/2022, 4:52 PM

i.e. if I can take the

Digest

from the

BinaryShims

instance and somehow get the underlying absolute path of the directory containing the digest’s contents, I could plug that into the `DockerBinary`’s

extra_env

sparse-lifeguard-95737

05/04/2022, 7:25 PM

@hundreds-father-404 what do you think of this idea: when docker tools are configured, we write an extra shim for

docker

into the temp working dir, and call it instead of directly invoking the

docker

binary. the shim will

export PATH=$(pwd)/.shims/bin:${PATH}

and then exec to the

docker

binary

sparse-lifeguard-95737

05/04/2022, 7:26 PM

it’s ugly, but (I think) any other approach would require some more major refactoring to components outside of the docker subsystem

sparse-lifeguard-95737

05/04/2022, 10:31 PM

https://github.com/pantsbuild/pants/pull/15330 should fix https://github.com/pantsbuild/pants/issues/15322 but the approach is a bit hacky, so I’ve opened for early review (still needs tests). can people take a look at let me know if it’s worth finishing out?

curved-television-6568

05/05/2022, 9:49 AM

I’m looking into this. Rather than requiring an extra dedicated shim here, I’ll propose to support a

{root}

placeholder value in the

PATH

env, so we can inject the sandbox path into the env var. cc @witty-crayon-22786 wdyt about that approach?

curved-television-6568

05/05/2022, 3:57 PM

I've pushed a couple PRs for review on this.

🙌 2

👀 1

Open in Slack

Previous Next