Skip to content
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-compile] define workflow in context instead of on fly; use workf… #7324

Merged
merged 5 commits into from Mar 15, 2019
Merged
Changes from 3 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -668,7 +668,8 @@ def format_length(self):
# Invalidated targets are a subset of relevant targets: get the context for this one.
compile_target = ivts.target
invalid_dependencies = self._collect_invalid_compile_dependencies(compile_target,
invalid_target_set)
invalid_target_set,
compile_contexts)

jobs.extend(
self.create_compile_jobs(compile_target, compile_contexts, invalid_dependencies, ivts,
@@ -745,7 +746,8 @@ def record(k, v):
record('sources_len', sources_len)
record('incremental', is_incremental)

def _collect_invalid_compile_dependencies(self, compile_target, invalid_target_set):
def _collect_invalid_compile_dependencies(self, compile_target, invalid_target_set,
compile_contexts):
# Collects all invalid dependencies that are not dependencies of other invalid dependencies
# within the closure of compile_target.
invalid_dependencies = OrderedSet()
@@ -758,13 +760,13 @@ def predicate(target):
return True
if target in invalid_target_set:
invalid_dependencies.add(target)
return self._on_invalid_compile_dependency(target, compile_target)
return self._on_invalid_compile_dependency(target, compile_target, compile_contexts)
return True

compile_target.walk(work, predicate)
return invalid_dependencies

def _on_invalid_compile_dependency(self, dep, compile_target):
def _on_invalid_compile_dependency(self, dep, compile_target, compile_contexts):
"""Decide whether to continue searching for invalid targets to use in the execution graph.
By default, don't recurse because once we have an invalid dependency, we can rely on its
@@ -125,6 +125,9 @@ def classify_target(cls, target):
target_type = None
return target_type

_JvmCompileWorkflowType.rsc_then_zinc = _JvmCompileWorkflowType('rsc-then-zinc')

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 12, 2019

Contributor

This should be unnecessary post #7269!

This comment has been minimized.

Copy link
@baroquebobcat

baroquebobcat Mar 13, 2019

Author Contributor

Oh, so if I changed the type names to underscore separated, then those class attrs would just work?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 14, 2019

Contributor

So, that should be true, but also, I got the PR number wrong -- as of #7304, you should be able to keep them separated by hyphens (the recommended convention), and have the attributes generated with underscores!

_JvmCompileWorkflowType.zinc_only = _JvmCompileWorkflowType('zinc-only')


class RscCompileContext(CompileContext):
def __init__(self,
@@ -135,9 +138,11 @@ def __init__(self,
jar_file,
log_dir,
zinc_args_file,
sources):
sources,
workflow):
super(RscCompileContext, self).__init__(target, analysis_file, classes_dir, jar_file,
log_dir, zinc_args_file, sources)
self.workflow = workflow
self.rsc_jar_file = rsc_jar_file

def ensure_output_dirs_exist(self):
@@ -226,9 +231,8 @@ def confify(entries):
# Ensure that the jar/rsc jar is on the rsc_classpath.
for target in targets:
rsc_cc, compile_cc = compile_contexts[target]
target_compile_type = self._classify_compile_target(target)
if target_compile_type is not None:
cp_entries = target_compile_type.resolve_for_enum_variant({
if rsc_cc.workflow is not None:
cp_entries = rsc_cc.workflow.resolve_for_enum_variant({
'zinc-only': lambda : confify([compile_cc.jar_file]),
'rsc-then-zinc': lambda : confify(
to_classpath_entries([rsc_cc.rsc_jar_file], self.context._scheduler)),
@@ -253,27 +257,25 @@ def create_empty_extra_products(self):
def select(self, target):
if not isinstance(target, JvmTarget):
return False
if self._classify_compile_target(target) is not None:
return True
return False
return target.has_sources('.java') or target.has_sources('.scala')

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 12, 2019

Contributor

I would still like it if we were to use self._classify_compile_target() here instead of having more one-off conditionals, it means there's one place to check if errors occur.

This comment has been minimized.

Copy link
@baroquebobcat

baroquebobcat Mar 13, 2019

Author Contributor

Hm. That's a fair point.


def _classify_compile_target(self, target):
return _JvmCompileWorkflowType.classify_target(target)

def _key_for_target_as_dep(self, target):
def _key_for_target_as_dep(self, target, workflow):
# used for jobs that are either rsc jobs or zinc jobs run against rsc
return self._classify_compile_target(target).resolve_for_enum_variant({
'zinc-only': lambda: self._zinc_key_for_target(target),
return workflow.resolve_for_enum_variant({
'zinc-only': lambda: self._zinc_key_for_target(target, workflow),
'rsc-then-zinc': lambda: self._rsc_key_for_target(target),
})()

def _rsc_key_for_target(self, compile_target):
return 'rsc({})'.format(compile_target.address.spec)
def _rsc_key_for_target(self, target):
return 'rsc({})'.format(target.address.spec)

def _zinc_key_for_target(self, compile_target):
return self._classify_compile_target(compile_target).resolve_for_enum_variant({
'zinc-only': lambda: 'zinc({})'.format(compile_target.address.spec),
'rsc-then-zinc': lambda: 'zinc_against_rsc({})'.format(compile_target.address.spec),
def _zinc_key_for_target(self, target, workflow):
return workflow.resolve_for_enum_variant({
'zinc-only': lambda: 'zinc({})'.format(target.address.spec),
'rsc-then-zinc': lambda: 'zinc_against_rsc({})'.format(target.address.spec),
})()

def create_compile_jobs(self,
@@ -384,9 +386,10 @@ def all_zinc_rsc_invalid_dep_keys(invalid_deps):
for tgt in invalid_deps:
# None can occur for e.g. JarLibrary deps, which we don't need to compile as they are
# populated in the resolve goal.
if self._classify_compile_target(tgt) is not None:
tgt_rsc_cc, tgt_z_cc = compile_contexts[tgt]
if tgt_rsc_cc.workflow is not None:
# Rely on the results of zinc compiles for zinc-compatible targets
yield self._key_for_target_as_dep(tgt)
yield self._key_for_target_as_dep(tgt, tgt_rsc_cc.workflow)

def make_rsc_job(target, dep_targets):
return Job(
@@ -403,12 +406,13 @@ def make_rsc_job(target, dep_targets):

def only_zinc_invalid_dep_keys(invalid_deps):
for tgt in invalid_deps:
if self._classify_compile_target(tgt) is not None:
yield self._zinc_key_for_target(tgt)
rsc_cc_tgt, zinc_cc_tgt = compile_contexts[tgt]
if rsc_cc_tgt.workflow is not None:
yield self._zinc_key_for_target(tgt, rsc_cc_tgt.workflow)

def make_zinc_job(target, input_product_key, output_products, dep_keys):
return Job(
key=self._zinc_key_for_target(target),
key=self._zinc_key_for_target(target, rsc_compile_context.workflow),
fn=functools.partial(
self._default_work_for_vts,
ivts,
@@ -424,7 +428,7 @@ def make_zinc_job(target, input_product_key, output_products, dep_keys):

# Create the rsc job.
# Currently, rsc only supports outlining scala.
workflow = self._classify_compile_target(compile_target)
workflow = rsc_compile_context.workflow
workflow.resolve_for_enum_variant({
'zinc-only': lambda: None,
'rsc-then-zinc': lambda: rsc_jobs.append(make_rsc_job(compile_target, invalid_dependencies)),
@@ -489,6 +493,7 @@ def create_compile_context(self, target, target_workdir):
rsc_jar_file=os.path.join(rsc_dir, 'm.jar'),
log_dir=os.path.join(rsc_dir, 'logs'),
sources=sources,
workflow=self._classify_compile_target(target),
),
CompileContext(
target=target,
@@ -607,17 +612,20 @@ def _jdk_libs_paths_and_digest(self, hermetic_dist):
def _jdk_libs_abs(self, nonhermetic_dist):
return nonhermetic_dist.find_libs(self._JDK_LIB_NAMES)

def _on_invalid_compile_dependency(self, dep, compile_target):
def _on_invalid_compile_dependency(self, dep, compile_target, contexts):
"""Decide whether to continue searching for invalid targets to use in the execution graph.
If a necessary dep is a Scala dep and the root is Java, continue to recurse because
otherwise we'll drop the path between Zinc compile of the Java target and a Zinc
compile of a transitive Scala dependency.
If a necessary dep is a Rsc compiled dep and the root is a Zinc one, continue to recurse because

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 12, 2019

Contributor

The terms "rsc-compatible" and "zinc-only" seem useful here to distinguish between compiler invocations (involving both rsc and zinc) and target types (strictly one or the other).

This comment has been minimized.

Copy link
@baroquebobcat

baroquebobcat Mar 13, 2019

Author Contributor

oh, as in, use the workflow names instead of plain language? I think that's reasonable.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 14, 2019

Contributor

Yes! But hopefully these distinctions can go away soon enough with further work on rsc compatibility / automatic rewriting.

otherwise we'll drop the path between Zinc compile of the Zinc target and a Zinc
compile of a transitive Rsc dependency.
This is only an issue for graphs like J -> S1 -> S2, where J is a Java target,
S1/2 are Scala targets and S2 must be on the classpath to compile J successfully.
This is only an issue for graphs like J -> S1 -> S2, where J is a Zinc target,
S1/2 are Rsc targets and S2 must be on the classpath to compile J successfully.
"""
if dep.has_sources('.scala') and compile_target.has_sources('.java'):
return True
else:
return False
return contexts[compile_target][0].workflow.resolve_for_enum_variant({
'zinc-only': lambda : contexts[dep][0].workflow.resolve_for_enum_variant({
'rsc-then-zinc': True,
'zinc-only': False
}),
'rsc-then-zinc': lambda : False
})()

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 12, 2019

Contributor

After #7269, this could be:

    return contexts[compile_target][0].workflow.resolve_for_enum_variant({
      'zinc-only': lambda : contexts[dep][0].workflow == _JvmCompileWorkflow.rsc_then_zinc,
      'rsc-then-zinc': lambda: False,
    })()

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 12, 2019

Contributor

I'll convert compile_contexts into a datatype in followup PR.

This comment has been minimized.

Copy link
@baroquebobcat

baroquebobcat Mar 13, 2019

Author Contributor

Cool.

This is the bit that I was referring to as a bug fix.

Could you please expand on what this is referring to?
It refers to the fact that this conditional was trying to model different workflows, but when I adjusted how workflows were chosen, I hadn't updated this.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Mar 14, 2019

Contributor

Oh oops, thank you for elaborating!

@@ -7,9 +7,11 @@
import os
from textwrap import dedent

from pants.backend.jvm.subsystems.junit import JUnit
from pants.backend.jvm.subsystems.scala_platform import ScalaPlatform
from pants.backend.jvm.targets.jar_library import JarLibrary
from pants.backend.jvm.targets.java_library import JavaLibrary
from pants.backend.jvm.targets.junit_tests import JUnitTests
from pants.backend.jvm.targets.scala_library import ScalaLibrary
from pants.backend.jvm.tasks.classpath_products import ClasspathProducts
from pants.backend.jvm.tasks.jvm_compile.execution_graph import ExecutionGraph
@@ -78,7 +80,7 @@ def test_no_dependencies_between_scala_and_java_targets(self):
zinc_against_rsc(scala/classpath:scala_lib) -> {}""").strip(),
dependee_graph)

