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

Hermetic zinc runs with a native-image #7796

Merged
merged 9 commits into from May 25, 2019

Conversation

Projects
None yet
4 participants
@illicitonion
Copy link
Contributor

commented May 23, 2019

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
Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2019

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 :)

@@ -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.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 23, 2019

Contributor

BinaryToolBase inherits from Subsystem though?

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 24, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 24, 2019

Contributor

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

# 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

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 23, 2019

Contributor

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

@@ -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

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 23, 2019

Contributor

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().

This comment has been minimized.

Copy link
@stuhood

stuhood May 24, 2019

Member

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.

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 24, 2019

Author Contributor

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...

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 24, 2019

Contributor

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.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 24, 2019

Contributor

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).

This comment has been minimized.

Copy link
@stuhood

stuhood May 24, 2019

Member

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.

# 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.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 23, 2019

Contributor

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

Show resolved Hide resolved src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py Outdated
@stuhood

This comment has been minimized.

Copy link
Member

commented May 23, 2019

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

This comment has been minimized.

Copy link
Member

commented May 24, 2019

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 stuhood force-pushed the twitter:dwagnerhall/nativeimage/zinc branch from 5eaf148 to 8ade214 May 24, 2019

@stuhood

This comment has been minimized.

Copy link
Member

commented May 24, 2019

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.

@@ -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:

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 24, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 24, 2019

Contributor

1000% agreed on this

This comment has been minimized.

Copy link
@stuhood

stuhood May 25, 2019

Member

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 =)

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 25, 2019

Contributor

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).

illicitonion and others added some commits May 23, 2019

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

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

1 check passed

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

@stuhood stuhood deleted the twitter:dwagnerhall/nativeimage/zinc branch May 25, 2019

@patliu85 patliu85 referenced this pull request May 28, 2019

Merged

Rsc native image support #7809

stuhood added a commit that referenced this pull request May 29, 2019

Rsc native image support (#7809)
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).

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

stuhood added a commit that referenced this pull request May 29, 2019

Hermetic zinc runs with a native-image (#7796)
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.

stuhood added a commit that referenced this pull request May 29, 2019

Rsc native image support (#7809)
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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.