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

ClasspathEntry optionally takes a DirectoryDigest #6297

Merged
merged 3 commits into from Aug 7, 2018

Conversation

4 participants
@illicitonion
Copy link
Contributor

illicitonion commented Aug 3, 2018

This will allow classpath entries to be used to drive remote execution.
Nothing currently produces or consumes this data. But soon it will!

ClasspathEntry optionally takes a DirectoryDigest
This will allow classpath entries to be used to drive remote execution.
Nothing currently produces or consumes this data. But soon it will!

@illicitonion illicitonion added this to To do in Remoting via automation Aug 3, 2018

@illicitonion illicitonion requested review from baroquebobcat and stuhood Aug 3, 2018

@illicitonion illicitonion added this to In progress in Remoting M3 via automation Aug 3, 2018

@illicitonion illicitonion requested a review from dotordogh Aug 3, 2018

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

lgtm!

@@ -9,6 +9,7 @@
from builtins import object, str
from multiprocessing import cpu_count

from six import string_types

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Aug 4, 2018

Contributor

Can this be tightened to either the backported str or bytes? We in general want to avoid string_types as much as possible. It's difficult to design APIs that accept both unicode and byte strings, as we're now finding 😵

This comment has been minimized.

@stuhood

stuhood Aug 4, 2018

Member

The catchall is fine in this case, because it's used for a deprecation. But should he use this one, or from future.utils import string_types?

@stuhood

stuhood approved these changes Aug 4, 2018

@@ -9,6 +9,7 @@
from builtins import object, str
from multiprocessing import cpu_count

from six import string_types

This comment has been minimized.

@stuhood

stuhood Aug 4, 2018

Member

The catchall is fine in this case, because it's used for a deprecation. But should he use this one, or from future.utils import string_types?

@Eric-Arellano

This comment has been minimized.

Copy link
Contributor

Eric-Arellano commented Aug 4, 2018

Six and future.utils have an equivalent implementation for string_types, so it doesn’t technically matter here.

But as a general policy it’s better to use future. Six has some problematic parts, like six.StringIO. Rather than having to remember which are safe to use and which aren’t, it’s easier to always use future.

@illicitonion illicitonion merged commit eac5ecf into pantsbuild:master Aug 7, 2018

1 check passed

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

Remoting automation moved this from To do to Done Aug 7, 2018

Remoting M3 automation moved this from In progress to Done Aug 7, 2018

@illicitonion illicitonion deleted the twitter:dwagnerhall/remotezinc/classpathentries branch Aug 7, 2018

CMLivingston pushed a commit to CMLivingston/pants that referenced this pull request Aug 27, 2018

ClasspathEntry optionally takes a DirectoryDigest (pantsbuild#6297)
This will allow classpath entries to be used to drive remote execution.
Nothing currently produces or consumes this data. But soon it will!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment