-
-
Notifications
You must be signed in to change notification settings - Fork 636
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
add the target fingerprint to the version of each local dist so that we don't use the first cached one #6022
Changes from all commits
b4c2eef
ad30c54
443aa29
f90c295
af6d90f
36d5174
7efdbfb
f4b5156
a3b9610
bd0603d
bb26f29
87b3524
3a199e5
fd322dd
d877cf8
5bbc660
e2eda1c
810f8b8
57093ee
356659d
4eb92ae
c9c4c8b
7041397
5f108db
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
import c_greet | ||
import cpp_greet | ||
|
||
|
||
def hello(): | ||
return '\n'.join([ | ||
c_greet.c_greet(), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,5 +4,6 @@ | |
|
||
from __future__ import absolute_import, division, print_function, unicode_literals | ||
|
||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can leave this file untouched. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in a7d2f23! |
||
def hello(): | ||
return 'Hello, world!' |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,18 +4,15 @@ | |
|
||
from __future__ import absolute_import, division, print_function, unicode_literals | ||
|
||
from builtins import str | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why did you remove this? Is it unused now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, because the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, sounds good. I don't know of any linter that checks for use of the We also unfortunately can't start running The ideal linter would check for any usages of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Made into #6157! |
||
|
||
from pex.interpreter import PythonIdentity | ||
from twitter.common.collections import maybe_list | ||
|
||
from pants.backend.python.targets.python_target import PythonTarget | ||
from pants.base.exceptions import TargetDefinitionException | ||
from pants.base.payload import Payload | ||
from pants.base.payload_field import PrimitiveField | ||
from pants.build_graph.target import Target | ||
|
||
|
||
class PythonDistribution(Target): | ||
class PythonDistribution(PythonTarget): | ||
"""A Python distribution target that accepts a user-defined setup.py.""" | ||
|
||
default_sources_globs = '*.py' | ||
|
@@ -28,7 +25,6 @@ def __init__(self, | |
address=None, | ||
payload=None, | ||
sources=None, | ||
compatibility=None, | ||
setup_requires=None, | ||
**kwargs): | ||
""" | ||
|
@@ -39,31 +35,23 @@ def __init__(self, | |
:type payload: :class:`pants.base.payload.Payload` | ||
:param sources: Files to "include". Paths are relative to the | ||
BUILD file's directory. | ||
:type sources: ``Fileset`` or list of strings. Must include setup.py. | ||
:param compatibility: either a string or list of strings that represents | ||
interpreter compatibility for this target, using the Requirement-style | ||
format, e.g. ``'CPython>=3', or just ['>=2.7','<3']`` for requirements | ||
agnostic to interpreter class. | ||
:type sources: :class:`twitter.common.dirutil.Fileset` or list of strings. Must include | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that this changes the behaviour of sources, you might want to merge from master and re-test post #5908 |
||
setup.py. | ||
:param list setup_requires: A list of python requirements to provide during the invocation of | ||
setup.py. | ||
""" | ||
if not 'setup.py' in sources: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why strip these out? Are you planning on adding setup.py checking back or is it happening elsewhere? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be the exact same as the previous target, just without unnecessarily repeated code, which I think should be a goal. It doesn't seem like too gross of a refactoring because it's limited to a single file. We call the superclass constructor ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I remember vaguely that we don't subclass There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed on the redundancy, but I think that PythonTarget will not check for setup.py presence as of right now, so it would be good to add that back in to this one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand -- we are still checking if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, weird, I must have read this wrong. This looks good to me regarding setup.py checking. |
||
raise TargetDefinitionException( | ||
self, | ||
'A file named setup.py must be in the same ' | ||
'directory as the BUILD file containing this target.') | ||
|
||
payload = payload or Payload() | ||
payload.add_fields({ | ||
'sources': self.create_sources_field(sources, address.spec_path, key_arg='sources'), | ||
'compatibility': PrimitiveField(maybe_list(compatibility or ())), | ||
'setup_requires': PrimitiveField(maybe_list(setup_requires or ())) | ||
}) | ||
super(PythonDistribution, self).__init__(address=address, payload=payload, **kwargs) | ||
|
||
if not 'setup.py' in sources: | ||
raise TargetDefinitionException( | ||
self, 'A setup.py in the top-level directory relative to the target definition is required.' | ||
) | ||
|
||
# Check that the compatibility requirements are well-formed. | ||
for req in self.payload.compatibility: | ||
try: | ||
PythonIdentity.parse_requirement(req) | ||
except ValueError as e: | ||
raise TargetDefinitionException(self, str(e)) | ||
super(PythonDistribution, self).__init__( | ||
address=address, payload=payload, sources=sources, **kwargs) | ||
|
||
@property | ||
def has_native_sources(self): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can leave this file untouched.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a7d2f23!