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

[Bugfix] Respect 3rdparty resolver setting in BootstrapJvmTools #6789

Merged
merged 14 commits into from Dec 4, 2018
14 changes: 12 additions & 2 deletions src/python/pants/backend/jvm/tasks/bootstrap_jvm_tools.py
Expand Up @@ -16,8 +16,11 @@
from future.utils import PY3

from pants.backend.jvm.subsystems.jvm_tool_mixin import JvmToolMixin
from pants.backend.jvm.subsystems.resolve_subsystem import JvmResolveSubsystem
from pants.backend.jvm.subsystems.shader import Shader
from pants.backend.jvm.targets.jar_library import JarLibrary
from pants.backend.jvm.tasks.classpath_products import ClasspathProducts
from pants.backend.jvm.tasks.coursier_resolve import CoursierMixin
from pants.backend.jvm.tasks.ivy_task_mixin import IvyResolveFingerprintStrategy, IvyTaskMixin
from pants.backend.jvm.tasks.jar_task import JarTask
from pants.base.exceptions import TaskError
Expand Down Expand Up @@ -70,7 +73,7 @@ def __eq__(self, other):
return type(self) == type(other) and self._tuple() == other._tuple()


class BootstrapJvmTools(IvyTaskMixin, JarTask):
class BootstrapJvmTools(IvyTaskMixin, CoursierMixin, JarTask):
class ToolUnderspecified(Exception):
pass

Expand Down Expand Up @@ -237,7 +240,14 @@ def _check_underspecified_tools(self, jvm_tool, targets):
def _bootstrap_classpath(self, jvm_tool, targets):
self._check_underspecified_tools(jvm_tool, targets)
workunit_name = 'bootstrap-{}'.format(jvm_tool.key)
return self.ivy_classpath(targets, silent=True, workunit_name=workunit_name)
if JvmResolveSubsystem.global_instance().get_options().resolver == 'ivy':
ivy_classpath = self.ivy_classpath(targets, silent=True, workunit_name=workunit_name)
return ivy_classpath
else:
classpath_holder = ClasspathProducts(self.get_options().pants_workdir)
CoursierMixin.resolve(self, targets, classpath_holder, sources=False, javadoc=False, executor=None)
coursier_classpath = [cp_entry for _, cp_entry in classpath_holder.get_for_targets(targets)]
return coursier_classpath

@memoized_property
def shader(self):
Expand Down
Expand Up @@ -49,7 +49,7 @@ def register_options(cls, register):
],
help='Additional options to pass to coursier fetch. See `coursier fetch --help`')
register('--artifact-types', type=list, fingerprint=True,
default=['jar', 'bundle', 'test-jar', 'maven-plugin', 'src', 'doc', 'aar'],
default=['jar', 'bundle', 'test-jar', 'maven-plugin', 'src', 'doc'],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is necessary because otherwise coursier will grab the jna-4.5.aar over the jar. Currently we don't have any android users, so it's probably okay.

help='Specify the type of artifacts to fetch. See `packaging` at https://maven.apache.org/pom.html#Maven_Coordinates, '
'except `src` and `doc` being coursier specific terms for sources and javadoc.')
register('--bootstrap-jar-url', fingerprint=True,
Expand Down
50 changes: 29 additions & 21 deletions src/python/pants/backend/jvm/tasks/coursier_resolve.py
Expand Up @@ -28,6 +28,9 @@
from pants.base.fingerprint_strategy import FingerprintStrategy
from pants.base.workunit import WorkUnitLabel
from pants.invalidation.cache_manager import VersionedTargetSet
from pants.java import util
from pants.java.distribution.distribution import DistributionLocator
from pants.java.executor import Executor, SubprocessExecutor
from pants.java.jar.jar_dependency_utils import M2Coordinate, ResolvedJar
from pants.util.contextutil import temporary_file
from pants.util.dirutil import safe_mkdir
Expand All @@ -38,7 +41,7 @@ class CoursierResultNotFound(Exception):
pass


class CoursierMixin(NailgunTask, JvmResolverBase):
class CoursierMixin(JvmResolverBase):
"""
Experimental 3rdparty resolver using coursier.

Expand Down Expand Up @@ -98,7 +101,7 @@ def _compute_jars_to_resolve_and_pin(raw_jars, artifact_set, manager):

return jar_list, untouched_pinned_artifact

def resolve(self, targets, compile_classpath, sources, javadoc):
def resolve(self, targets, compile_classpath, sources, javadoc, executor):
"""
This is the core function for coursier resolve.

Expand All @@ -119,12 +122,18 @@ def resolve(self, targets, compile_classpath, sources, javadoc):
:param compile_classpath: classpath product that holds the resolution result. IMPORTANT: this parameter will be changed.
:param sources: if True, fetch sources for 3rdparty
:param javadoc: if True, fetch javadoc for 3rdparty
:param executor: An instance of `pants.java.executor.Executor`. If None, a subprocess executor will be assigned.
:return: n/a
"""
manager = JarDependencyManagement.global_instance()

jar_targets = manager.targets_by_artifact_set(targets)

executor = executor or SubprocessExecutor(DistributionLocator.cached())
if not isinstance(executor, Executor):
raise ValueError('The executor argument must be an Executor instance, given {} of type {}'.format(
executor, type(executor)))

for artifact_set, target_subset in jar_targets.items():
# TODO(wisechengyi): this is the only place we are using IvyUtil method, which isn't specific to ivy really.
raw_jar_deps, global_excludes = IvyUtils.calculate_classpath(target_subset)
Expand Down Expand Up @@ -165,7 +174,7 @@ def resolve(self, targets, compile_classpath, sources, javadoc):
manager)

results = self._get_result_from_coursier(jars_to_resolve, global_excludes, pinned_coords,
coursier_cache_dir, sources, javadoc)
coursier_cache_dir, sources, javadoc, executor)

for conf, result_list in results.items():
for result in result_list:
Expand Down Expand Up @@ -198,7 +207,7 @@ def _prepare_workdir(self):
return pants_jar_base_dir

def _get_result_from_coursier(self, jars_to_resolve, global_excludes, pinned_coords,
coursier_cache_path, sources, javadoc):
coursier_cache_path, sources, javadoc, executor):
"""
Calling coursier and return the result per invocation.

Expand All @@ -209,6 +218,7 @@ def _get_result_from_coursier(self, jars_to_resolve, global_excludes, pinned_coo
:param global_excludes: List of `M2Coordinate`s to exclude globally
:param pinned_coords: List of `M2Coordinate`s that need to be pinned.
:param coursier_cache_path: path to where coursier cache is stored.
:param executor: An instance of `pants.java.executor.Executor`

:return: The aggregation of results by conf from coursier. Each coursier call could return
the following:
Expand Down Expand Up @@ -264,18 +274,18 @@ def _get_result_from_coursier(self, jars_to_resolve, global_excludes, pinned_coo

results_by_conf = self._get_default_conf_results(common_args, coursier_jar, global_excludes, jars_to_resolve,
coursier_work_temp_dir,
pinned_coords)
pinned_coords, executor)
if sources or javadoc:
non_default_conf_results = self._get_non_default_conf_results(common_args, coursier_jar, global_excludes,
jars_to_resolve, coursier_work_temp_dir,
pinned_coords, sources, javadoc)
pinned_coords, sources, javadoc, executor)
results_by_conf.update(non_default_conf_results)

return results_by_conf

def _get_default_conf_results(self, common_args, coursier_jar, global_excludes, jars_to_resolve,
coursier_work_temp_dir,
pinned_coords):
pinned_coords, executor):

# Variable to store coursier result each run.
results = defaultdict(list)
Expand All @@ -289,13 +299,13 @@ def _get_default_conf_results(self, common_args, coursier_jar, global_excludes,
coursier_work_temp_dir,
output_fn)

results['default'].append(self._call_coursier(cmd_args, coursier_jar, output_fn))
results['default'].append(self._call_coursier(cmd_args, coursier_jar, output_fn, executor))

return results

def _get_non_default_conf_results(self, common_args, coursier_jar, global_excludes, jars_to_resolve,
coursier_work_temp_dir, pinned_coords,
sources, javadoc):
sources, javadoc, executor):
# To prevent improper api usage during development. User should not see this anyway.
if not sources and not javadoc:
raise TaskError("sources or javadoc has to be True.")
Expand Down Expand Up @@ -331,21 +341,19 @@ def _get_non_default_conf_results(self, common_args, coursier_jar, global_exclud
cmd_args.extend(special_args)

# sources and/or javadoc share the same conf
results['src_doc'] = [self._call_coursier(cmd_args, coursier_jar, output_fn)]
results['src_doc'] = [self._call_coursier(cmd_args, coursier_jar, output_fn, executor)]
return results

def _call_coursier(self, cmd_args, coursier_jar, output_fn):

labels = [WorkUnitLabel.COMPILER] if self.get_options().report else [WorkUnitLabel.TOOL]
def _call_coursier(self, cmd_args, coursier_jar, output_fn, executor):

return_code = self.runjava(
runner = executor.runner(
classpath=[coursier_jar],
main='coursier.cli.Coursier',
args=cmd_args,
jvm_options=self.get_options().jvm_options,
workunit_name='coursier',
workunit_labels=labels,
)
args=cmd_args)

labels = [WorkUnitLabel.COMPILER] if self.get_options().report else [WorkUnitLabel.TOOL]
return_code = util.execute_runner(runner, self.context.new_workunit, 'coursier', labels)

if return_code:
raise TaskError('The coursier process exited non-zero: {0}'.format(return_code))
Expand Down Expand Up @@ -653,7 +661,7 @@ def _get_path_to_jar(cls, coursier_cache_path, pants_jar_path_base, jar_path):
return os.path.join(pants_jar_path_base, 'relative', os.path.relpath(jar_path, coursier_cache_path))


class CoursierResolve(CoursierMixin):
class CoursierResolve(CoursierMixin, NailgunTask):

@classmethod
def subsystem_dependencies(cls):
Expand Down Expand Up @@ -692,8 +700,8 @@ def execute(self):
classpath_products = self.context.products.get_data('compile_classpath',
init_func=ClasspathProducts.init_func(
self.get_options().pants_workdir))

self.resolve(self.context.targets(), classpath_products, sources=False, javadoc=False)
executor = self.create_java_executor()
self.resolve(self.context.targets(), classpath_products, sources=False, javadoc=False, executor=executor)

def check_artifact_cache_for(self, invalidation_check):
# Coursier resolution is an output dependent on the entire target set, and is not divisible
Expand Down
5 changes: 4 additions & 1 deletion src/python/pants/backend/project_info/tasks/export.py
Expand Up @@ -113,6 +113,8 @@ def register_options(cls, register):
help='Causes sources to be output.')
register('--formatted', type=bool, implicit_value=False,
help='Causes output to be a single line of JSON.')
register('--jvm-options', type=list, metavar='<option>...',
help='Run the JVM 3rdparty resolver with these jvm options.')

@classmethod
def prepare(cls, options, round_manager):
Expand Down Expand Up @@ -155,7 +157,8 @@ def resolve_jars(self, targets):
else:
CoursierMixin.resolve(self, targets, compile_classpath,
sources=self.get_options().libraries_sources,
javadoc=self.get_options().libraries_javadocs)
javadoc=self.get_options().libraries_javadocs,
executor=executor)

return compile_classpath

Expand Down
Expand Up @@ -4,18 +4,20 @@

from __future__ import absolute_import, division, print_function, unicode_literals

from pants_test.pants_run_integration_test import PantsRunIntegrationTest
from pants_test.pants_run_integration_test import PantsRunIntegrationTest, ensure_resolver


class BootstrapJvmToolsIntegrationTest(PantsRunIntegrationTest):

@ensure_resolver
def test_bootstrap_jarjar_succeeds_normally(self):
# NB(gmalmquist): The choice of jarjar here is arbitrary; any jvm-tool that is integral to pants
# would suffice (eg, nailgun or jar-tool).
self.assert_success(self.run_pants(['clean-all']))
pants_run = self.run_pants(['bootstrap', '3rdparty:junit'])
self.assert_success(pants_run)

@ensure_resolver
def test_bootstrap_jarjar_failure(self):
self.assert_success(self.run_pants(['clean-all']))
pants_run = self.run_pants(['bootstrap', '--shader-jarjar="fake-target"', '3rdparty:junit'])
Expand Down
Expand Up @@ -23,6 +23,7 @@
from pants.backend.jvm.tasks.coursier_resolve import (CoursierResolve,
CoursierResolveFingerprintStrategy)
from pants.base.exceptions import TaskError
from pants.java import util
from pants.java.jar.exclude import Exclude
from pants.java.jar.jar_dependency import JarDependency
from pants.task.task import Task
Expand Down Expand Up @@ -100,7 +101,7 @@ def test_resolve_specific_with_sources_javadocs(self):
compile_classpath = context.products.get_data('compile_classpath',
init_func=ClasspathProducts.init_func(workdir)
)
task.resolve([jar_lib, scala_lib], compile_classpath, sources=True, javadoc=True)
task.resolve([jar_lib, scala_lib], compile_classpath, sources=True, javadoc=True, executor=None)

# Both javadoc and sources jars are added to the classpath product
self.assertEqual(['default', 'src_doc', 'src_doc'],
Expand Down Expand Up @@ -251,15 +252,15 @@ def test_when_invalid_hardlink_and_coursier_cache_should_trigger_resolve(self):
# Remove coursier's cache
safe_rmtree(couriser_cache_dir)

task.runjava = MagicMock()
util.execute_runner = MagicMock()

# Ignore any error because runjava may fail due to undefined behavior
try:
task.execute()
except TaskError:
pass

task.runjava.assert_called()
util.execute_runner.assert_called()

def test_resolve_jarless_pom(self):
jar = JarDependency('org.apache.commons', 'commons-weaver-privilizer-parent', '1.3')
Expand Down
10 changes: 10 additions & 0 deletions tests/python/pants_test/jvm/jvm_task_test_base.py
Expand Up @@ -6,8 +6,10 @@

import os

from pants.backend.jvm.subsystems.resolve_subsystem import JvmResolveSubsystem
from pants.backend.jvm.tasks.classpath_products import ClasspathProducts
from pants.util.dirutil import safe_file_dump, safe_mkdir, safe_mkdtemp
from pants_test.subsystem.subsystem_util import init_subsystem
from pants_test.task_test_base import TaskTestBase


Expand All @@ -16,6 +18,14 @@ class JvmTaskTestBase(TaskTestBase):
:API: public
"""

def setUp(self):
"""
:API: public
"""
super(JvmTaskTestBase, self).setUp()
init_subsystem(JvmResolveSubsystem)
self.set_options_for_scope('resolver', resolver='ivy')

def populate_runtime_classpath(self, context, classpath=None):
"""
Helps actual test cases to populate the 'runtime_classpath' products data mapping
Expand Down
Expand Up @@ -5,11 +5,12 @@
from __future__ import absolute_import, division, print_function, unicode_literals

from pants.util.contextutil import temporary_dir
from pants_test.pants_run_integration_test import PantsRunIntegrationTest
from pants_test.pants_run_integration_test import PantsRunIntegrationTest, ensure_resolver


class BootstrapJvmToolsIntegrationTest(PantsRunIntegrationTest):

@ensure_resolver
def test_zinc_tool_reuse_between_scala_and_java(self):
with temporary_dir() as artifact_cache:
bootstrap_args = [
Expand Down