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

[rsc-compile] update jdk dist lookup and usage to work remotely #6593

Merged
merged 2 commits into from Oct 8, 2018

Conversation

Projects
None yet
3 participants
@baroquebobcat
Copy link
Contributor

baroquebobcat commented Oct 4, 2018

Remote execution assumes a .jdk that contains the jdk. Before, RscCompile would just use the local jdk paths, which would be wrong on a remote worker.

This patch introduces a small shim class to provide a distribution that creates remote compatible paths while still ensuring that the path lookup works correctly. It also changes the desandboxification helper to work for both local and remote exec.

[rsc-compile] update jdk dist lookup and usage to work remotely
Remote execution assumes a .jdk that contains the jdk. Before, RscCompile would just use the local jdk paths, which would be wrong on a remote worker.

This patch introduces a small shim class to provide a distribution that creates remote compatible paths while still ensuring that the path lookup works correctly
@stuhood

stuhood approved these changes Oct 4, 2018

Copy link
Member

stuhood left a comment

Would be good to treat the "desandboxifying" as a bug, but other than that, this looks good.

Should probably add an integration test for rsc+hermetic too.

input_files=input_files_directory_digest,
output_files=tuple(),
output_directories=(output_dir,),
timeout_seconds=15*60,
description='run {} for {}'.format(tool_name, tgt)
description='run {} for {}'.format(tool_name, tgt),
# TODO: These should always be unicodes

This comment has been minimized.

@stuhood

stuhood Oct 4, 2018

Member

@Eric-Arellano : Does this look right to you? Will using six have the appropriate effect?

This comment has been minimized.

@Eric-Arellano

Eric-Arellano Oct 4, 2018

Contributor

Hm so depends on the interpreter we're using and the values of those two variables.

It looks the intended types are tool_name: str, tgt: Optional[str]? (The backported str, i.e. unicode).

When using Python 2, we can mix byte strings and unicode freely. So, if either tool_name or tgt are bytes, nothing will complain, which it sounds like we do not want to allow this mixing? In Python 3, we'll get an exception for trying to use .format() on bytes and for mixing unicode and bytes.

--

six won't actually be very helpful here. We have two options to ensure it's unicode:

tool_name = ensure_text(tool_name)  # will coerce into unicode
from builtins import str

assert isinstance(tool_name, str)

Also a little weirdness that tgt is Optional. It looks like it will print the string literal "None" if the value is None. Is that intended?

This comment has been minimized.

@baroquebobcat

baroquebobcat Oct 5, 2018

Contributor

This todo is from https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py#L436-L438

        description="zinc compile for {}".format(ctx.target.address.spec),
        # TODO: These should always be unicodes
        # Since this is always hermetic, we need to use `underlying_dist`
jdk_home=text_type(self._zinc.underlying_dist.home),

I didn't know the context for sure, but it was the thing that I was missing here.

# TODO when these are generated once, we won't need to collect them here.
metai_classpath.append(desandboxify_pantsd_loc(metacp_result["scalaLibrarySynthetics"]))
# NB The json is absolute pathed pointing into either the buildroot or
# NB The json uses absolute paths pointing into either the buildroot or

This comment has been minimized.

@stuhood

stuhood Oct 4, 2018

Member

We should probably either request/make a change for this upstream to output relative json (cc @xeno-by) or add a script that runs after metacp to re-write the json while we're still in the sandbox.

This comment has been minimized.

@baroquebobcat

baroquebobcat Oct 5, 2018

Contributor

Yeah. I think that's the right approach.

return metai_classpath

def _get_jvm_distribution(self):
# TODO We may want to use different jvm distributions depending on what

This comment has been minimized.

@stuhood

stuhood Oct 4, 2018

Member

I expect that @illicitonion has a ticket about this.

This comment has been minimized.

@baroquebobcat

baroquebobcat Oct 5, 2018

Contributor

Yes. I think it's this one. #6416
I'll add a note pointing to it.

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

I have a simple hermetic integration test right now. The thing that failed, ironically wasn't the hermetic case, but the other one.

input_files=input_files_directory_digest,
output_files=tuple(),
output_directories=(output_dir,),
timeout_seconds=15*60,
description='run {} for {}'.format(tool_name, tgt)
description='run {} for {}'.format(tool_name, tgt),
# TODO: These should always be unicodes

This comment has been minimized.

@baroquebobcat

baroquebobcat Oct 5, 2018

Contributor

This todo is from https://github.com/pantsbuild/pants/blob/master/src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py#L436-L438

        description="zinc compile for {}".format(ctx.target.address.spec),
        # TODO: These should always be unicodes
        # Since this is always hermetic, we need to use `underlying_dist`
jdk_home=text_type(self._zinc.underlying_dist.home),

I didn't know the context for sure, but it was the thing that I was missing here.

# TODO when these are generated once, we won't need to collect them here.
metai_classpath.append(desandboxify_pantsd_loc(metacp_result["scalaLibrarySynthetics"]))
# NB The json is absolute pathed pointing into either the buildroot or
# NB The json uses absolute paths pointing into either the buildroot or

This comment has been minimized.

@baroquebobcat

baroquebobcat Oct 5, 2018

Contributor

Yeah. I think that's the right approach.

return metai_classpath

def _get_jvm_distribution(self):
# TODO We may want to use different jvm distributions depending on what

This comment has been minimized.

@baroquebobcat

baroquebobcat Oct 5, 2018

Contributor

Yes. I think it's this one. #6416
I'll add a note pointing to it.

@baroquebobcat baroquebobcat merged commit 0a29c4c into pantsbuild:master Oct 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment