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.

Sign up for GitHub

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

Jump to bottom

fix issue with duplicated BinaryPath instances passed to BinaryShimsRequest #21745

Merged
merged 5 commits into from
Dec 13, 2024
Merged

fix issue with duplicated BinaryPath instances passed to BinaryShimsRequest #21745

merged 5 commits into from
Dec 13, 2024

Conversation

tdyas
Copy link
Contributor

@tdyas tdyas commented Dec 11, 2024 •
edited
Loading

As reported in #21709, a Docker tool appearing in both the --docker-tools and --docker-optional-tools options was causing an error with the create_digest intrinsic: Snapshots must be constructed from unique path stats; got duplicates in [Some("docker-credential-ecr-login"), Some("getent"), Some("sw_vers")]

The root cause is that duplicate BinaryPath instances were being passed to BinaryShimsRequest.for_paths which was then translated eventually into a create_digest call with the duplicate paths (which is not permitted).

Solution: De-duplicate paths in BinaryShimsRequest.for_paths (and sort into stable order for good measure to ensure better cacheability).

Closes #21709.

@tdyas tdyas added the category:bugfix Bug fixes for released features label Dec 11, 2024
@tdyas tdyas requested review from cburroughs, kaos and huonw December 11, 2024 18:56
@tdyas tdyas mentioned this pull request Dec 11, 2024
Closed
@tdyas tdyas added this to the 2.23.x milestone Dec 11, 2024
@huonw
Copy link
Contributor

huonw commented Dec 12, 2024

Could we have a test for this?

@tdyas
Copy link
Contributor Author

tdyas commented Dec 12, 2024

Could we have a test for this?

Added tests.

Also added detection of duplicate paths where the content hashes differ (which results in unequal BinaryPath instances but is still a problem for creating a Digest).

huonw
huonw approved these changes Dec 13, 2024
View reviewed changes
Copy link
Contributor

