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

Stabilize PythonInterpreter __eq__. #31

Merged

Conversation

jsirois
Copy link
Member

@jsirois jsirois commented Dec 12, 2014

The prior check used the binary's stat which was unstable across
variations of mounts atime setting on different machines. If the
partition the python binary lived on had atime enabled (default in
OSX so an important case), then equality checks would ~always fail
for even the same binary.

This change switches to using binary real path + its reporter
version info which should be all we care about. It's true that
the binary might be replaced by a new one of the same version at
the same path but compiled with different options though, so we
still could do more and instead use binary real path + a content
hash of the binary if needed.

https://rbcommons.com/s/twitter/r/1477/

The prior check used the binary's stat which was unstable across
variations of mounts atime setting on different machines. If the
partition the python binary lived on had atime enabled (default in
OSX so an important case), then equality checks would ~always fail
for even the same binary.

This change switches to using binary real path + its reporter
version  info which should be all we care about.  It's true that
the binary might be replaced by a new one of the same version at
the same path but compiled with different options though, so we
still could do more and instead use binary real path + a content
hash of the binary if needed.
wickman added a commit that referenced this pull request Dec 15, 2014
@wickman wickman merged commit edcd370 into pex-tool:master Dec 15, 2014
@jsirois jsirois deleted the jsirois/PythonInterpreter/fix__eq__ branch December 17, 2014 01:13
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.

2 participants