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 BinaryTool subsystems for node and yarnpkg. #5584

Merged
merged 3 commits into from Mar 14, 2018

Conversation

Projects
None yet
3 participants
@benjyw
Copy link
Contributor

benjyw commented Mar 10, 2018

Previously those two were mashed together in a single subsystem.
The new subsystems each use the BinaryTool mechanism.

New BinaryTool subsystems for node and yarnpkg.
Previously those two were mashed together in a single subsystem.
The new subsystems each use the BinaryTool mechanism.

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

@@ -144,23 +131,22 @@ def install_node(self):
:returns: The Node distribution bin path.
:rtype: string
"""
node_package_path = self.unpack_package(
supportdir=self._supportdir, version=self.version, filename='node.tar.gz')
node_package_path = self.select()

This comment has been minimized.

@UnrememberMe

UnrememberMe Mar 12, 2018

Contributor

should we have context here?

This comment has been minimized.

@benjyw

benjyw Mar 13, 2018

Contributor

No, because we didn't set replaces_scope/replaces_name, so it's not needed. And the fewer places we plumb it through the fewer places we have to remove it from when the deprecation is over.


logger = logging.getLogger(__name__)


class NodeDistribution(object):
class NodeDistribution(NativeTool):

This comment has been minimized.

@UnrememberMe

UnrememberMe Mar 12, 2018

Contributor

We should preserve TGZ.extract(tarball_filepath, work_dir, concurrency_safe=True) functionality that move the extracted tar ball atomically. Refer to #5248

This comment has been minimized.

@benjyw

benjyw Mar 13, 2018

Contributor

The base class takes care of this.

This comment has been minimized.

@benjyw

benjyw Mar 13, 2018

Contributor

Or at least it was supposed to, let me check.

This comment has been minimized.

@benjyw

benjyw Mar 13, 2018

Contributor

Hmm, looks like it didn't. Good catch! Fixed.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Mar 14, 2018

Ping.

@stuhood
Copy link
Member

stuhood left a comment

Thanks Benjy.

@benjyw benjyw merged commit 84422c3 into pantsbuild:master Mar 14, 2018

1 check passed

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

@benjyw benjyw deleted the benjyw:node_binary_tool2 branch Mar 14, 2018

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