def test_scala_dep_for_scala_and_java_targets(self):
def test_rsc_dep_for_scala_java_and_test_targets(self):
self.init_dependencies_for_scala_libraries()

scala_dep = self.make_target(
@@ -99,10 +101,17 @@ def test_scala_dep_for_scala_and_java_targets(self):
dependencies=[scala_dep]
)

test_target = self.make_target(
'scala/classpath:scala_test',
target_type=JUnitTests,
sources=['com/example/Test.scala'],
dependencies=[scala_target]
)

with temporary_dir() as tmp_dir:
invalid_targets = [java_target, scala_target, scala_dep]
invalid_targets = [java_target, scala_target, scala_dep, test_target]
task = self.create_task_with_target_roots(
target_roots=[java_target, scala_target]
target_roots=[java_target, scala_target, test_target]
)

jobs = task._create_compile_jobs(
@@ -118,15 +127,19 @@ def test_scala_dep_for_scala_and_java_targets(self):
rsc(scala/classpath:scala_lib) -> {
zinc_against_rsc(scala/classpath:scala_lib)
}
zinc_against_rsc(scala/classpath:scala_lib) -> {}
zinc_against_rsc(scala/classpath:scala_lib) -> {
zinc(scala/classpath:scala_test)
}
rsc(scala/classpath:scala_dep) -> {
rsc(scala/classpath:scala_lib),
zinc_against_rsc(scala/classpath:scala_lib),
zinc_against_rsc(scala/classpath:scala_dep)
}
zinc_against_rsc(scala/classpath:scala_dep) -> {
zinc(java/classpath:java_lib)
}""").strip(),
zinc(java/classpath:java_lib),
zinc(scala/classpath:scala_test)
}
zinc(scala/classpath:scala_test) -> {}""").strip(),
dependee_graph)

