Merge pull request #5681 from aws-lumberyard-dev/Prism/CLIDisplayLastError

Show python errors in Project Manager

Code/Tools/ProjectManager/Source/DownloadController.cpp

@@ -73,13 +73,25 @@ namespace O3DE::ProjectManager
         emit GemDownloadProgress(m_gemNames.front(), bytesDownloaded, totalBytes);
     }
 
-    void DownloadController::HandleResults(const QString& result)
+    void DownloadController::HandleResults(const QString& result, const QString& detailedError)
     {
         bool succeeded = true;
         
         if (!result.isEmpty())
         {
-            QMessageBox::critical(nullptr, tr("Gem download"), result);
+            if (!detailedError.isEmpty())
+            {
+                QMessageBox gemDownloadError;
+                gemDownloadError.setIcon(QMessageBox::Critical);
+                gemDownloadError.setWindowTitle(tr("Gem download"));
+                gemDownloadError.setText(result);
+                gemDownloadError.setDetailedText(detailedError);
+                gemDownloadError.exec();
+            }
+            else
+            {
+                QMessageBox::critical(nullptr, tr("Gem download"), result);
+            }
             succeeded = false;
         }
 

Code/Tools/ProjectManager/Source/DownloadController.h

@@ -54,7 +54,7 @@ namespace O3DE::ProjectManager
         }
     public slots:
         void UpdateUIProgress(int bytesDownloaded, int totalBytes);
-        void HandleResults(const QString& result);
+        void HandleResults(const QString& result, const QString& detailedError);
 
     signals:
         void StartGemDownload(const QString& gemName);

Code/Tools/ProjectManager/Source/DownloadWorker.cpp

@@ -24,16 +24,16 @@ namespace O3DE::ProjectManager
         {
             emit UpdateProgress(bytesDownloaded, totalBytes);
         };
-        AZ::Outcome<void, AZStd::string> gemInfoResult =
+        AZ::Outcome<void, AZStd::pair<AZStd::string, AZStd::string>> gemInfoResult =
             PythonBindingsInterface::Get()->DownloadGem(m_gemName, gemDownloadProgress, /*force*/true);
 
         if (gemInfoResult.IsSuccess())
         {
-            emit Done("");
+            emit Done("", "");
         }
         else
         {
-            emit Done(tr("Gem download failed"));
+            emit Done(gemInfoResult.GetError().first.c_str(), gemInfoResult.GetError().second.c_str());
         }
     }
 

Code/Tools/ProjectManager/Source/DownloadWorker.h

@@ -32,7 +32,7 @@ namespace O3DE::ProjectManager
 
     signals:
         void UpdateProgress(int bytesDownloaded, int totalBytes);
-        void Done(QString result = "");
+        void Done(QString result = "", QString detailedResult = "");
 
     private:
 

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

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

Code/Tools/ProjectManager/Source/PythonBindings.cpp

@@ -23,6 +23,7 @@
 #include <AzCore/IO/SystemFile.h>
 #include <AzCore/std/containers/unordered_set.h>
 #include <AzCore/std/string/conversions.h>
+#include <AzCore/std/numeric.h>
 #include <AzCore/StringFunc/StringFunc.h>
 
 #include <QDir>
@@ -210,6 +211,16 @@ namespace RedirectOutput
         });
 
         SetRedirection("stderr", g_redirect_stderr_saved, g_redirect_stderr, []([[maybe_unused]] const char* msg) {
+            AZStd::string lastPythonError = msg;
+            constexpr const char* pythonErrorPrefix = "ERROR:root:";
+            constexpr size_t lengthOfErrorPrefix = AZStd::char_traits<char>::length(pythonErrorPrefix);
+            auto errorPrefix = lastPythonError.find(pythonErrorPrefix);
+            if (errorPrefix != AZStd::string::npos)
+            {
+                lastPythonError.erase(errorPrefix, lengthOfErrorPrefix);
+            }
+            O3DE::ProjectManager::PythonBindingsInterface::Get()->AddErrorString(lastPythonError);
+
             AZ_TracePrintf("Python", msg);
         });
 
@@ -376,6 +387,8 @@ namespace O3DE::ProjectManager
         pybind11::gil_scoped_release release;
         pybind11::gil_scoped_acquire acquire;
 
+        ClearErrorStrings();
+
         try
         {
             executionCallback();
@@ -1051,7 +1064,7 @@ namespace O3DE::ProjectManager
         return result && refreshResult;
     }
 
-    bool PythonBindings::AddGemRepo(const QString& repoUri)
+    AZ::Outcome<void, AZStd::pair<AZStd::string, AZStd::string>> PythonBindings::AddGemRepo(const QString& repoUri)
     {
         bool registrationResult = false;
         bool result = ExecuteWithLock(
@@ -1065,7 +1078,12 @@ namespace O3DE::ProjectManager
                 registrationResult = !pythonRegistrationResult.cast<bool>();
             });
 
-        return result && registrationResult;
+        if (!result || !registrationResult)
+        {
+            return AZ::Failure<AZStd::pair<AZStd::string, AZStd::string>>(GetSimpleDetailedErrorPair());
+        }
+
+        return AZ::Success();
     }
 
     bool PythonBindings::RemoveGemRepo(const QString& repoUri)
@@ -1243,7 +1261,7 @@ namespace O3DE::ProjectManager
         return AZ::Success(AZStd::move(gemInfos));
     }
 
-    AZ::Outcome<void, AZStd::string> PythonBindings::DownloadGem(
+    AZ::Outcome<void, AZStd::pair<AZStd::string, AZStd::string>> PythonBindings::DownloadGem(
         const QString& gemName, std::function<void(int, int)> gemProgressCallback, bool force)
     {
         // This process is currently limited to download a single gem at a time.
@@ -1272,11 +1290,12 @@ namespace O3DE::ProjectManager
 
         if (!result.IsSuccess())
         {
-            return result;
+            AZStd::pair<AZStd::string, AZStd::string> pythonRunError(result.GetError(), result.GetError());
+            return AZ::Failure<AZStd::pair<AZStd::string, AZStd::string>>(AZStd::move(pythonRunError));
         }
         else if (!downloadSucceeded)
         {
-            return AZ::Failure<AZStd::string>("Failed to download gem.");
+            return AZ::Failure<AZStd::pair<AZStd::string, AZStd::string>>(GetSimpleDetailedErrorPair());
         }
 
         return AZ::Success();
@@ -1302,4 +1321,23 @@ namespace O3DE::ProjectManager
 
         return result && updateAvaliableResult;
     }
+
+    AZStd::pair<AZStd::string, AZStd::string> PythonBindings::GetSimpleDetailedErrorPair()
+    {
+        AZStd::string detailedString = m_pythonErrorStrings.size() == 1
+            ? ""
+            : AZStd::accumulate(m_pythonErrorStrings.begin(), m_pythonErrorStrings.end(), AZStd::string(""));
+
+        return AZStd::pair<AZStd::string, AZStd::string>(m_pythonErrorStrings.front(), detailedString);
+    }
+
+    void PythonBindings::AddErrorString(AZStd::string errorString)
+    {
+        m_pythonErrorStrings.push_back(errorString);
+    }
+
+    void PythonBindings::ClearErrorStrings()
+    {
+        m_pythonErrorStrings.clear();
+    }
 }

Code/Tools/ProjectManager/Source/PythonBindings.h

@@ -62,15 +62,19 @@ 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::pair<AZStd::string, 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> GetGemInfosForRepo(const QString& repoUri) override;
         AZ::Outcome<QVector<GemInfo>, AZStd::string> GetGemInfosForAllRepos() override;
-        AZ::Outcome<void, AZStd::string> DownloadGem(const QString& gemName, std::function<void(int, int)> gemProgressCallback, bool force = false) override;
+        AZ::Outcome<void, AZStd::pair<AZStd::string, AZStd::string>> DownloadGem(
+            const QString& gemName, std::function<void(int, int)> gemProgressCallback, bool force = false) override;
         void CancelDownload() override;
         bool IsGemUpdateAvaliable(const QString& gemName, const QString& lastUpdated) override;
 
+        void AddErrorString(AZStd::string errorString) override;
+        void ClearErrorStrings() override;
+
     private:
         AZ_DISABLE_COPY_MOVE(PythonBindings);
 
@@ -83,6 +87,7 @@ namespace O3DE::ProjectManager
         AZ::Outcome<void, AZStd::string> GemRegistration(const QString& gemPath, const QString& projectPath, bool remove = false);
         bool RegisterThisEngine();
         bool StopPython();
+        AZStd::pair<AZStd::string, AZStd::string> GetSimpleDetailedErrorPair();
 
 
         bool m_pythonStarted = false;
@@ -102,5 +107,6 @@ namespace O3DE::ProjectManager
         pybind11::handle m_pathlib;
 
         bool m_requestCancelDownload = false;
+        AZStd::vector<AZStd::string> m_pythonErrorStrings;
     };
 }

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 pair of string error and detailed messages on failure.
          */
-        virtual bool AddGemRepo(const QString& repoUri) = 0;
+        virtual AZ::Outcome<void, AZStd::pair<AZStd::string, AZStd::string>> AddGemRepo(const QString& repoUri) = 0;
 
         /**
          * Unregisters this gem repo with the current engine.
@@ -235,9 +235,9 @@ namespace O3DE::ProjectManager
          * @param gemName the name of the Gem to download.
          * @param gemProgressCallback a callback function that is called with an int percentage download value.
          * @param force should we forcibly overwrite the old version of the gem.
-         * @return an outcome with a string error message on failure.
+         * @return an outcome with a pair of string error and detailed messages on failure.
          */
-        virtual AZ::Outcome<void, AZStd::string> DownloadGem(
+        virtual AZ::Outcome<void, AZStd::pair<AZStd::string, AZStd::string>> DownloadGem(
             const QString& gemName, std::function<void(int, int)> gemProgressCallback, bool force = false) = 0;
 
         /**
@@ -252,6 +252,17 @@ namespace O3DE::ProjectManager
          * @return true if update is avaliable, false if not.
          */
         virtual bool IsGemUpdateAvaliable(const QString& gemName, const QString& lastUpdated) = 0;
+
+        /**
+         * Add an error string to be returned when the current python call is complete.
+         * @param The error string to be displayed.
+         */
+        virtual void AddErrorString(AZStd::string errorString) = 0;
+
+        /**
+         * Clears the current list of error strings.
+         */
+        virtual void ClearErrorStrings() = 0;
     };
 
     using PythonBindingsInterface = AZ::Interface<IPythonBindings>;

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

@@ -23,6 +23,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()
 
@@ -113,7 +114,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:
@@ -166,6 +167,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:
+        try:
-            download_file_size = 0
+            with urllib.request.urlopen(parsed_uri.geturl()) as s:
-            try:
+                download_file_size = 0
-                download_file_size = s.headers['content-length']
+                try:
-            except KeyError:
+                    download_file_size = s.headers['content-length']
-                pass
+                except KeyError:
-            def download_progress(downloaded_bytes):
+                    pass
-                if download_progress_callback:
+                def download_progress(downloaded_bytes):
-                    return download_progress_callback(int(downloaded_bytes), int(download_file_size))
+                    if download_progress_callback:
-                return False
+                        return download_progress_callback(int(downloaded_bytes), int(download_file_size))
-            with download_path.open('wb') as f:
+                    return False
-                download_cancelled = copyfileobj(s, f, download_progress)
+                with download_path.open('wb') as f:
-                if download_cancelled:
+                    download_cancelled = copyfileobj(s, f, download_progress)
-                    return 1
+                    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