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

Zinc compiles can execute hermetically #6351

Merged
merged 2 commits into from Sep 1, 2018

Conversation

Projects
None yet
3 participants
@illicitonion
Copy link
Contributor

illicitonion commented Aug 15, 2018

There are a LOT of TODOs here, but it fundamentally works.

Notable things which are missing:

  • Cached vts won't provide files on the classpath - #6429
  • Rebase map is not set up properly - #6434
  • use-classpath-jars doesn't work
  • workdir must be a child of buildroot
  • classpath entries in the java dist don't work
  • Upstream analysis may or may not work properly in all cases
  • -zinc-cache-dir is in a homedir, so won't be written if that homedir
    doesn't exist - #6155
  • Many things (e.g. jar_library) don't populate ClasspathEntry
    DirectoryDigests - #6432
  • A big pile of inefficiencies

@illicitonion illicitonion requested review from baroquebobcat , stuhood and ity Aug 15, 2018

@illicitonion illicitonion requested a review from dotordogh Aug 29, 2018

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Aug 29, 2018

Ping? :)

@stuhood stuhood added this to the 1.10.x milestone Aug 29, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks! Very sorry for the long-delayed review.

It would be good to link some of these TODOs to github issues.

))[0]
return self._snapshot

# TODO: Relativise (and sort)

This comment has been minimized.

@stuhood

stuhood Aug 29, 2018

Member

By definition, it is not possible to relativise these: they need to align with the host executing the zinc process.

Fixing this will require moving discovery of the JDK home into zinc, most likely.

@@ -207,6 +209,20 @@ def __init__(self, *args, **kwargs):
# Validate zinc options.
ZincCompile.validate_arguments(self.context.log, self.get_options().whitelisted_args,
self._args)
if self.execution_strategy == self.HERMETIC:
if fast_relpath(self.get_options().pants_workdir, get_buildroot()).startswith('..'):

This comment has been minimized.

@stuhood

stuhood Aug 29, 2018

Member

fast_relpath eagerly fails if arg1 does not contain arg2, so it will never result in a .. prefix.

@@ -284,10 +300,13 @@ def compile(self, ctx, args, dependency_classpath, upstream_analysis,
if self.get_options().capture_classpath:
self._record_compile_classpath(absolute_classpath, ctx.target, ctx.classes_dir)

self._verify_zinc_classpath(absolute_classpath)
self._verify_zinc_classpath(list(upstream_analysis.keys()))
# TODO: Allow use of absolute classpath entries with hermetic execution, specifically by putting in $JAVA_HOME placeholders

This comment has been minimized.

@stuhood

stuhood Aug 29, 2018

Member

This is related to the rebase map.

"execution".format(dep)
)

if scala_path:

This comment has been minimized.

@stuhood

stuhood Aug 29, 2018

Member

Could move this into the memoized construction of self.scala_path, or next to it? @memoized_method should work in this case.

zinc_relpath = fast_relpath(self._zinc.zinc, get_buildroot())

snapshots = [
self._zinc.snapshot(self.context._scheduler),

This comment has been minimized.

@stuhood

stuhood Aug 29, 2018

Member

Ditto @memoized_method.

}
}

with self.temporary_workdir(cleanup=False) as workdir:

This comment has been minimized.

@stuhood

stuhood Aug 29, 2018

Member

Should remove cleanup=False before landing.

illicitonion added some commits Aug 8, 2018

Zinc compiles can execute hermetically
There are a LOT of TODOs here, but it fundamentally works.

Notable things which are missing:
 * Cached vts won't provide files on the classpath
 * Rebase map is not set up properly
 * use-classpath-jars doesn't work
 * workdir must be a child of buildroot
 * classpath entries in the java dist don't work
 * Upstream analysis may or may not work properly in all cases
 * Remote execution will fail because .jdk isn't created yet
 * -zinc-cache-dir is in a homedir, so won't be written if that homedir
   doesn't exist
 * Many things (e.g. jar_library) don't populate ClasspathEntry
   DirectoryDigests
 * A big pile of inefficiencies

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/remotezinc/works branch from b135ad9 to 65426cb Aug 31, 2018

@ity

ity approved these changes Aug 31, 2018

Copy link
Member

ity left a comment

looks good

@illicitonion illicitonion merged commit 2b34eda into pantsbuild:master Sep 1, 2018

1 check passed

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

@illicitonion illicitonion deleted the twitter:dwagnerhall/remotezinc/works branch Sep 1, 2018

ity added a commit to ity/pants that referenced this pull request Sep 5, 2018

Zinc compiles can execute hermetically (pantsbuild#6351)
There are a LOT of TODOs here, but it fundamentally works.

Notable things which are missing:
 * Cached vts won't provide files on the classpath - pantsbuild#6429
 * Rebase map is not set up properly - pantsbuild#6434
 * use-classpath-jars doesn't work
 * workdir must be a child of buildroot
 * classpath entries in the java dist don't work 
 * Upstream analysis may or may not work properly in all cases
 * -zinc-cache-dir is in a homedir, so won't be written if that homedir
   doesn't exist - pantsbuild#6155
 * Many things (e.g. jar_library) don't populate ClasspathEntry
   DirectoryDigests - pantsbuild#6432
 * A big pile of inefficiencies

@ity ity added the needs-cherrypick label Sep 5, 2018

@ity ity removed the needs-cherrypick label Sep 17, 2018

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