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

[rsc] break out metacp-ing jars into a separate job in RscCompile #6538

Merged
merged 6 commits into from Sep 26, 2018

Conversation

Projects
None yet
2 participants
@baroquebobcat
Copy link
Contributor

baroquebobcat commented Sep 20, 2018

Problem

The current implementation re-metacps all of the 3rdparty jars for each target that depends on them.

Solution

Break out a separate Job that is added to the execution graph and metacps jar library targets.

Result

Jars are only metacp'd once. fixes #6515.

Note: There's some further clean up to be done on this before it can land.

@stuhood
Copy link
Member

stuhood left a comment

Before we land this, I think we should incorporate one @rule. I suspect that it would make this somewhat clearer, because the inputs/outputs would be more obvious.

@@ -71,6 +71,13 @@ def defaulted_property(self, target, selector):
prop = prop or selector(ScalaPlatform.global_instance())
return prop

def dependencies_respecting_strict_deps(self, target):

This comment has been minimized.

@stuhood

stuhood Sep 24, 2018

Member

This doesn't use self, but should I think.

This comment has been minimized.

@baroquebobcat

baroquebobcat Sep 24, 2018

Contributor

Yep. Updating it.

return [fast_relpath(c, buildroot) for c in collection]
def f(c, buildroot):
try:
return fast_relpath(c, buildroot)

This comment has been minimized.

@stuhood

stuhood Sep 24, 2018

Member

Prefer fast_relpath_optional to catching.

This comment has been minimized.

@baroquebobcat

baroquebobcat Sep 24, 2018

Contributor

Nice. Thanks.

# Include the current machine's jdk lib jars. This'll blow up remotely.
# We need a solution for that.
# Probably something to do with https://github.com/pantsbuild/pants/pull/6346
# TODO perhaps determine the platform of the jar and use that here.

This comment has been minimized.

@stuhood

stuhood Sep 24, 2018

Member

This should use @wisechengyi's work from 4d475b3 probably.

This comment has been minimized.

@baroquebobcat

baroquebobcat Sep 24, 2018

Contributor

yes. I'd thought I'd broken out an issue for that, but I didn't find one. Filed #6547


metai_classpath = self._collect_metai_classpath(
metacp_result, classpath_rel, jvm_lib_jars_abs)

This comment has been minimized.

@stuhood

stuhood Sep 24, 2018

Member

Before we land this, I think we should incorporate at least one @rule to begin weaning ourselves off of the use of the execution graph.

This comment has been minimized.

@baroquebobcat

baroquebobcat Sep 24, 2018

Contributor

I'd prefer to have introducing @rule be a separate PR from this. I could bail on this change and start an @rule conversion before landing it, or I could fix this up and land it and then introduce a migration towards using @rules.

This comment has been minimized.

@stuhood

stuhood Sep 24, 2018

Member

@baroquebobcat : I agree that getting this green and landable and then working on @rules in a separate commit would be a good idea. Depending on how that goes, you can either land them as separate PRs, or land it here.

@stuhood
Copy link
Member

stuhood left a comment

Looking again at the overall structure of the work_for_vts_rsc method, I agree that we should use a separate PR to migrate to @rules, since it will likely involve a fair amount of code movement.

Feel free to land once this is green.

@baroquebobcat baroquebobcat merged commit 2be0b1d into pantsbuild:master Sep 26, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment