From c8bb85d7956214aaa24703f7e877510ed9e5a88d Mon Sep 17 00:00:00 2001 From: Danny McClanahan <1305167+cosmicexplorer@users.noreply.github.com> Date: Sat, 12 May 2018 18:43:28 -0700 Subject: [PATCH] change --force-baseurls to --allow-external-binary-tool-downloads - also add associated testing --- .../contrib/go/subsystems/go_distribution.py | 2 +- .../pants/backend/native/subsystems/llvm.py | 2 +- src/python/pants/binaries/binary_tool.py | 13 +- src/python/pants/binaries/binary_util.py | 40 +++--- src/python/pants/option/global_options.py | 9 +- src/python/pants/pantsd/watchman_launcher.py | 2 +- .../pants_test/binaries/test_binary_tool.py | 4 +- .../pants_test/binaries/test_binary_util.py | 115 +++++++++++++----- 8 files changed, 122 insertions(+), 65 deletions(-) diff --git a/contrib/go/src/python/pants/contrib/go/subsystems/go_distribution.py b/contrib/go/src/python/pants/contrib/go/subsystems/go_distribution.py index 09edaf9452b..032efd4bd71 100644 --- a/contrib/go/src/python/pants/contrib/go/subsystems/go_distribution.py +++ b/contrib/go/src/python/pants/contrib/go/subsystems/go_distribution.py @@ -37,7 +37,7 @@ class GoDistribution(NativeTool): default_version = '1.8.3' archive_type = 'tgz' - def url_generator(self): + def get_external_url_generator(self): return GoReleaseUrlGenerator() @memoized_property diff --git a/src/python/pants/backend/native/subsystems/llvm.py b/src/python/pants/backend/native/subsystems/llvm.py index 81e308a2af8..43c3c757a80 100644 --- a/src/python/pants/backend/native/subsystems/llvm.py +++ b/src/python/pants/backend/native/subsystems/llvm.py @@ -36,7 +36,7 @@ class LLVM(NativeTool, ExecutablePathProvider): default_version = '6.0.0' archive_type = 'txz' - def url_generator(self): + def get_external_url_generator(self): return LLVMReleaseUrlGenerator() @memoized_method diff --git a/src/python/pants/binaries/binary_tool.py b/src/python/pants/binaries/binary_tool.py index afd9a2a1806..d46029e7535 100644 --- a/src/python/pants/binaries/binary_tool.py +++ b/src/python/pants/binaries/binary_tool.py @@ -75,17 +75,18 @@ def _get_archiver(self): return create_archiver(self.archive_type) - def url_generator(self): + def get_external_url_generator(self): """Override and return an instance of BinaryToolUrlGenerator to download from those urls. If this method returns None, urls to download the tool will be constructed from --binaries-baseurls. Otherwise, generate_urls() will be invoked on the result with the requested version and host platform. - If the bootstrap option --force-baseurls is set, the result of this method will be ignored. In - general, implementors of BinaryTool subclasses should try to ensure (within reason) that the - rest of their code can handle the result of downloading from different urls if url_generator() - is overridden yet again by a subclass, or if it is ignored with --force-baseurls. + If the bootstrap option --allow-external-binary-tool-downloads is False, the result of this + method will be ignored. In general, implementors of BinaryTool subclasses should try to ensure + (within reason) that the rest of their code can handle the result of downloading from different + urls if get_external_url_generator() is overridden yet again by a subclass, or if it is ignored + with --no-allow-external-binary-tool-downloads. See the :class:`LLVM` subsystem for an example of usage. """ @@ -168,7 +169,7 @@ def _make_binary_request(self, version): version=version, name=self._name_to_fetch(), platform_dependent=self.platform_dependent, - url_generator=self.url_generator(), + external_url_generator=self.get_external_url_generator(), archiver=self._get_archiver()) def _select_for_version(self, version): diff --git a/src/python/pants/binaries/binary_util.py b/src/python/pants/binaries/binary_util.py index 797e2879b79..c58379f8a90 100644 --- a/src/python/pants/binaries/binary_util.py +++ b/src/python/pants/binaries/binary_util.py @@ -48,7 +48,8 @@ class BinaryToolUrlGenerator(object): :API: public :class:`BinaryTool` subclasses can return an instance of a class mixing this in to - url_generator(self) to download their file or archive from some specified url or set of urls. + get_external_url_generator(self) to download their file or archive from some specified url or set + of urls. """ @abstractmethod @@ -71,8 +72,8 @@ def generate_urls(self, version, host_platform): class PantsHosted(BinaryToolUrlGenerator): """Given a binary request and --binaries-baseurls, generate urls to download the binary from. - This url generator is used if url_generator(self) is not overridden by a BinaryTool subclass, or - if --force-baseurls is set. + This url generator is used if get_external_url_generator(self) is not overridden by a BinaryTool + subclass, or if --allow-external-binary-tool-downloads is False. NB: "pants-hosted" is referring to the organization of the urls being specific to pants. It also happens that most binaries are downloaded from S3 hosting at binaries.pantsbuild.org by default -- @@ -111,7 +112,7 @@ class BinaryRequest(datatype([ 'name', 'platform_dependent', # NB: this can be None! - 'url_generator', + 'external_url_generator', # NB: this can be None! 'archiver', ])): @@ -263,7 +264,7 @@ def _create_for_cls(cls, binary_util_cls): baseurls=options.binaries_baseurls, binary_tool_fetcher=binary_tool_fetcher, path_by_id=options.binaries_path_by_id, - force_baseurls=options.force_baseurls) + allow_external_binary_tool_downloads=options.allow_external_binary_tool_downloads) class MissingMachineInfo(TaskError): """Indicates that pants was unable to map this machine's OS to a binary path prefix.""" @@ -281,8 +282,8 @@ def __init__(self, binary_request, base_exception): "Error resolving binary request {}: {}".format(binary_request, base_exception), base_exception) - def __init__(self, baseurls, binary_tool_fetcher, path_by_id=None, force_baseurls=False, - uname_func=None): + def __init__(self, baseurls, binary_tool_fetcher, path_by_id=None, + allow_external_binary_tool_downloads=True, uname_func=None): """Creates a BinaryUtil with the given settings to define binary lookup behavior. This constructor is primarily used for testing. Production code will usually initialize @@ -295,8 +296,9 @@ def __init__(self, baseurls, binary_tool_fetcher, path_by_id=None, force_baseurl search for binaries in, or download binaries to if needed. :param dict path_by_id: Additional mapping from (sysname, id) -> (os, arch) for tool directory naming - :param bool force_baseurls: Whether to use --binaries-baseurls to download all binaries - regardless of whether a :class:`BinaryToolUrlGenerator` is provided. + :param bool allow_external_binary_tool_downloads: If False, use --binaries-baseurls to download + all binaries, regardless of whether an + external_url_generator field is provided. :param function uname_func: method to use to emulate os.uname() in testing """ self._baseurls = baseurls @@ -306,7 +308,7 @@ def __init__(self, baseurls, binary_tool_fetcher, path_by_id=None, force_baseurl if path_by_id: self._path_by_id.update((tuple(k), tuple(v)) for k, v in path_by_id.items()) - self._force_baseurls = force_baseurls + self._allow_external_binary_tool_downloads = allow_external_binary_tool_downloads self._uname_func = uname_func or os.uname _ID_BY_OS = { @@ -349,14 +351,18 @@ def _get_download_path(self, binary_request): return binary_request.get_download_path(self._host_platform()) def _get_url_generator(self, binary_request): - url_generator = binary_request.url_generator - logger.debug("self._force_baseurls: {}".format(self._force_baseurls)) - logger.debug("url_generator: {}".format(url_generator)) - if self._force_baseurls or not url_generator: + external_url_generator = binary_request.external_url_generator + + logger.debug("self._allow_external_binary_tool_downloads: {}" + .format(self._allow_external_binary_tool_downloads)) + logger.debug("external_url_generator: {}".format(external_url_generator)) + + if external_url_generator and self._allow_external_binary_tool_downloads: + url_generator = external_url_generator + else: if not self._baseurls: raise self.NoBaseUrlsError("--binaries-baseurls is empty.") - url_generator = PantsHosted(binary_request=binary_request, baseurls=self._baseurls) return url_generator @@ -416,7 +422,7 @@ def _make_deprecated_binary_request(self, supportdir, version, name): version=version, name=name, platform_dependent=True, - url_generator=None, + external_url_generator=None, archiver=None) def select_binary(self, supportdir, version, name): @@ -429,7 +435,7 @@ def _make_deprecated_script_request(self, supportdir, version, name): version=version, name=name, platform_dependent=False, - url_generator=None, + external_url_generator=None, archiver=None) def select_script(self, supportdir, version, name): diff --git a/src/python/pants/option/global_options.py b/src/python/pants/option/global_options.py index 200c8fd8e96..2fa382de1b8 100644 --- a/src/python/pants/option/global_options.py +++ b/src/python/pants/option/global_options.py @@ -164,11 +164,10 @@ def register_bootstrap_options(cls, register): help=("Maps output of uname for a machine to a binary search path: " "(sysname, id) -> (os, arch), e.g. {('darwin', '15'): ('mac', '10.11'), " "('linux', 'arm32'): ('linux', 'arm32')}.")) - # BinaryTool options. - register('--force-baseurls', type=bool, default=False, advanced=True, - help="Force BinaryTool subclasses to download from urls generated from " - "--binaries-baseurls, even if the tool has a custom url generator. " - "This can be necessary if using Pants in an environment which cannot " + register('--allow-external-binary-tool-downloads', type=bool, default=True, advanced=True, + help="If False, require BinaryTool subclasses to download their contents from urls " + "generated from --binaries-baseurls, even if the tool has an external url " + "generator. This can be necessary if using Pants in an environment which cannot " "contact the wider Internet.") # Pants Daemon options. diff --git a/src/python/pants/pantsd/watchman_launcher.py b/src/python/pants/pantsd/watchman_launcher.py index ddc31b60578..d789cdc677f 100644 --- a/src/python/pants/pantsd/watchman_launcher.py +++ b/src/python/pants/pantsd/watchman_launcher.py @@ -28,7 +28,7 @@ def create(cls, bootstrap_options): binary_tool_fetcher=binary_tool_fetcher, path_by_id=bootstrap_options.binaries_path_by_id, # TODO(cosmicexplorer): do we need to test this? - force_baseurls=bootstrap_options.force_baseurls) + allow_external_binary_tool_downloads=bootstrap_options.allow_external_binary_tool_downloads) return WatchmanLauncher( binary_util, diff --git a/tests/python/pants_test/binaries/test_binary_tool.py b/tests/python/pants_test/binaries/test_binary_tool.py index 3298eb7d6df..baa4678c610 100644 --- a/tests/python/pants_test/binaries/test_binary_tool.py +++ b/tests/python/pants_test/binaries/test_binary_tool.py @@ -63,7 +63,7 @@ class CustomUrls(BinaryToolBase): name = 'custom_urls_tool' default_version = 'v2.1' - def url_generator(self): + def get_external_url_generator(self): return CustomUrlGenerator() def _select_for_version(self, version): @@ -115,7 +115,7 @@ def test_replacing_legacy_options(self): def test_urls(self): default_version_tool = DefaultVersion.global_instance() - self.assertIsNone(default_version_tool.url_generator()) + self.assertIsNone(default_version_tool.get_external_url_generator()) with self.assertRaises(BinaryUtilPrivate.BinaryResolutionError) as cm: default_version_tool.select() diff --git a/tests/python/pants_test/binaries/test_binary_util.py b/tests/python/pants_test/binaries/test_binary_util.py index 4c41eb5c28e..0ab515f2d4e 100644 --- a/tests/python/pants_test/binaries/test_binary_util.py +++ b/tests/python/pants_test/binaries/test_binary_util.py @@ -10,7 +10,7 @@ import mock -from pants.binaries.binary_util import BinaryToolFetcher, BinaryUtilPrivate +from pants.binaries.binary_util import BinaryRequest, BinaryToolFetcher, BinaryToolUrlGenerator, BinaryUtilPrivate from pants.net.http.fetcher import Fetcher from pants.util.contextutil import temporary_dir from pants.util.dirutil import safe_open @@ -20,6 +20,16 @@ logger = logging.getLogger(__name__) +class ExternalUrlGenerator(BinaryToolUrlGenerator): + + def generate_urls(self, version, host_platform): + return ['https://example.com/some-binary', 'https://example.com/same-binary'] + + # Make the __str__ deterministic, for testing exception messages. + def __str__(self): + return 'ExternalUrlGenerator()' + + # TODO(cosmicexplorer): test requests with an archiver! class BinaryUtilTest(BaseTest): """Tests binary_util's binaries_baseurls handling.""" @@ -62,7 +72,7 @@ def _fake_url(cls, binaries, base, binary_key): @classmethod def _gen_binary_tool_fetcher(cls, bootstrap_dir='/tmp', timeout_secs=30, fetcher=None, - ignore_cached_download=True): + ignore_cached_download=True): return BinaryToolFetcher( bootstrap_dir=bootstrap_dir, timeout_secs=timeout_secs, @@ -70,11 +80,13 @@ def _gen_binary_tool_fetcher(cls, bootstrap_dir='/tmp', timeout_secs=30, fetcher ignore_cached_download=ignore_cached_download) @classmethod - def _gen_binary_util(cls, baseurls=[], path_by_id=None, uname_func=None, **kwargs): + def _gen_binary_util(cls, baseurls=[], path_by_id=None, allow_external_binary_tool_downloads=True, + uname_func=None, **kwargs): return BinaryUtilPrivate( baseurls=baseurls, binary_tool_fetcher=cls._gen_binary_tool_fetcher(**kwargs), path_by_id=path_by_id, + allow_external_binary_tool_downloads=allow_external_binary_tool_downloads, uname_func=uname_func) @classmethod @@ -97,18 +109,71 @@ def test_timeout(self): path_or_fd=mock.ANY, timeout_secs=timeout_value) - def test_nobases(self): + def test_no_base_urls_error(self): """Tests exception handling if build support urls are improperly specified.""" binary_util = self._gen_binary_util() - # TODO: test error message! - with self.assertRaises(binary_util.BinaryResolutionError) as cm: - # TODO: test select_binary() producing the right BinaryRequest to fulfill? - binary_util.select_binary(supportdir='bin/protobuf', - version='2.4.1', - name='protoc') - self.fail('Expected downloading the binary to raise.') - expected_msg = "--binaries-baseurls is empty." - self.assertIn(expected_msg, str(cm.exception)) + + with self.assertRaises(BinaryUtilPrivate.BinaryResolutionError) as cm: + binary_util.select_script("supportdir", "version", "name") + the_raised_exception_message = str(cm.exception) + + self.assertIn(BinaryUtilPrivate.NoBaseUrlsError.__name__, the_raised_exception_message) + expected_msg = ( + "Error resolving binary request BinaryRequest(supportdir=supportdir, version=version, " + "name=name, platform_dependent=False, external_url_generator=None, archiver=None): " + "--binaries-baseurls is empty.") + self.assertIn(expected_msg, the_raised_exception_message) + + def test_external_url_generator(self): + binary_util = self._gen_binary_util() + + binary_request = BinaryRequest( + supportdir='supportdir', + version='version', + name='name', + platform_dependent=False, + external_url_generator=ExternalUrlGenerator(), + # TODO: test archiver! + archiver=None) + + with self.assertRaises(BinaryUtilPrivate.BinaryResolutionError) as cm: + binary_util.select(binary_request) + the_raised_exception_message = str(cm.exception) + + self.assertIn(BinaryToolFetcher.BinaryNotFound.__name__, the_raised_exception_message) + expected_msg = ( + "Error resolving binary request BinaryRequest(supportdir=supportdir, version=version, " + "name=name, platform_dependent=False, " + "external_url_generator=ExternalUrlGenerator(), archiver=None): " + "Failed to fetch name binary from any source: (Failed to fetch binary from " + "https://example.com/some-binary: Fetch of https://example.com/some-binary failed with " + "status code 404, Failed to fetch binary from https://example.com/same-binary: Fetch of " + "https://example.com/same-binary failed with status code 404)'") + self.assertIn(expected_msg, the_raised_exception_message) + + def test_disallowing_external_urls(self): + binary_util = self._gen_binary_util(allow_external_binary_tool_downloads=False) + + binary_request = binary_request = BinaryRequest( + supportdir='supportdir', + version='version', + name='name', + platform_dependent=False, + external_url_generator=ExternalUrlGenerator(), + # TODO: test archiver! + archiver=None) + + with self.assertRaises(BinaryUtilPrivate.BinaryResolutionError) as cm: + binary_util.select(binary_request) + the_raised_exception_message = str(cm.exception) + + self.assertIn(BinaryUtilPrivate.NoBaseUrlsError.__name__, the_raised_exception_message) + expected_msg = ( + "Error resolving binary request BinaryRequest(supportdir=supportdir, version=version, " + "name=name, platform_dependent=False, " + "external_url_generator=ExternalUrlGenerator(), archiver=None): " + "--binaries-baseurls is empty.") + self.assertIn(expected_msg, the_raised_exception_message) def test_support_url_multi(self): """Tests to make sure existing base urls function as expected.""" @@ -214,7 +279,7 @@ def uname_func(): self.assertIn(BinaryUtilPrivate.MissingMachineInfo.__name__, the_raised_exception_message) expected_msg = ( "Error resolving binary request BinaryRequest(supportdir=supportdir, version=version, " - "name=name, platform_dependent=True, url_generator=None, archiver=None): " + "name=name, platform_dependent=True, external_url_generator=None, archiver=None): " "Pants could not resolve binaries for the current host: platform 'vms' was not recognized. " "Recognized platforms are: [u'darwin', u'linux'].") self.assertIn(expected_msg, the_raised_exception_message) @@ -232,7 +297,7 @@ def uname_func(): self.assertIn(BinaryUtilPrivate.MissingMachineInfo.__name__, the_raised_exception_message) expected_msg = ( "Error resolving binary request BinaryRequest(supportdir=mysupportdir, version=myversion, " - "name=myname, platform_dependent=True, url_generator=None, archiver=None): Pants could not " + "name=myname, platform_dependent=True, external_url_generator=None, archiver=None): Pants could not " "resolve binaries for the current host. Update --binaries-path-by-id to find binaries for " "the current host platform (u\'darwin\', u\'999\').\\n--binaries-path-by-id was:") self.assertIn(expected_msg, the_raised_exception_message) @@ -251,9 +316,9 @@ def uname_func(): expected_msg = ( "Error resolving binary request BinaryRequest(supportdir=mysupportdir, version=myversion, " # platform_dependent=False when doing select_script() - "name=myname, platform_dependent=False, url_generator=None, archiver=None): Pants could not " - "resolve binaries for the current host. Update --binaries-path-by-id to find binaries for " - "the current host platform (u\'darwin\', u\'999\').\\n--binaries-path-by-id was:") + "name=myname, platform_dependent=False, external_url_generator=None, archiver=None): Pants " + "could not resolve binaries for the current host. Update --binaries-path-by-id to find " + "binaries for the current host platform (u\'darwin\', u\'999\').\\n--binaries-path-by-id was:") self.assertIn(expected_msg, the_raised_exception_message) def test_select_binary_base_path_override(self): @@ -267,17 +332,3 @@ def uname_func(): self.assertEquals("supportdir/skynet/42/version/name", binary_util._get_download_path(binary_request)) - - def test_no_base_urls_error(self): - binary_util = self._gen_binary_util() - - with self.assertRaises(BinaryUtilPrivate.BinaryResolutionError) as cm: - binary_util.select_script("supportdir", "version", "name") - the_raised_exception_message = str(cm.exception) - - self.assertIn(BinaryUtilPrivate.NoBaseUrlsError.__name__, the_raised_exception_message) - expected_msg = ( - "Error resolving binary request BinaryRequest(supportdir=supportdir, version=version, " - "name=name, platform_dependent=False, url_generator=None, archiver=None): " - "--binaries-baseurls is empty.") - self.assertIn(expected_msg, the_raised_exception_message)