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

Fail for missing classpath entries with hermetic execution. #8037

Conversation

@stuhood
Copy link
Member

commented Jul 11, 2019

Problem

When hermetic execution is used for zinc or rsc and DirectoryDigests are not available for input classpath entries, we currently trigger a warning. But this being only a warning (and not an error) definitely masks a few different potential bugs, and might allow more to slip in in the future.

Solution

Convert the warning into an error, and fix all of the issues exposed by that change:

  • In some Task configurations, ResourcesTask could run before the compilers, meaning that its resources needed to be digested. This also prepares for remote execution of tests.
  • When --no-use-classpath-jars was in use:
    • ClasspathEntrys having digests was preventing them from having an equality match in order to be removed from the classpath.
    • rsc was universally adding the jar for a target to the classpath, even though it was empty.
    • Both the loose classpath and output jar needed to be captured in hermetic execution.
  • Tools that were bootstrapped were not propagating the digests that had already been captured for them in #7835, which caused "global" scalac plugins to not have digests.

Result

Hermetic execution has improved validation that should help to catch additional bugs in the future.

@stuhood stuhood force-pushed the twitter:stuhood/error-for-missing-classpath-entries-with-hermetic branch from 127bc44 to 2623c0a Jul 13, 2019

@stuhood stuhood requested review from illicitonion, cosmicexplorer and ity Jul 14, 2019

@stuhood

This comment has been minimized.

Copy link
Member Author

commented Jul 14, 2019

All the commits are useful to review independently (and roughly correspond to the bullets above).

There are a few ways that this could maybe be broken up into multiple PRs if folks feel that that would be useful: in particular, the bootstrap changes could be broken out as pre-work.

@stuhood stuhood force-pushed the twitter:stuhood/error-for-missing-classpath-entries-with-hermetic branch 2 times, most recently from 479158f to d591307 Jul 15, 2019

@stuhood

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

The shard timeouts are mysterious... not looking forward to debugging those. Oy.

@ity

ity approved these changes Jul 18, 2019

Copy link
Contributor

left a comment

high level, looks good to me - i havent dug into the travis failures.
this will likely conflict with the deprecation patch that I am working on - so I will race you to it :)

return [shaded_jar]
return [self._shaded_jar_as_classpath_entry(shaded_jar)]

def _shaded_jar_as_classpath_entry(self, shaded_jar):

This comment has been minimized.

Copy link
@ity

ity Jul 18, 2019

Contributor

maybe just as_classpath_entry

@illicitonion
Copy link
Contributor

left a comment

Looks great, thanks! It would be good to verify that this doesn't affect noop/incremental-compile performance of large targets before merging.

if not vt.valid:
self.prepare_resources(vt.target, vt.results_dir)
processed_targets.append(vt.target)
vt.update()

This comment has been minimized.

Copy link
@illicitonion

illicitonion Jul 19, 2019

Contributor

Why don't we need the vt.update() call on the invalidated vts any more?

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 27, 2019

Author Member

It's called implicitly at the end of the block: while it would have been fine to leave this here, resource generation is not generally expensive, which is the reason to allow it to fail incrementally by calling vt.update() explicitly.

@@ -66,5 +66,5 @@ def _consolidate_classpath(self, targets, classpath_products):
jar.write(entry.path)

# Replace directory classpath entry with its jarpath.
classpath_products.remove_for_target(vt.target, [(conf, entry.path)])
classpath_products.remove_for_target(vt.target, [(conf, entry)])

This comment has been minimized.

Copy link
@illicitonion

illicitonion Jul 19, 2019

Contributor

It may (probably not) be worth changing ClasspathProducts and UnionProducts to use a dict[path -> ClasspathEntry] instead of set[ClasspathEntry] internally, and have remove_for_target remove ignoring digest... Probably not, but thought I'd mention it as an idea...

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 27, 2019

Author Member

Mm, possibly. The remove case is a strange one, but IMO, always matching on identity is less magical.

@@ -462,11 +462,17 @@ def _compile_hermetic(self, jvm_options, ctx, classes_dir, jar_file,
(self.post_compile_extra_resources_digest(ctx), argfile_snapshot.directory_digest)
)

# NB: We always capture the output jar, but if classpath jars are not used, we additionally

This comment has been minimized.

Copy link
@illicitonion

illicitonion Jul 19, 2019

Contributor

Do we produce an output jar if classpath jars are not used? I would have assumed not?

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 27, 2019

Author Member

We do, because after compilation, we use a jar on the classpath 100% of the time. This is because runtime classloading (for tests) is drastically (hundreds of times) faster with jars than with loose directories.

@@ -123,7 +123,7 @@ def _temporary_buildroots(self, files_to_copy=None, current_root=None, iteration
def test_config_buildroot_does_not_invalidate_targets(self, cache_args):
previous_names = set()
for buildroot in self._temporary_buildroots(['examples']):
with self.temporary_workdir() as workdir:
with temporary_dir(root_dir=buildroot, prefix='.pants.d', suffix='.pants.d') as workdir:

This comment has been minimized.

Copy link
@illicitonion

illicitonion Jul 19, 2019

Contributor

Prefix and suffix?

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 27, 2019

Author Member

Whoops. That's not really necessary.

:rtype: str
:rtype: ClasspathEntry

This comment has been minimized.

Copy link
@illicitonion

illicitonion Jul 19, 2019

Contributor

I don't know how public we view this API - I'm fine with this change, but others may not be?

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 27, 2019

Author Member

It's not marked :API: public, so should be good.

@stuhood

This comment has been minimized.

Copy link
Member Author

commented Jul 24, 2019

I'm crossing my fingers that the timeouts are due to bumping the implementation version of bootstrap (since the total amount of work we're doing has not changed at all) and running this another time.

@stuhood stuhood force-pushed the twitter:stuhood/error-for-missing-classpath-entries-with-hermetic branch from ece5ff0 to bfbe365 Jul 25, 2019

@stuhood

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

While staring at Shard 0, I noticed #8108: so that's good at least!

@stuhood stuhood force-pushed the twitter:stuhood/error-for-missing-classpath-entries-with-hermetic branch 2 times, most recently from 25958bc to 86c2ae9 Jul 25, 2019

@stuhood stuhood force-pushed the twitter:stuhood/error-for-missing-classpath-entries-with-hermetic branch 4 times, most recently from 8fa8e25 to 85151b4 Jul 27, 2019

@stuhood stuhood force-pushed the twitter:stuhood/error-for-missing-classpath-entries-with-hermetic branch from 85151b4 to 4444902 Jul 28, 2019

@stuhood stuhood merged commit d45af28 into pantsbuild:master Jul 28, 2019

1 check passed

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

@stuhood stuhood deleted the twitter:stuhood/error-for-missing-classpath-entries-with-hermetic branch Jul 28, 2019

cosmicexplorer pushed a commit that referenced this pull request Aug 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.