pantsbuild / pants

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

method to have optional docker.tools #21098

Closed cburroughs closed 1 month ago

cburroughs commented 3 months ago

Is your feature request related to a problem? Please describe.

We use amazon-ecr-credential-helper to authenticate to ECR (Amazon's Docker registry offering) where we store Docker images. We configure it for Pants as:

[docker]
tools = ["docker-credential-ecr-login"]

However, docker-credential-ecr-login shells out to additional operating system dependent binaries:

We currently handle this with instruction to setup a short ~/.pants.rc file like:

    $ cat ~/.pants.rc
    [docker]
    tools.add = ["sw_vers"]

But this this manual out-of-repo step is a source of papercuts. docker.tools requires the binaries to exist to we can't list the super set of all needed ones:

13:22:52.49 [ERROR] 1 Exception encountered:

Engine traceback:
  in `publish` goal

BinaryNotFoundError: Cannot find `sw_vers` on `['/bin', '/home/ecsb/.cargo/bin', '/home/ecsb/.local/bin', '/home/ecsb/bin/', '/opt/bin', '/sbin', '/usr/bin', '/usr/lib/llvm/14/bin', '/usr/lib/llvm/15/bin', '/usr/lib/llvm/16/bin', '/usr/lib64/julia-1.9.4/bin', '/usr/local/bin', '/usr/local/sbin', '/usr/sbin']`. Please ensure that it is installed so that Pants can use docker.

Describe the solution you'd like

docker.optional_tools or another way of listing tools that should be used if present, but do not throw an error when absent.

Describe alternatives you've considered

Some sort of way to specify per OS docker.tools I think this would eventually turn into a mini-langauge for (OS,arch,libc) and be quite complex.

Additional context I thought this had been discussed in a prior issue, but I have failed to find it.

tdyas commented 2 months ago

The relevant call site where tool search occurs in the Docker backend is https://github.com/pantsbuild/pants/blob/275b6ec0eaa4aa1acd055dd3ad21f37817295c03/src/python/pants/backend/docker/util_rules/docker_binary.py#L167-L175.

Looking at the current implementation, this would first need a small refactor for BinaryPathRequest -> BinaryPaths to support fallible lookups. Then that call site could use such support for any optional_tools support.

tdyas commented 1 month ago

As it turns out, no refactor was necessary. https://github.com/pantsbuild/pants/pull/21283 is a draft PR implementing a --docker-optional-tools option.

This is one solution. Another solution would focus on per-platform mandatory tools. Probably worth a discussion on the development Slack.

riisi commented 1 month ago

Ah this is great, thanks! Have been hitting this with docker-credentials-osxkeychain not needed on windows.