-
-
Notifications
You must be signed in to change notification settings - Fork 646
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
Task to gather local python sources into a pex. #4084
Conversation
round_manager.require_data(PythonInterpreter) | ||
|
||
def execute(self): | ||
targets = self.context.targets(lambda tgt: isinstance(tgt, PythonTarget) or |
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.
The isinstance(tgt, (PythonTarget, Resources))
form still fits in <100 cols and leverages the lib.
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.
Oh yeah! I forgot that was possible. Fixed.
self.context.log.debug(' Dumping sources: {}'.format(tgt)) | ||
for relpath in tgt.sources_relative_to_source_root(): | ||
try: | ||
copy_to_chroot(tgt.target_base, relpath, builder.add_source) |
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.
Perhaps just inline copy_to_chroot
, its overly indirect since there is only ever builder.add_source
passed as add_function
for example.
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.
Good call. This was a leftover from the code I copied it from, which handled old-style resources using copy_to_chroot as well, and I didn't notice I could kill it. Done.
|
||
if getattr(tgt, 'resources', None): | ||
# No one should be on old-style resources any more. And if they are, | ||
# switching to the new python pipeline will be a great opportunity to get fix that. |
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.
Verbage is off, probably just s/get fix/fix/
.
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.
Ooops. Fixed.
Part of the new python pipeline.