def test_scala_lib_with_java_sources_not_passed_to_rsc(self):
@@ -175,12 +188,6 @@ def test_scala_lib_with_java_sources_not_passed_to_rsc(self):
zinc(scala/classpath:scala_with_indirect_java_sources) -> {}""").strip(),
dependee_graph)

def construct_dependee_graph_str(self, jobs, task):
exec_graph = ExecutionGraph(jobs, task.get_options().print_exception_stacktrace)
dependee_graph = exec_graph.format_dependee_graph()
print(dependee_graph)
return dependee_graph

def test_desandbox_fn(self):
# TODO remove this after https://github.com/scalameta/scalameta/issues/1791 is released
desandbox = _create_desandboxify_fn(['.pants.d/cool/beans.*', '.pants.d/c/r/c/.*'])
@@ -199,6 +206,12 @@ def test_desandbox_fn(self):
self.assertEqual(desandbox('/some/path/.pants.d/exec-location/.pants.d/tmp.pants.d/cool/beans'),
'.pants.d/tmp.pants.d/cool/beans')

def construct_dependee_graph_str(self, jobs, task):
exec_graph = ExecutionGraph(jobs, task.get_options().print_exception_stacktrace)
dependee_graph = exec_graph.format_dependee_graph()
print(dependee_graph)
return dependee_graph

def wrap_in_vts(self, invalid_targets):
return [LightWeightVTS(t) for t in invalid_targets]

@@ -212,11 +225,19 @@ def init_dependencies_for_scala_libraries(self):
}
}
)
init_subsystem(
JUnit,
)
self.make_target(
'//:scala-library',
target_type=JarLibrary,
jars=[JarDependency(org='com.example', name='scala', rev='0.0.0')]
)
self.make_target(
'//:junit-library',
target_type=JarLibrary,
jars=[JarDependency(org='com.example', name='scala', rev='0.0.0')]
)

def create_task_with_target_roots(self, target_roots):
context = self.context(target_roots=target_roots)
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.