Hermetic zinc runs with a native-image #7796

illicitonion · 2019-05-23T14:03:08Z

This will not work with remoting across platforms yet.
This will depend on a native image being uploaded to binaries.pantsbuild.org

Some pieces of this are a little ugly, but the options system kind of forces them to be. I don't want to do a significant restructure of BinaryUtil because #7790 would probably replace it anyway...

Commits are separately useful.

stuhood

In order to land this, it will need to be conditional I think? Or is the idea that "hermetic always means native-image"? If so, works for me.

illicitonion · 2019-05-23T16:38:40Z

In order to land this, it will need to be conditional I think? Or is the idea that "hermetic always means native-image"? If so, works for me.

Yeah, hermetic == native image :)

cosmicexplorer · 2019-05-23T18:59:51Z

src/python/pants/backend/jvm/subsystems/zinc.py

@@ -46,9 +49,27 @@ class Zinc(object):

  _lock = Lock()

-  class Factory(Subsystem, JvmToolMixin):
+  # This is both a NativeTool (for the graal native image of zinc), _and_ a Subsystem by its own
+  # right. We're not allowed to explicitly inherit from both because of MRO.


BinaryToolBase inherits from Subsystem though?

Yeah, but BinaryTool has a really specific meaning, and I wanted to call out that this isn't just a BinaryTool, it also builds some Subsystem behaviour of its own.

Ok, that makes sense. I was thinking that the discussion on why to put the NativeTool on an existing options scope would cover that.

cosmicexplorer · 2019-05-23T19:02:50Z

src/python/pants/backend/jvm/subsystems/zinc.py

+    # Because the native image's version is tightly coupled to this class, and zinc is the options
+    # scope we'd want to pick for it, we just put these details here.
+
+    allow_version_override = False


pants.util.meta.classproperty allows adding a docstring for these kinds of properties!

cosmicexplorer · 2019-05-23T19:09:43Z

src/python/pants/binaries/binary_tool.py

@@ -50,6 +50,8 @@ class BinaryToolBase(Subsystem):
  # Subclasses may set this to provide extra register() kwargs for the --version option.
  extra_version_option_kwargs = None

+  allow_version_override = True


I might maybe just change this to a @classproperty named something like hardcoded_version which defaults to None? That would mean the zinc native-image factory subsystem could just set hardcoded_version instead of having to override version().

I agree with both of your design suggestions here.

On the other hand, I think that the underlying idea that we won't want to be able to point this at multiple versions is not accurate. It's fine to change the default, but I don't think that we want to hardcode it.

I pretty strongly think it's important that there's one way to select what version of zinc is being used; I would be very confused, both as a pants user and as someone supporting other pants users, trying to work out which version settings override which...

I don't see allow_version_override as being any less confusing than something like hardcoded_version, but I do see it being more indirect (where is the actual override occurring?). I misinterpreted this reading it earlier as assuming that allow_version_override meant "allow the class to override any version set by the user", as opposed to the other way around -- because I didn't think of the user specifying a specific --version option value as "overriding" the version, but rather just "setting" it.

On the other hand, I think that the underlying idea that we won't want to be able to point this at multiple versions is not accurate. It's fine to change the default, but I don't think that we want to hardcode it.

I don't think we'll want to provide native images as BinaryTools for very long, though -- e.g. #6893 will be able to avoid any hardcoded dependence on a specific zinc or scala-library version by just converting whatever the input classpath ends up being into an image! So I would recommend shaping the solution in a way that lets us remove as cleanly as possible later (both of the approaches here would seem to work).

Yep. This patch represents a temporary solution to allow us to iterate. And we're immediately going to have multiple versions of the native image.

cosmicexplorer · 2019-05-23T19:11:31Z

src/python/pants/backend/jvm/subsystems/zinc.py

+    # to put the NativeTool on an existing options scope (this one makes sense!), or we'd need to
+    # make an options scope with a dummy option just for the ZincNativeImage NativeTool.
+    # Because the native image's version is tightly coupled to this class, and zinc is the options
+    # scope we'd want to pick for it, we just put these details here.


Good explanation! I think keeping it in the same class makes sense right now due to the version coupling as well.

src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py

stuhood · 2019-05-23T20:36:34Z

After discussion with @cosmicexplorer and @patliu85 , the goal is to add back a conditional flag for hermetic to actually use or not use the native image. While that won't allow for maximal test coverage of use of the native image yet, it allows us to preserve hermetic coverage, and decouples the "how to deliver a native-image" question from this patch.

We'll also add support for using a native image for rsc in this patch.

stuhood · 2019-05-24T03:46:52Z

This experienced only #7800, and it successfully invoked a native image locally. Going to go ahead and merge. EDIT: Oh, nevermind. Applying Danny's review feedback.

stuhood · 2019-05-24T04:04:31Z

I dropped the version hardcoding, because particularly as we iterate on getting a working native-image for our usecases, we'll likely need to publish a bunch of them.

illicitonion · 2019-05-24T10:14:15Z

src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py

@@ -440,15 +440,30 @@ def _compile_hermetic(self, jvm_options, ctx, classes_dir, zinc_args,
      classpath_entry.directory_digest for classpath_entry in scalac_classpath_entries
    )

+    if self._zinc.use_native_image:


I think it's pretty important to error here if there are jvm_options specified which we're going to ignore. Otherwise, I can easily imagine people wasting hours debugging why the options they're setting aren't having effect.

1000% agreed on this

The counterpoint is that everything sets jvm_options, both upstream and downstream, because the jvm_options have default values. They also never contain logic.

Having said that: there is consensus. Will do it =)

It's possible to explicitly set jvm_options: [] whenever a native image is used for zinc to avoid this (which is why it might be better as an option on the zinc task instead of the subsystem, but since this is temporary anyway it should be fine).

We want to tie the Zinc NativeImage version to the version of Zinc we use, not have it be overridable

This will need to change as part of pantsbuild#7790 and pantsbuild#7735

This will not work with remoting across platforms yet. This will depend on a native image being uploaded to binaries.pantsbuild.org

This reverts commit f0d84dd.

As a continuation of #7807: similar changes to #7796, but for rsc invocations in RscCompile (which may also use a zinc native-image for hermetic zinc invocations, orthogonally to this change).

Add an option to allow using a native-image of zinc via a Native tool hosted binary. In future, the native-image will be built automatically from the JVM artifacts, but for now this helps us to gain experience with the model.

As a continuation of #7807: similar changes to #7796, but for rsc invocations in RscCompile (which may also use a zinc native-image for hermetic zinc invocations, orthogonally to this change).

illicitonion requested review from stuhood, benjyw, cosmicexplorer and patliu85 May 23, 2019 14:03

illicitonion mentioned this pull request May 23, 2019

add hacky options to ZincCompile to use a provided native-image binary #7755

Closed

stuhood approved these changes May 23, 2019

View reviewed changes

cosmicexplorer reviewed May 23, 2019

View reviewed changes

stuhood force-pushed the dwagnerhall/nativeimage/zinc branch from 5eaf148 to 8ade214 Compare May 24, 2019 04:03

illicitonion commented May 24, 2019

View reviewed changes

illicitonion and others added 9 commits May 24, 2019 17:05

BinaryToolBase can have fixed version

c9356b8

We want to tie the Zinc NativeImage version to the version of Zinc we use, not have it be overridable

NativeTool can be snapshotted, not just Script

2856388

This will need to change as part of pantsbuild#7790 and pantsbuild#7735

Zinc is a NativeTool representing its Graal native-image

27b3353

Hermetic zinc runs with a native-image

7f64432

This will not work with remoting across platforms yet. This will depend on a native image being uploaded to binaries.pantsbuild.org

Added option to use native-image for hermetic with zinc

2e4a02e

Revert "BinaryToolBase can have fixed version"

de1bb88

This reverts commit f0d84dd.

Allow for overrides of the zinc compiler native-image version.

9d426c4

Include rt.jar on the scala boot classpath.

005c2ab

Restore the jvm_options check, and include the rce.jar.

302d1a6

stuhood force-pushed the dwagnerhall/nativeimage/zinc branch from 62688b3 to 302d1a6 Compare May 25, 2019 01:05

stuhood merged commit 7731c97 into pantsbuild:master May 25, 2019

stuhood deleted the dwagnerhall/nativeimage/zinc branch May 25, 2019 05:35

cosmicexplorer mentioned this pull request May 28, 2019

Rsc native image changes #7807

Closed

patliu85 mentioned this pull request May 28, 2019

Rsc native image support #7809

Merged

stuhood added this to the 1.16.x milestone May 29, 2019

stuhood added the needs-cherrypick label May 29, 2019

stuhood removed needs-cherrypick labels May 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hermetic zinc runs with a native-image #7796

Hermetic zinc runs with a native-image #7796

illicitonion commented May 23, 2019

stuhood left a comment

illicitonion commented May 23, 2019

cosmicexplorer May 23, 2019

illicitonion May 24, 2019

cosmicexplorer May 24, 2019

cosmicexplorer May 23, 2019

cosmicexplorer May 23, 2019

stuhood May 24, 2019

illicitonion May 24, 2019

cosmicexplorer May 24, 2019

cosmicexplorer May 24, 2019

stuhood May 24, 2019

cosmicexplorer May 23, 2019

stuhood commented May 23, 2019 •

edited

Loading

stuhood commented May 24, 2019 •

edited

Loading

stuhood commented May 24, 2019 •

edited

Loading

illicitonion May 24, 2019

cosmicexplorer May 24, 2019

stuhood May 25, 2019 •

edited

Loading

cosmicexplorer May 25, 2019

Hermetic zinc runs with a native-image #7796

Hermetic zinc runs with a native-image #7796

Conversation

illicitonion commented May 23, 2019

stuhood left a comment

Choose a reason for hiding this comment

illicitonion commented May 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood commented May 23, 2019 • edited Loading

stuhood commented May 24, 2019 • edited Loading

stuhood commented May 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood May 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood commented May 23, 2019 •

edited

Loading

stuhood commented May 24, 2019 •

edited

Loading

stuhood commented May 24, 2019 •

edited

Loading

stuhood May 25, 2019 •

edited

Loading