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

Deprecate BinaryUtil as public API. #5601

Merged
merged 2 commits into from Mar 15, 2018

Conversation

Projects
None yet
3 participants
@benjyw
Copy link
Contributor

benjyw commented Mar 14, 2018

Clients shouldn't use it directly, because we want to be able to discover which binary tools are "registered" for use via BinaryToolBase subclass subsystems.

This change also gets rid of a heavy-handed mechanism for checking if two files have the same content. The old code was convoluted and hard to reason about, and included degrees of freedom (such as setting the hasher) that weren't used in practice. It also contained a bug (it used a hashlib.sha1(), which is mutable, as a default value for a method arg).

The replacement logic uses the built-in filecmp module. This is much more straightforward, and likely faster, than computing two hashes and comparing them.

@benjyw benjyw requested review from jsirois and UnrememberMe Mar 14, 2018

@benjyw benjyw force-pushed the benjyw:binary_util_private branch from 626c3ca to 0086daf Mar 14, 2018

@benjyw benjyw force-pushed the benjyw:binary_util_private branch from 0086daf to 72c85c0 Mar 14, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

@benjyw benjyw merged commit 09ec2ae into pantsbuild:master Mar 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@benjyw benjyw deleted the benjyw:binary_util_private branch Mar 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment