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

Store and populate DirectoryDigests for cached targets #6504

Merged
merged 18 commits into from Sep 21, 2018

Conversation

Projects
None yet
3 participants
@dotordogh
Copy link
Contributor

dotordogh commented Sep 14, 2018

For #6429.

@dotordogh dotordogh requested review from stuhood and illicitonion Sep 14, 2018

@@ -716,6 +725,10 @@ def work_for_vts(vts, ctx):
compiler_option_sets,
zinc_file_manager,
counter)

# This might be missing the analysis file?
ctx.classes_dir = ClasspathEntry(ctx.classes_dir.path, directory_digest)

This comment has been minimized.

@dotordogh

dotordogh Sep 14, 2018

Contributor

@illicitonion: From what I can tell by following the code through, this won't contain the analysis file. Is that an issue? Wouldn't my test have failed if it was?

This comment has been minimized.

@stuhood

stuhood Sep 14, 2018

Member

If the analysis file is missing, zinc will execute a clean compile (ie, it will not execute an incremental compile), so it will still succeed.

Honestly, it would probably be totally reasonable to require --no-compile-zinc-incremental when the hermetic strategy is in use (for now, with TODOs), and then never include analysis in input snapshots (to force them to always be clean compiles). You would still capture output analysis from a compile, and you'd still check it out on disk in the relevant place, because that would put it in the build cache.

This comment has been minimized.

@illicitonion

illicitonion Sep 17, 2018

Contributor

We currently capture the analysis cache in the snapshot; see src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py line 434

@stuhood raises a reasonable point that we should probably either never capture the analysis file, or only capture it if the flag is set to say we should, though.

(It's also super hacky that we capture the analysis file in the same snapshot as the classpath entry, but it's a lot more convenient than trying to capture it in a separate snapshot)

@stuhood
Copy link
Member

stuhood left a comment

Would suggest talking the incremental compile issue through with @illicitonion before landing, but this looks good.

"""Adds classpath path elements to the products of the provided target.
:param target: The target for which to add the classpath elements.
:param classpath_elements: List of strings or pants.backend.jvm.tasks.ClasspathEntry

This comment has been minimized.

@stuhood

stuhood Sep 14, 2018

Member

It appears from wrap_path_elements that rather than a list of strings, this would be a list of tuples (probably (conf, filename), if I remember correctly).

@@ -470,6 +473,15 @@ def _record_compile_classpath(self, classpath, target, outdir):
with open(path, 'w') as f:
f.write(text)

def _get_directory_digest_of_dependencies(self, compile_context):

This comment has been minimized.

@stuhood

stuhood Sep 14, 2018

Member

This will snapshot the target itself, but not its dependencies. So would consider renaming this method.

But this is also related to the other comment about analysis files: there is conditional logic to either include/ignore analysis files in the if not is_incremental: block elsewhere in this file. We implement disabling incremental compiles by not passing an analysis file to zinc.

@@ -716,6 +725,10 @@ def work_for_vts(vts, ctx):
compiler_option_sets,
zinc_file_manager,
counter)

# This might be missing the analysis file?
ctx.classes_dir = ClasspathEntry(ctx.classes_dir.path, directory_digest)

This comment has been minimized.

@stuhood

stuhood Sep 14, 2018

Member

If the analysis file is missing, zinc will execute a clean compile (ie, it will not execute an incremental compile), so it will still succeed.

Honestly, it would probably be totally reasonable to require --no-compile-zinc-incremental when the hermetic strategy is in use (for now, with TODOs), and then never include analysis in input snapshots (to force them to always be clean compiles). You would still capture output analysis from a compile, and you'd still check it out on disk in the relevant place, because that would put it in the build cache.

@illicitonion
Copy link
Contributor

illicitonion left a comment

Looking great, thanks!

wrapped_path_elements = []
for element in classpath_elements:
if isinstance(element[1], ClasspathEntry):
wrapped_path_elements.append(element)

This comment has been minimized.

@illicitonion

illicitonion Sep 17, 2018

Contributor

Maybe raise if len(element) != 2

@@ -395,8 +396,8 @@ def execute(self):
for ccs in compile_contexts.values():
cc = self.select_runtime_context(ccs)
for conf in self._confs:
classpath_product.remove_for_target(cc.target, [(conf, cc.classes_dir)])
classpath_product.add_for_target(cc.target, [(conf, cc.jar_file)])
classpath_product.remove_for_target(cc.target, [(conf, cc.classes_dir.path)])

This comment has been minimized.

@illicitonion

illicitonion Sep 17, 2018

Contributor

I suspect this code doesn't actually remove any more, because it removes the passed value from the set, rather than doing a path check; I think remove_for_target needs to iterate across the products, and remove the one with the matching path...

This comment has been minimized.

@dotordogh

dotordogh Sep 18, 2018

Contributor

Did this in the body of remove_for_target.

def _get_directory_digest_of_dependencies(self, compile_context):
relative_classes_dir = fast_relpath(compile_context.classes_dir.path, get_buildroot())
relative_analysis_file = fast_relpath(compile_context.analysis_file, get_buildroot())
snapshot = self.context._scheduler.capture_snapshots((PathGlobsAndRoot(

This comment has been minimized.

@illicitonion

illicitonion Sep 17, 2018

Contributor

Let's batch these snapshot captures, so that we do all of the targets in one invocation, rather than doing one per loop iteration; just like in the other PR :)

@@ -716,6 +725,10 @@ def work_for_vts(vts, ctx):
compiler_option_sets,
zinc_file_manager,
counter)

# This might be missing the analysis file?
ctx.classes_dir = ClasspathEntry(ctx.classes_dir.path, directory_digest)

This comment has been minimized.

@illicitonion

illicitonion Sep 17, 2018

Contributor

We currently capture the analysis cache in the snapshot; see src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py line 434

@stuhood raises a reasonable point that we should probably either never capture the analysis file, or only capture it if the flag is set to say we should, though.

(It's also super hacky that we capture the analysis file in the same snapshot as the classpath entry, but it's a lot more convenient than trying to capture it in a separate snapshot)

def test_hermetic_binary_cache_with_dependencies(self):
build_file_abs_path = os.path.join(get_buildroot(), 'examples/src/scala/org/pantsbuild/example/hello/exe/BUILD')

with open(build_file_abs_path, 'r') as f:

This comment has been minimized.

@illicitonion

illicitonion Sep 17, 2018

Contributor

It would be nice to extract this with-read-try-finally-write into a context manager in src/python/pants/util/contextutil.py - maybe called with_overwriten_file_content or similar?

)
self.assert_success(pants_run)
self.assertIn(
'Num args passed: 0. Stand by for welcome...\nHello, Resource World!',

This comment has been minimized.

@illicitonion

illicitonion Sep 17, 2018

Contributor

It would be good to have the modification to source change this message, rather than changing the BUILD file; right now, if you remove the self.assert_success call, and revert your non-test code changes, this text actually still gets output - I'm a little scared that the assert_success is the only safety net we have here, whereas if we replaced Num with Number or something, we'd see that some re-compilation definitely happened.

This comment has been minimized.

@dotordogh

dotordogh Sep 18, 2018

Contributor

Did this in the following assert.

@illicitonion
Copy link
Contributor

illicitonion left a comment

Thanks! :)

wrapped_path_elements = []
for element in classpath_elements:
if len(element) != 2:
raise ValueError('Input must be a list of tuples containing two elements.')

This comment has been minimized.

@illicitonion

illicitonion Sep 19, 2018

Contributor

I think this isn't quite in the right place... The cases are:

  • len == 2 and is ClasspathEntry
  • len == 2 and is string
  • len == 3 and are string and DirectoryDigest (we maybe want to no longer support this, and just update these callers to use ClasspathEntry directly)

so we either need to move this check to be nested in the isinstance if, or we should change line 344 to just do ClasspathEntry(element[1])) rather than ClasspathEntry(*element[1:])

This comment has been minimized.

@dotordogh

dotordogh Sep 19, 2018

Contributor

We no longer support the third case but I didn't want to remove the *element[1:] just in case because it does the same thing as element[1] when we throw an error if element doesn't have a length of two, but I will remove it.

file_abs_path = os.path.join(get_buildroot(),
'examples/src/scala/org/pantsbuild/example/hello/exe/Exe.scala')

def run_then_overwrite_file_then_run_again(file_path):

This comment has been minimized.

@illicitonion

illicitonion Sep 19, 2018

Contributor

I think this may be a little more clear as:

do initial run...
with temporarily_overwritten_file_content(file_abs_path, new_temp_target):
  do next run...

which I think should work if instead of taking a method_to_run and calling it, you do the file write, and then just yield where you currently have method_to_run(file_path)

@dotordogh dotordogh merged commit ceec385 into pantsbuild:master Sep 21, 2018

1 check passed

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

@dotordogh dotordogh deleted the dotordogh:dotordogh/6429 branch Sep 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment