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

Support tests for BinaryPaths. #10770

Merged
merged 2 commits into from
Sep 13, 2020
Merged

Conversation

jsirois
Copy link
Contributor

@jsirois jsirois commented Sep 13, 2020

A BinaryPathRequest can now include a test to run against each found
binary to validate it works and optionally fingerprint it. Eventually
fingerprinting the binary contents should be performed automatically
and the optional test fingerprint mixed in with it when present as
tracked by #10769.

We leverage testing found binaries in PexEnvironment to ensure we find
only interpreter paths compatible with Pex bootstrapping. This plugs an
existing hole not yet encountered in the wild where a Python 2.6 binary
(for example) could be chosen and then PEX file bootstrapping fail as a
result. We additionally fingerprint interpreters passing the version
range test to ensure we detect interpreter upgrades and pyenv shim
switches. Even with the automatic hashing of binaries tracked in #10769
working, we'd still need to do this in the pyenv shim case since the
same shim script can redirect to different interpreters depending on
configuration external to the shim script.

[ci skip-rust]
[ci skip-build-wheels]

A BinaryPathRequest can now include a test to run against each found
binary to validate it works and optionally fingerprint it. Eventually
fingerprinting the binary contents should be performed automatically
and the optional test fingerprint mixed in with it when present as
tracked by pantsbuild#10769.

We leverage testing found binaries in PexEnvironment to ensure we find
only interpreter paths compatible with Pex bootstrapping. This plugs an
existing hole not yet encountered in the wild where a Python 2.6 binary
(for example) could be chosen and then PEX file bootstrapping fail as a
result. We additionally fingerprint interpreters passing the version
range test to ensure we detect interpreter upgrades and pyenv shim
switches. Even with the automatic hashing of binaries tracked in pantsbuild#10769
working, we'd still need to do this in the pyenv shim case since the
same shim script can redirect to different interpreters depending on
configuration external to the shim script.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@coveralls
Copy link

coveralls commented Sep 13, 2020

Coverage Status

Coverage remained the same at 0.0% when pulling ada70a7 on jsirois:pull/10768/follow-up into 9d93bdc on pantsbuild:master.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Fantastic! Thanks John.


found_paths = result.stdout.decode().splitlines()
if not request.test_args:
return dataclasses.replace(binary_paths, paths=[BinaryPath(path) for path in found_paths])
Copy link
Contributor

Choose a reason for hiding this comment

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

dataclasses.replace is neat, but also more cognitive load. How about instead having line 453 use return BinaryPaths(binary_name=request.binary_name), and likewise creating a new BinaryPaths with these returns?

Yes, more duplication. But simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I started with the more duplication code (and more nested code, which I'm fine with but you tend to push back on) and landed here so this appears to be eye of the beholder. Since dataclasses are ubiquitous in rules, using standard dataclasses APIs does not strike me as neat, its just pedestrian.

src/python/pants/engine/process.py Outdated Show resolved Hide resolved
src/python/pants/engine/process.py Show resolved Hide resolved
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks.

@jsirois jsirois merged commit 73c3c78 into pantsbuild:master Sep 13, 2020
@jsirois jsirois deleted the pull/10768/follow-up branch September 13, 2020 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants