Show python errors in Project Manager for adding repos and downloading gems

Signed-off-by: AMZN-Phil <[email protected]>

Code/Tools/ProjectManager/Source/DownloadWorker.cpp

@@ -33,7 +33,7 @@ namespace O3DE::ProjectManager
         }
         else
         {
-            emit Done(tr("Gem download failed"));
+            emit Done(gemInfoResult.GetError().c_str());
         }
     }
 

Code/Tools/ProjectManager/Source/GemRepo/GemRepoScreen.cpp

@@ -92,8 +92,8 @@ namespace O3DE::ProjectManager
                 return;
             }
 
-            bool addGemRepoResult = PythonBindingsInterface::Get()->AddGemRepo(repoUri);
-            if (addGemRepoResult)
+            AZ::Outcome<void, AZStd::string> addGemRepoResult = PythonBindingsInterface::Get()->AddGemRepo(repoUri);
+            if (addGemRepoResult.IsSuccess())
             {
                 Reinit();
                 emit OnRefresh();
@@ -101,8 +101,8 @@ namespace O3DE::ProjectManager
             else
             {
                 QString failureMessage = tr("Failed to add gem repo: %1.").arg(repoUri);
-                QMessageBox::critical(this, tr("Operation failed"), failureMessage);
-                AZ_Error("Project Manger", false, failureMessage.toUtf8());
+                QMessageBox::critical(this, failureMessage, addGemRepoResult.GetError().c_str());
+                AZ_Error("Project Manager", false, failureMessage.toUtf8());
             }
         }
     }

Code/Tools/ProjectManager/Source/PythonBindings.cpp

@@ -61,6 +61,7 @@ namespace Platform
 namespace RedirectOutput
 {
     using RedirectOutputFunc = AZStd::function<void(const char*)>;
+    AZStd::string lastPythonError;
 
     struct RedirectOutput
     {
@@ -210,6 +211,16 @@ namespace RedirectOutput
         });
 
         SetRedirection("stderr", g_redirect_stderr_saved, g_redirect_stderr, []([[maybe_unused]] const char* msg) {
+            if (lastPythonError.empty())
+            {
+                lastPythonError = msg;
+                const int lengthOfErrorPrefix = 11;
+                auto errorPrefix = lastPythonError.find("ERROR:root:");
+                if (errorPrefix != AZStd::string::npos)
+                {
+                    lastPythonError.erase(errorPrefix, lengthOfErrorPrefix);
+                }
+            }
             AZ_TracePrintf("Python", msg);
         });
 
@@ -1051,12 +1062,13 @@ namespace O3DE::ProjectManager
         return result && refreshResult;
     }
 
-    bool PythonBindings::AddGemRepo(const QString& repoUri)
+    AZ::Outcome<void, AZStd::string> PythonBindings::AddGemRepo(const QString& repoUri)
     {
         bool registrationResult = false;
         bool result = ExecuteWithLock(
             [&]
             {
+                RedirectOutput::lastPythonError.clear();
                 auto pyUri = QString_To_Py_String(repoUri);
                 auto pythonRegistrationResult = m_register.attr("register")(
                     pybind11::none(), pybind11::none(), pybind11::none(), pybind11::none(), pybind11::none(), pybind11::none(), pyUri);
@@ -1065,7 +1077,12 @@ namespace O3DE::ProjectManager
                 registrationResult = !pythonRegistrationResult.cast<bool>();
             });
 
-        return result && registrationResult;
+        if (!result || !registrationResult)
+        {
+            return AZ::Failure<AZStd::string>(AZStd::move(RedirectOutput::lastPythonError));
+        }
+
+        return AZ::Success();
     }
 
     bool PythonBindings::RemoveGemRepo(const QString& repoUri)
@@ -1225,6 +1242,7 @@ namespace O3DE::ProjectManager
         auto result = ExecuteWithLockErrorHandling(
             [&]
             {
+                RedirectOutput::lastPythonError.clear();
                 auto downloadResult = m_download.attr("download_gem")(
                     QString_To_Py_String(gemName), // gem name
                     pybind11::none(), // destination path
@@ -1248,7 +1266,7 @@ namespace O3DE::ProjectManager
         }
         else if (!downloadSucceeded)
         {
-            return AZ::Failure<AZStd::string>("Failed to download gem.");
+            return AZ::Failure<AZStd::string>(AZStd::move(RedirectOutput::lastPythonError));
         }
 
         return AZ::Success();

Code/Tools/ProjectManager/Source/PythonBindings.h

@@ -62,7 +62,7 @@ namespace O3DE::ProjectManager
         // Gem Repos
         AZ::Outcome<void, AZStd::string> RefreshGemRepo(const QString& repoUri) override;
         bool RefreshAllGemRepos() override;
-        bool AddGemRepo(const QString& repoUri) override;
+        AZ::Outcome<void, AZStd::string> AddGemRepo(const QString& repoUri) override;
         bool RemoveGemRepo(const QString& repoUri) override;
         AZ::Outcome<QVector<GemRepoInfo>, AZStd::string> GetAllGemRepoInfos() override;
         AZ::Outcome<QVector<GemInfo>, AZStd::string> GetAllGemRepoGemsInfos() override;

Code/Tools/ProjectManager/Source/PythonBindingsInterface.h

@@ -200,9 +200,9 @@ namespace O3DE::ProjectManager
         /**
          * Registers this gem repo with the current engine.
          * @param repoUri the absolute filesystem path or url to the gem repo.
-         * @return true on success, false on failure.
+         * @return an outcome with a string error message on failure.
          */
-        virtual bool AddGemRepo(const QString& repoUri) = 0;
+        virtual AZ::Outcome<void, AZStd::string> AddGemRepo(const QString& repoUri) = 0;
 
         /**
          * Unregisters this gem repo with the current engine.

scripts/o3de/o3de/register.py

@@ -490,7 +490,7 @@ def register_repo(json_data: dict,
     repo_sha256 = hashlib.sha256(url.encode())
     cache_file = manifest.get_o3de_cache_folder() / str(repo_sha256.hexdigest() + '.json')
 
-    result = utils.download_file(parsed_uri, cache_file)
+    result = utils.download_file(parsed_uri, cache_file, True)
     if result == 0:
         json_data.setdefault('repos', []).insert(0, repo_uri)
 

scripts/o3de/o3de/repo.py

@@ -24,6 +24,7 @@ def process_add_o3de_repo(file_name: str or pathlib.Path,
                           repo_set: set) -> int:
     file_name = pathlib.Path(file_name).resolve()
     if not validation.valid_o3de_repo_json(file_name):
+        logger.error(f'Repository JSON {file_name} could not be loaded or is missing required values')
         return 1
     cache_folder = manifest.get_o3de_cache_folder()
 
@@ -114,7 +115,7 @@ def get_gem_json_paths_from_cached_repo(repo_uri: str) -> set:
 
     file_name = pathlib.Path(cache_filename).resolve()
     if not file_name.is_file():
-        logger.error(f'Could not find cached repo json file for {repo_uri}')
+        logger.error(f'Could not find cached repository json file for {repo_uri}. Try refreshing the repository.')
         return gem_set
 
     with file_name.open('r') as f:
@@ -167,6 +168,7 @@ def refresh_repo(repo_uri: str,
 
     download_file_result = utils.download_file(parsed_uri, cache_file, True)
     if download_file_result != 0:
+        logger.error(f'Repo json {repo_uri} could not download.')
         return download_file_result
 
     if not validation.valid_o3de_repo_json(cache_file):

scripts/o3de/o3de/utils.py

@@ -125,7 +125,8 @@ def download_file(parsed_uri, download_path: pathlib.Path, force_overwrite: bool
     """
     if download_path.is_file():
         if not force_overwrite:
-            logger.warn(f'File already downloaded to {download_path}.')
+            logger.error(f'File already downloaded to {download_path} and force_overwrite is not set.')
+            return 1
         else:
             try:
                 os.unlink(download_path)
@@ -134,20 +135,25 @@ def download_file(parsed_uri, download_path: pathlib.Path, force_overwrite: bool
                 return 1
 
     if parsed_uri.scheme in ['http', 'https', 'ftp', 'ftps']:
-        with urllib.request.urlopen(parsed_uri.geturl()) as s:
-            download_file_size = 0
-            try:
-                download_file_size = s.headers['content-length']
-            except KeyError:
-                pass
-            def download_progress(downloaded_bytes):
-                if download_progress_callback:
-                    return download_progress_callback(int(downloaded_bytes), int(download_file_size))
-                return False
-            with download_path.open('wb') as f:
-                download_cancelled = copyfileobj(s, f, download_progress)
-                if download_cancelled:
-                    return 1
+        try:
+            with urllib.request.urlopen(parsed_uri.geturl()) as s:
+                download_file_size = 0
+                try:
+                    download_file_size = s.headers['content-length']
+                except KeyError:
+                    pass
+                def download_progress(downloaded_bytes):
+                    if download_progress_callback:
+                        return download_progress_callback(int(downloaded_bytes), int(download_file_size))
+                    return False
+                with download_path.open('wb') as f:
+                    download_cancelled = copyfileobj(s, f, download_progress)
+                    if download_cancelled:
+                        logger.warn(f'Download of file to {download_path} cancelled.')
+                        return 1
+        except urllib.error.HTTPError as e:
+            logger.error(f'HTTP Error {e.code} opening {parsed_uri.geturl()}')
+            return 1
     else:
         origin_file = pathlib.Path(parsed_uri.geturl()).resolve()
         if not origin_file.is_file():
@@ -167,7 +173,7 @@ def download_zip_file(parsed_uri, download_zip_path: pathlib.Path, force_overwri
         return download_file_result
 
     if not zipfile.is_zipfile(download_zip_path):
-        logger.error(f"File zip {download_zip_path} is invalid.")
+        logger.error(f"File zip {download_zip_path} is invalid. Try re-downloading the file.")
         download_zip_path.unlink()
         return 1