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

Compile a VERY simple java source remotely (no dependencies or inner classes) #5999

Merged
merged 40 commits into from Jul 4, 2018

Conversation

Projects
None yet
6 participants
@dotordogh
Copy link
Contributor

dotordogh commented Jun 21, 2018

This is the first step to compiling JVM targets remotely. In this change set, you'll find a test that executes a compile of a very simple java source remotely, and checks that the class file is present locally.

This PR depends on #5981 and extends it a bit by adding the hermetic flag.

@illicitonion
Copy link
Contributor

illicitonion left a comment

Looks good, thanks!

If you want to pick up #6007 (should take less than an hour :)), let's do that and then land this on top of it to use that function.

If you want someone else to do #6007 (I added it to the cross-pollination list), let's merge this, and then clean-up to use #6007 after that's merged.

if return_code:
raise TaskError('javac exited with return code {rc}'.format(rc=return_code))
if self.execution_strategy() == 'hermetic':
self._execute_compile_remotely(javac_cmd, ctx)

This comment has been minimized.

@illicitonion

illicitonion Jun 22, 2018

Contributor

s/_execute_compile_remotely/_execute_hermetic_compile/ - it may be local or remote, depending on other config

# do not have any dependencies or inner classes

input_files = set()
input_files.add(os.path.relpath(ctx.classes_dir, get_buildroot()))

This comment has been minimized.

@illicitonion

illicitonion Jun 22, 2018

Contributor

Does this do anything? Adding a directory to input_files shouldn't do anything...

[exec_result.output_directory_digest]
)[0].dependencies

# dump the files content into directory

This comment has been minimized.

@illicitonion

illicitonion Jun 22, 2018

Contributor

We should expose this as a rust intrinsic, rather than having to round-trip these file contents through Python and do the IO synchronously - filed #6007

register('--use-nailgun', type=bool,
help='Use nailgun to make repeated invocations of this task quicker.',
removal_version='1.10.0.dev0', removal_hint='Please use --execution-strategy instead.')
register('--execution-strategy', choices=[cls.NAILGUN, cls.SUBPROCESS],

This comment has been minimized.

@illicitonion

illicitonion Jun 22, 2018

Contributor

Shouldn't this have a new choices value for cls.HERMETIC?

This comment has been minimized.

@dotordogh

dotordogh Jun 22, 2018

Contributor

Yes!! It registered with me this morning when I was in my appointment that when I merged the other branch in, I just defaulted to taking all of the other changes and I forgot to add the new value. I'll add it. Sorry about that

@@ -24,3 +26,24 @@ def test_basic_binary(self):
],
workdir, config)
self.assert_success(pants_run)

def test_basic_binary_remote(self):

This comment has been minimized.

@illicitonion

illicitonion Jun 22, 2018

Contributor

s/remote/hermetic/

])
if self.execution_strategy() == 'hermetic':
javac_cmd.extend([
# The comment below is how we will add the output directory once it

This comment has been minimized.

@illicitonion

illicitonion Jun 22, 2018

Contributor

I'm actually not sure whether we should use output directories here, or whether we should output a jar, and have the process execution just fetch the jar...

Benefits of output directories:

  • Less file copying across the remote boundary when sub-targets change
  • Potentially less I/O for the compiler?

Benefits of jar files:

  • More predictable output paths (don't need to worry about annotation processors generating stuff in weird places)
  • Don't need to worry about directory prefixes - I can believe that using the below approach we'll need to strip off a prefix directory or two; but I'm not sure... I guess let's see how this plays out :)
  • Fewer small files to copy around (where overhead per-file may be a thing)

This comment has been minimized.

@stuhood

stuhood Jun 22, 2018

Member

Twitter recently enabled --compile-zinc-use-classpath-jars internally because modern zinc supports it a bit better. So yea, I think we're about ready to commit, enable that flag by default, and deprecate it.

@benjyw : In your JVM usecases, have been using --use-classpath-jars?

This comment has been minimized.

@benjyw

benjyw Jun 22, 2018

Contributor

No, but I don't see why we couldn't. I'll enable it and see what happens.

This comment has been minimized.

@benjyw

benjyw Jun 22, 2018

Contributor

Looks fine - we poke around in the runtime classpath, which already uses z.jar, I believe.

This comment has been minimized.

@stuhood

stuhood Jun 22, 2018

Member

Yep. Cool.

@benjyw

This comment has been minimized.

Copy link
Contributor

benjyw commented Jun 22, 2018

I have nothing wise to say except that I'm super-excited for this.

@illicitonion
Copy link
Contributor

illicitonion left a comment

Looks great! Thanks!

# snapshot.
'-d', '.',
# TODO: support -release
'-source', str(settings.source_level),

This comment has been minimized.

@illicitonion

illicitonion Jun 29, 2018

Contributor

The source and target flags should probably be added unconditionally, rather than in the if block :)

'-target', str(settings.target_level),
])

if self.execution_strategy() == 'hermetic':

This comment has been minimized.

@wisechengyi

wisechengyi Jun 29, 2018

Contributor

Use the constant?

workunit.set_outcome(WorkUnit.FAILURE if return_code else WorkUnit.SUCCESS)
if return_code:
raise TaskError('javac exited with return code {rc}'.format(rc=return_code))
if self.execution_strategy() == 'hermetic':

This comment has been minimized.

@wisechengyi

wisechengyi Jun 29, 2018

Contributor

ditto

@@ -26,7 +26,7 @@ def test_scala_repl_helloworld_input(self):
)
self.assert_success(pants_run)
self.assertIn('Hello, World!', pants_run.stdout_data.splitlines())

This comment has been minimized.

@wisechengyi

wisechengyi Jun 29, 2018

Contributor

You can leave this file untouched.

@@ -28,6 +30,8 @@
# Well known metadata file to register annotation processors with a java 1.6+ compiler.
_PROCESSOR_INFO_FILE = 'META-INF/services/javax.annotation.processing.Processor'

# Execution stratetgies:
_HERMETIC = 'hermetic'

This comment has been minimized.

@illicitonion

illicitonion Jul 3, 2018

Contributor

Because JavacCompile transitively extends NailgunTaskBase, you should be able to just self.HERMETIC where you're using this constant, and remove this constant :)

Dorothy Ordogh

@dotordogh dotordogh merged commit 22db5cf into pantsbuild:master Jul 4, 2018

1 check passed

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

@dotordogh dotordogh deleted the dotordogh:dotordogh/update-nailgun-flags-with-jvm-comp-changes branch Jul 4, 2018

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