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

New-style BinaryTool Subsystems for isort and go distribution. #5523

Merged
merged 4 commits into from Feb 28, 2018

Conversation

Projects
None yet
2 participants
@benjyw
Copy link
Contributor

benjyw commented Feb 26, 2018

Best to review each commit separately. They are submitted together to expedite progress.

benjyw added some commits Feb 26, 2018

Retcon the go distribution subsystem to use the new mechanism.
Gets rid of the indirection via a Factory. It's easier to just
make the distribution itself the subsystem, since the base class
takes care of basically everything the factory previously did.

@benjyw benjyw requested review from jsirois and cosmicexplorer Feb 26, 2018

options_scope = 'go-distribution'
name = 'go'
default_version = '1.8.3'
archive_type = 'tgz'

This comment has been minimized.

@jsirois

jsirois Feb 26, 2018

Member

Looks like maybe this bit is naive in the face of .tgz vs .tar.gz: https://github.com/pantsbuild/pants/blob/master/src/python/pants/binaries/binary_util.py#L180

This comment has been minimized.

@jsirois

jsirois Feb 26, 2018

Member

I should have linked the CI test failure - this was looking like a bug in the underlying extraction process.

This comment has been minimized.

@jsirois

jsirois Feb 26, 2018

Member

Ah, did not read the backtrace closely. The extraction blowup is in GoDistribution code using TGZ directly.

This comment has been minimized.

@benjyw

benjyw Feb 27, 2018

Contributor

Yeah, took me a moment to figure it out, but the extension thing doesn't matter - it's just how we generate a name for the dir so it doesn't collide with the name of the downloaded file. The issue was I forgot to remove the unpacking code in GoDistribution, since BinaryTool now does this for us...

benjyw added some commits Feb 26, 2018

Don't attempt to unpack the already-unpacked distribution.
Also improved the fetching code to use a proper tmpdir instead
of a hardcoded name that might cause contention.
TGZ.extract(go_distribution, tmp_dist)
os.rename(tmp_dist, outdir)
return os.path.join(outdir, 'go')
#go_distribution = self.select()

This comment has been minimized.

@jsirois

jsirois Feb 27, 2018

Member

Commented out code can be killed.

This comment has been minimized.

@benjyw

benjyw Feb 28, 2018

Contributor

Whoops!

@@ -176,9 +176,10 @@ def _select_archive(self, supportdir, version, name, platform_dependent, archive
"""
full_name = '{}.{}'.format(name, archiver.extension)
downloaded_file = self._select_file(supportdir, version, full_name, platform_dependent)
# use filename without extension as the directory name
# use filename without rightmost extension as the directory name.

This comment has been minimized.

@jsirois

jsirois Feb 27, 2018

Member

s/u/U/ while you're sentencing

This comment has been minimized.

@benjyw

benjyw Feb 28, 2018

Contributor

Hoisted by my own petard!

@@ -237,29 +238,29 @@ def _fetch_binary(self, name, binary_path):
bootstrap_dir = os.path.realpath(os.path.expanduser(self._pants_bootstrapdir))
bootstrapped_binary_path = os.path.join(bootstrap_dir, binary_path)
if not os.path.exists(bootstrapped_binary_path):
downloadpath = bootstrapped_binary_path + '~'
try:
safe_mkdir_for(bootstrapped_binary_path)

This comment has been minimized.

@jsirois

jsirois Feb 27, 2018

Member

I think you solved this already: with safe_concurrent_creation(bootstrapped_binary_path) as downloadpath:

This comment has been minimized.

@benjyw

benjyw Feb 28, 2018

Contributor

Right! I forgot about that somehow. Done.

digest = digest or hashlib.sha1()

if os.path.isfile(filepath) and checksum:
return hash_file(filepath, digest=digest) == checksum

return os.path.isfile(filepath)

def is_bin_valid(self, basepath, binary_file_specs=[]):
def is_bin_valid(self, basepath, binary_file_specs=tuple()):

This comment has been minimized.

@jsirois

jsirois Feb 27, 2018

Member

If using tuple() instead of () was deliberate, no beef, but it was slightly surprising to read.

This comment has been minimized.

@benjyw

benjyw Feb 28, 2018

Contributor

I think I forgot that () is a valid literal. Changed.

@@ -89,14 +61,14 @@ class GoCommand(namedtuple('GoCommand', ['cmdline', 'env'])):
"""Encapsulates a go command that can be executed."""

@classmethod
def _create(cls, goroot, cmd, go_env, args=None):
def create(cls, goroot, cmd, go_env, args=None):

This comment has been minimized.

@jsirois

jsirois Feb 27, 2018

Member

Not a big objection, but it's not used publically fwict - perhaps you have plans?

This comment has been minimized.

@benjyw

benjyw Feb 28, 2018

Contributor

No plans, I think my IDE didn't like accessing a private member of another class. But I guess it's fine if the class is a member of the same class as the code is in. I changed it back.

@benjyw benjyw merged commit 3bdd550 into pantsbuild:master Feb 28, 2018

1 check passed

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

@benjyw benjyw deleted the benjyw:isort_tool branch Feb 28, 2018

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