@huonw huonw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to merge (question doesn't block it).

src/python/pants/core/util_rules/system_binaries.py
for path, group in groupby(paths, key=lambda x: x.path):
if len(list(group)) > 1:
duplicate_paths.add(path)
if duplicate_paths:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, how can two files listed under the same path have different content, on a single machine?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given how BinaryPaths are produced, they wouldn't ever have different content hashes, assuming they were generated correctly. This is me just protecting against an edge case if somehow rule logic decided to mix BinaryPath instances in some weird way.

@tdyas
Copy link
Contributor Author

tdyas commented Dec 13, 2024

The test failure seems flaky. Fix in #21756.

@tdyas tdyas force-pushed the docker/fix_duplicate_tools_error branch from 0b300ca to ef3df7f Compare December 13, 2024 21:34
@tdyas tdyas merged commit 220e91a into pantsbuild:main Dec 13, 2024
24 checks passed
@tdyas tdyas deleted the docker/fix_duplicate_tools_error branch December 13, 2024 22:42
@WorkerPants
Copy link
Member

I tried to automatically cherry-pick this change back to each relevant milestone, so that it is available in those older releases of Pants.

❌ 2.23.x

I was unable to cherry-pick this PR to 2.23.x, likely due to merge-conflicts.

Steps to Cherry-Pick locally

To resolve:

  1. (Ensure your git working directory is clean)
  2. Run the following script to reproduce the merge-conflicts: 
    git fetch https://github.com/pantsbuild/pants main \
  && git fetch https://github.com/pantsbuild/pants 2.23.x \
  && git checkout -b cherry-pick-21745-to-2.23.x FETCH_HEAD \
  && git cherry-pick 220e91a3cf688689f146574a18e9ffb66c56287c
  3. Fix the merge conflicts and commit the changes
  4. Run build-support/cherry_pick/make_pr.sh "21745" "2.23.x"

Please note that I cannot re-run CI if a job fails. Please work with your PR approver(s) to re-run CI if necessary.

✔️ 2.24.x

Successfully opened #21758.

When you're done manually cherry-picking, please remove the needs-cherrypick label on this PR.

Thanks again for your contributions!

🤖 Beep Boop here's my run link

@WorkerPants WorkerPants added the auto-cherry-picking-failed Auto Cherry-Picking Failed label Dec 13, 2024
tdyas added a commit that referenced this pull request Dec 13, 2024
@tdyas
fix issue with duplicated BinaryPath instances passed to `BinaryShi…
ad9a937 
…msRequest` (#21745)

As reported in #21709, a
Docker tool appearing in both the `--docker-tools` and
`--docker-optional-tools` options was causing an error with the
`create_digest` intrinsic: `Snapshots must be constructed from unique
path stats; got duplicates in [Some("docker-credential-ecr-login"),
Some("getent"), Some("sw_vers")]`

The root cause is that duplicate `BinaryPath` instances were being
passed to `BinaryShimsRequest.for_paths` which was then translated
eventually into a `create_digest` call with the duplicate paths (which
is not permitted).

Solution: De-duplicate paths in `BinaryShimsRequest.for_paths` (and sort
into stable order for good measure to ensure better cacheability).

Closes #21709.

---------

Co-authored-by: Tom Dyas <[email protected]>
@tdyas tdyas removed needs-cherrypick auto-cherry-picking-failed Auto Cherry-Picking Failed labels Dec 13, 2024
@tdyas
Copy link
Contributor Author

tdyas commented Dec 13, 2024

Manually cherry picked to 2.23.x here: #21759

tdyas added a commit that referenced this pull request Dec 14, 2024
@WorkerPants @tdyas
fix issue with duplicated BinaryPath instances passed to `BinaryShi…
5a60b66 
…msRequest` (Cherry-pick of #21745) (#21758)

As reported in #21709, a
Docker tool appearing in both the `--docker-tools` and
`--docker-optional-tools` options was causing an error with the
`create_digest` intrinsic: `Snapshots must be constructed from unique
path stats; got duplicates in [Some("docker-credential-ecr-login"),
Some("getent"), Some("sw_vers")]`

The root cause is that duplicate `BinaryPath` instances were being
passed to `BinaryShimsRequest.for_paths` which was then translated
eventually into a `create_digest` call with the duplicate paths (which
is not permitted).

Solution: De-duplicate paths in `BinaryShimsRequest.for_paths` (and sort
into stable order for good measure to ensure better cacheability).

Co-authored-by: Tom Dyas <[email protected]>
Co-authored-by: Tom Dyas <[email protected]>
tdyas added a commit that referenced this pull request Dec 14, 2024
@tdyas
fix issue with duplicated BinaryPath instances passed to `BinaryShi…
7901814 
…msRequest` (Cherry pick of #21745) (#21759)

As reported in #21709, a
Docker tool appearing in both the `--docker-tools` and
`--docker-optional-tools` options was causing an error with the
`create_digest` intrinsic: `Snapshots must be constructed from unique
path stats; got duplicates in [Some("docker-credential-ecr-login"),
Some("getent"), Some("sw_vers")]`

The root cause is that duplicate `BinaryPath` instances were being
passed to `BinaryShimsRequest.for_paths` which was then translated
eventually into a `create_digest` call with the duplicate paths (which
is not permitted).

Solution: De-duplicate paths in `BinaryShimsRequest.for_paths` (and sort
into stable order for good measure to ensure better cacheability).

Closes #21709.

---------

Co-authored-by: Tom Dyas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@huonw huonw huonw approved these changes

@cburroughs cburroughs cburroughs approved these changes

@kaos kaos Awaiting requested review from kaos

Assignees
No one assigned
Labels
category:bugfix Bug fixes for released features
Projects
None yet
Milestone
2.23.x
Development

Successfully merging this pull request may close these issues.

infinite error loop if docker.tools and docker.optional_tools have the same entry
4 participants
@tdyas @huonw @WorkerPants @cburroughs