change --force-baseurls to --allow-external-binary-tool-downloads

- also add associated testing

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

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

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

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

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.

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,

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

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(<example __str__()>)'
+
+
 # TODO(cosmicexplorer): test requests with an archiver!
 class BinaryUtilTest(BaseTest):
   """Tests binary_util's binaries_baseurls handling."""
@@ -62,19 +72,21 @@ 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,
       fetcher=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(<example __str__()>), 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(<example __str__()>), 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)