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

Fix rsc compile transitive dep bug introduced in #7742 #7825

Merged

Conversation

Projects
None yet
3 participants
@cosmicexplorer
Copy link
Contributor

commented May 31, 2019

Problem

#7742 removed the method self._on_invalid_compile_dependency() from JvmCompile, as it seemed complex and it wasn't clear if it was still needed. We were able to reproduce an error internally which made it clear that logic was still necessary, so that method was added back, along with some testing to avoid further regression.

Commits are independently reviewable.

Solution

  • Re-add self._on_invalid_compile_dependency().
  • Introduce enum options for --jvm-platform-compiler and --resolver-resolver, and architect the jvm resolve tasks more similarly to the jvm compile tasks, where a specific resolve task is selected by matching its declared resolver type to the --resolver-resolver option value.
  • Refactor rsc compile integration testing to remove the manual split between between hermetic and nonhermetic testing.
  • Add a test case for a zinc-java target depending transitively on a scala target compiled with rsc-and-zinc.

Result

The rsc compile task passes our internal CI job which compiles some code using rsc and zinc!

@stuhood
Copy link
Member

left a comment

Great investigation. v1 strikes again!

Thanks Danny!

@@ -237,17 +237,26 @@ def _check_underspecified_tools(self, jvm_tool, targets):
default option was provided to use for bootstrap.
""".format(jvm_tool.key)))

@property

This comment has been minimized.

Copy link
@illicitonion

illicitonion May 31, 2019

Contributor

Why not an @abstractproperty? (And execute_resolve an @abstractmethod?

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 31, 2019

Author Contributor

Because this is already an @abstractproperty by virtue of the IvyTaskMixin and CoursierMixin subclassing JvmResolverBase -- if these methods aren't explicitly overridden with a non-abstract method/property, you get:

Exception message: Can't instantiate abstract class BootstrapJvmTools_bootstrap_bootstrap_jvm_tools with abstract methods resolver_name
Show resolved Hide resolved src/python/pants/backend/jvm/tasks/ivy_imports.py Outdated
Show resolved Hide resolved src/python/pants/backend/jvm/tasks/resolve_shared.py Outdated

@stuhood stuhood self-assigned this May 31, 2019

@stuhood stuhood force-pushed the cosmicexplorer:fix-rsc-compile-transitive-dep-bug branch from a25497f to d118426 May 31, 2019

@stuhood

This comment has been minimized.

Copy link
Member

commented May 31, 2019

I've reverted the enum changes out in 220f9cb: git reverting that commit should reapply them, but it would be good to wait at least a week before doing that.

There are two other new commits. Will merge on green.

@stuhood stuhood force-pushed the cosmicexplorer:fix-rsc-compile-transitive-dep-bug branch from 7bd4f49 to 847758e Jun 1, 2019

@@ -373,7 +380,8 @@ def run_pants_with_workdir(self, command, workdir, config=None, stdin_data=None,
handle = self.run_pants_with_workdir_without_waiting(command, workdir, **kwargs)
return handle.join(stdin_data=stdin_data, tee_output=tee_output)

def run_pants(self, command, config=None, stdin_data=None, extra_env=None, **kwargs):
def run_pants(self, command, config=None, stdin_data=None, extra_env=None, cleanup_workdir=True,

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Jun 1, 2019

Author Contributor
Suggested change
def run_pants(self, command, config=None, stdin_data=None, extra_env=None, cleanup_workdir=True,
def run_pants(self, command, config=None, stdin_data=None, extra_env=None, **kwargs

This was a mistake before I realized that self.pants_results() already existed -- the cleanup_workdir arg can be removed entirely.

('pid', Exactly(*IntegerForPid)),
])):
pass


class PantsJoinHandle(datatype(['command', 'process', 'workdir'])):
class PantsJoinHandle(datatype([
# NB: May be either a string list or string (in the case of `shell=True`).

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Jun 1, 2019

Author Contributor

This is something we may be able to represent with TypeConstraints without too much overhead (and really what I'd like to do is to allow enum to support wrapping a value in addition to a tag). I'll make an issue.

@@ -0,0 +1,6 @@
// Copyright 2014 Pants project contributors (see CONTRIBUTORS.md).

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Jun 1, 2019

Author Contributor
Suggested change
// Copyright 2014 Pants project contributors (see CONTRIBUTORS.md).
// Copyright 2019 Pants project contributors (see CONTRIBUTORS.md).

We should add another pre-commit hook for this, I'll make an issue.

cmd = list(args)
success = kwargs.pop('success', True)

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Jun 1, 2019

Author Contributor

This could be a python 3 TODO to make success a keyword-only argument (see #7076).

@contextmanager
def do_command_yielding_workdir(self, *args, **kwargs):
cmd = list(args)
success = kwargs.pop('success', True)

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer Jun 1, 2019

Author Contributor

Also could leave a TODO pointing to #7076.

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented Jun 1, 2019

Left a review but we should merge this when green to fix rsc compiles and worry about the comments later, along with the enum option fixes which were reverted earlier. Thanks!

@stuhood stuhood merged commit 9974632 into pantsbuild:master Jun 1, 2019

1 check passed

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

stuhood added a commit that referenced this pull request Jun 1, 2019

Fix rsc compile transitive dep bug introduced in #7742 (#7825)
### Problem

#7742 removed the method `self._on_invalid_compile_dependency()` from `JvmCompile`, as it seemed complex and it wasn't clear if it was still needed. We were able to reproduce an error internally which made it clear that logic was still necessary, so that method was added back, along with some testing to avoid further regression.

### Solution

- Re-add `self._on_invalid_compile_dependency()`.
- Refactor rsc compile integration testing to remove the manual split between between hermetic and nonhermetic testing.
- Add a test case for a `zinc-java` target depending transitively on a scala target compiled with `rsc-and-zinc`.

### Result

The rsc compile task passes our internal CI job which compiles some code using rsc and zinc!

stuhood added a commit that referenced this pull request Jun 1, 2019

Fix rsc compile transitive dep bug introduced in #7742 (#7825)
### Problem

#7742 removed the method `self._on_invalid_compile_dependency()` from `JvmCompile`, as it seemed complex and it wasn't clear if it was still needed. We were able to reproduce an error internally which made it clear that logic was still necessary, so that method was added back, along with some testing to avoid further regression.

### Solution

- Re-add `self._on_invalid_compile_dependency()`.
- Refactor rsc compile integration testing to remove the manual split between between hermetic and nonhermetic testing.
- Add a test case for a `zinc-java` target depending transitively on a scala target compiled with `rsc-and-zinc`.

### Result

The rsc compile task passes our internal CI job which compiles some code using rsc and zinc!
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.