Clarify manual download required if unable to fetch package (#18242)

Clarify manual download required if unable to fetch (from mirror(s)); support (and tests) for package-specific download instructions

4 files changed, 87 insertions, 11 deletions

diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index d5cb3065a8..3a0c17da88 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -577,7 +577,8 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
     archive_files = []
 
     #: Boolean. Set to ``True`` for packages that require a manual download.
-    #: This is currently only used by package sanity tests.
+    #: This is currently used by package sanity tests and generation of a
+    #: more meaningful fetch failure error.
     manual_download = False
 
     #: Set of additional options used when fetching package versions.
@@ -1210,6 +1211,20 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
         """
         spack.store.layout.remove_install_directory(self.spec)
 
+    @property
+    def download_instr(self):
+        """
+        Defines the default manual download instructions.  Packages can
+        override the property to provide more information.
+
+        Returns:
+            (str):  default manual download instructions
+        """
+        required = ('Manual download is required for {0}. '
+                    .format(self.spec.name) if self.manual_download else '')
+        return ('{0}Refer to {1} for download instructions.'
+                .format(required, self.spec.package.homepage))
+
     def do_fetch(self, mirror_only=False):
         """
         Creates a stage directory and downloads the tarball for this package.
@@ -1244,7 +1259,8 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
                                  self.spec.format('{name}{@version}'), ck_msg)
 
         self.stage.create()
-        self.stage.fetch(mirror_only)
+        err_msg = None if not self.manual_download else self.download_instr
+        self.stage.fetch(mirror_only, err_msg=err_msg)
         self._fetch_time = time.time() - start_time
 
         if checksum and self.version in self.versions:
diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py
index 95e7dcc592..cf32bc92f8 100644
--- a/lib/spack/spack/stage.py
+++ b/lib/spack/spack/stage.py
@@ -400,8 +400,14 @@ class Stage(object):
         """Returns the well-known source directory path."""
         return os.path.join(self.path, _source_path_subdir)
 
-    def fetch(self, mirror_only=False):
-        """Downloads an archive or checks out code from a repository."""
+    def fetch(self, mirror_only=False, err_msg=None):
+        """Retrieves the code or archive
+
+        Args:
+            mirror_only (bool): only fetch from a mirror
+            err_msg (str or None): the error message to display if all fetchers
+                fail or ``None`` for the default fetch failure message
+        """
         fetchers = []
         if not mirror_only:
             fetchers.append(self.default_fetcher)
@@ -480,9 +486,8 @@ class Stage(object):
         else:
             print_errors(errors)
 
-            err_msg = 'All fetchers failed for {0}'.format(self.name)
             self.fetcher = self.default_fetcher
-            raise fs.FetchError(err_msg, None)
+            raise fs.FetchError(err_msg or 'All fetchers failed', None)
 
         print_errors(errors)
 
diff --git a/lib/spack/spack/test/packaging.py b/lib/spack/spack/test/packaging.py
index 75ead64955..bee0d01b16 100644
--- a/lib/spack/spack/test/packaging.py
+++ b/lib/spack/spack/test/packaging.py
@@ -560,3 +560,52 @@ def test_macho_make_paths():
                    '/Users/Shared/spack/pkgB/libB.dylib',
                    '/usr/local/lib/libloco.dylib':
                    '/usr/local/lib/libloco.dylib'}
+
+
+@pytest.fixture()
+def mock_download():
+    """Mock a failing download strategy."""
+    class FailedDownloadStrategy(spack.fetch_strategy.FetchStrategy):
+        def mirror_id(self):
+            return None
+
+        def fetch(self):
+            raise spack.fetch_strategy.FailedDownloadError(
+                "<non-existent URL>", "This FetchStrategy always fails")
+
+    fetcher = FetchStrategyComposite()
+    fetcher.append(FailedDownloadStrategy())
+
+    @property
+    def fake_fn(self):
+        return fetcher
+
+    orig_fn = spack.package.PackageBase.fetcher
+    spack.package.PackageBase.fetcher = fake_fn
+    yield
+    spack.package.PackageBase.fetcher = orig_fn
+
+
+@pytest.mark.parametrize("manual,instr", [(False, False), (False, True),
+                                          (True, False), (True, True)])
+@pytest.mark.disable_clean_stage_check
+def test_manual_download(install_mockery, mock_download, monkeypatch, manual,
+                         instr):
+    """
+    Ensure expected fetcher fail message based on manual download and instr.
+    """
+    @property
+    def _instr(pkg):
+        return 'Download instructions for {0}'.format(pkg.spec.name)
+
+    spec = Spec('a').concretized()
+    pkg = spec.package
+
+    pkg.manual_download = manual
+    if instr:
+        monkeypatch.setattr(spack.package.PackageBase, 'download_instr',
+                            _instr)
+
+    expected = pkg.download_instr if manual else 'All fetchers failed'
+    with pytest.raises(spack.fetch_strategy.FetchError, match=expected):
+        pkg.do_fetch()
diff --git a/lib/spack/spack/test/stage.py b/lib/spack/spack/test/stage.py
index 9cf50b8da2..204a6e6a3e 100644
--- a/lib/spack/spack/test/stage.py
+++ b/lib/spack/spack/test/stage.py
@@ -529,15 +529,21 @@ class TestStage(object):
                 pass
         check_destroy(stage, self.stage_name)
 
-    def test_search_if_default_fails(self, failing_fetch_strategy, search_fn):
+    @pytest.mark.parametrize(
+        "err_msg,expected", [('Fetch from fetch.test.com',
+                              'Fetch from fetch.test.com'),
+                             (None, 'All fetchers failed')])
+    def test_search_if_default_fails(self, failing_fetch_strategy, search_fn,
+                                     err_msg, expected):
         stage = Stage(failing_fetch_strategy,
                       name=self.stage_name,
                       search_fn=search_fn)
+
         with stage:
-            try:
-                stage.fetch(mirror_only=False)
-            except spack.fetch_strategy.FetchError:
-                pass
+            with pytest.raises(spack.fetch_strategy.FetchError,
+                               match=expected):
+                stage.fetch(mirror_only=False, err_msg=err_msg)
+
         check_destroy(stage, self.stage_name)
         assert search_fn.performed_search