misc review feedback fixes

- formatting and spelling
- missing non-empty gem url test case
- not including project repos
- GitProvider Python class fixes
- re-order download params for better readability

Signed-off-by: Alex Peterson <[email protected]>

Code/Tools/ProjectManager/Source/GemCatalog/GemInspector.cpp

@@ -254,7 +254,7 @@ namespace O3DE::ProjectManager
             }
             else
             {
-                m_compatibilityTextLabel->setText(tr("This version has missing on incompatible gem dependencies"));
+                m_compatibilityTextLabel->setText(tr("This version has missing or incompatible gem dependencies"));
             }
         }
 

Code/Tools/ProjectManager/Source/GemCatalog/GemModel.cpp

@@ -163,7 +163,7 @@ namespace O3DE::ProjectManager
 
         if (mostCompatibleVersion.isEmpty() && !newVersionIsCompatible)
         {
-            // no compatibile versions available (yet) so refresh if version is the same or higher
+            // no compatible versions available (yet) so refresh if version is the same or higher
             return versionResult >= 0;
         }
 
@@ -533,14 +533,6 @@ namespace O3DE::ProjectManager
             return {};
         }
 
-        const GemInfo& gemInfo = versionList.at(0).value<GemInfo>();
-        if (gemInfo.IsCompatible())
-        {
-            // most common case is this gem is an engine gem,
-            // or the gem is compatible
-            return gemInfo.m_version;
-        }
-
         // versions are sorted from highest to lowest so return the first compatible version
         for (const auto& versionVariant : versionList)
         {
@@ -900,7 +892,7 @@ namespace O3DE::ProjectManager
             }
 
             // if this is a remote gem that hasn't been downloaded, show that the update is
-            // available the user has downloaded an older version
+            // available if the user has downloaded an older version
             if (gemInfo.m_gemOrigin == GemInfo::Remote &&
                 gemInfo.m_downloadStatus == GemInfo::NotDownloaded)
             {

Code/Tools/ProjectManager/Source/GemRepo/GemRepoInfo.h

@@ -29,7 +29,7 @@ namespace O3DE::ProjectManager
 
         bool operator<(const GemRepoInfo& gemRepoInfo) const;
 
-        enum BadgeType
+        enum class BadgeType
         {
             NoBadge = 0,
             BlueBadge,

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

@@ -55,7 +55,6 @@ namespace O3DE::ProjectManager
 
         vLayout->addWidget(m_contentStack);
 
-        //Reinit();
         m_notificationsView = AZStd::make_unique<AzToolsFramework::ToastNotificationsView>(this, AZ_CRC("ReposNotificationsView"));
         m_notificationsView->SetOffset(QPoint(10, 10));
         m_notificationsView->SetMaxQueuedNotifications(1);

Code/Tools/ProjectManager/Source/PythonBindingsInterface.h

@@ -245,7 +245,7 @@ namespace O3DE::ProjectManager
         virtual DetailedOutcome AddGemsToProject(const QStringList& gemPaths, const QStringList& gemNames, const QString& projectPath, bool force = false) = 0;
 
         /**
-         * Get gems that are incompatibile with this project 
+         * Get gems that are incompatible with this project 
          * @param gemPaths the absolute paths to the gems
          * @param gemNames the names of the gems to add with optional version specifiers
          * @param projectPath the absolute path to the project
@@ -254,7 +254,7 @@ namespace O3DE::ProjectManager
         virtual AZ::Outcome<QStringList, AZStd::string> GetIncompatibleProjectGems(const QStringList& gemPaths, const QStringList& gemNames, const QString& projectPath) = 0;
 
         /**
-         * Get objects that are incompatibile with the provided project and engine.
+         * Get objects that are incompatible with the provided project and engine.
          * The objects could be engine APIs or gems dependencies that might prevent this project from compiling
          * with the engine.
          * @param projectPath the absolute path to the project

scripts/o3de/o3de/download.py

@@ -129,7 +129,7 @@ def download_o3de_object(object_name: str, default_folder_name: str, dest_path:
     else:
         origin_uri = downloadable_object_data.get('download_source_uri')
         if not origin_uri:
-            # fallback to schemaVersion 1.0.0
+            # legacy repo.json schema used origin_uri instead of download_source_uri
             origin_uri = downloadable_object_data.get('origin_uri')
         
         if not origin_uri:
@@ -385,29 +385,29 @@ def add_parser_args(parser):
     :param parser: the caller passes an argparse parser like instance to this method
     """
     group = parser.add_mutually_exclusive_group(required=True)
-    group.add_argument('-e', '--engine-name', type=str, required=False,
+    group.add_argument('--engine-name', '-e', type=str, required=False,
                        help='Downloadable engine name.')
-    group.add_argument('-p', '--project-name', type=str, required=False,
+    group.add_argument('--project-name', '-p', type=str, required=False,
                        help='Downloadable project name with optional version specifier e.g. project==1.2.3\nIf no version specifier is provided, the most recent version will be downloaded.')
-    group.add_argument('-g', '--gem-name', type=str, required=False,
+    group.add_argument('--gem-name', '-g', type=str, required=False,
                        help='Downloadable gem name with optional version specifier e.g. gem==1.2.3\nIf no version specifier is provided, the most recent version will be downloaded.')
-    group.add_argument('-t', '--template-name', type=str, required=False,
+    group.add_argument('--template-name', '-t', type=str, required=False,
                        help='Downloadable template name with optional version specifier e.g. template==1.2.3\nIf no version specifier is provided, the most recent version will be downloaded.')
-    parser.add_argument('-dp', '--dest-path', type=str, required=False,
+    parser.add_argument('--dest-path', '-dp', type=str, required=False,
                             default=None,
                             help='Optional destination folder to download into.'
                                  ' i.e. download --project-name "CustomProject" --dest-path "C:/projects"'
                                  ' will result in C:/projects/CustomProject'
                                  ' If blank will download to default object type folder')
-    parser.add_argument('-sar', '--skip-auto-register', action='store_true', required=False,
+    parser.add_argument('--skip-auto-register', '-sar', action='store_true', required=False,
                             default=False,
                             help = 'Skip the automatic registration of new object download')
-    parser.add_argument('-f', '--force', action='store_true', required=False,
+    parser.add_argument('--force', '-f', action='store_true', required=False,
                             default=False,
                             help = 'Force overwrite the current object')
-    parser.add_argument('-s', '--use-source-control', action='store_true', required=False,
+    parser.add_argument('--use-source-control', '--src', action='store_true', required=False,
                             default=False,
-                            help = 'Acquire from source control instead of downloading a .zip archive.  Requires that the object has ')
+                            help = 'Acquire from source control instead of downloading a .zip archive.  Requires that the object has a valid source_control_uri.')
 
     parser.set_defaults(func=_run_download)
 

scripts/o3de/o3de/git_utils.py

@@ -22,11 +22,11 @@ logger = logging.getLogger('o3de.git_utils')
 logging.basicConfig(format=LOG_FORMAT)
 
 class GenericGitProvider(gitproviderinterface.GitProviderInterface):
-    def get_specific_file_uri(parsed_uri):
+    def get_specific_file_uri(self, parsed_uri: ParseResult):
         logger.warning(f"GenericGitProvider does not yet support retrieving individual files")
         return parsed_uri
 
-    def clone_from_git(uri, download_path: pathlib.Path, force_overwrite: bool = False, ref: str = None) -> int:
+    def clone_from_git(self, uri, download_path: pathlib.Path, force_overwrite: bool = False, ref: str = None) -> int:
         """
         :param uri: uniform resource identifier
         :param download_path: location path on disk to download file
@@ -64,11 +64,11 @@ class GenericGitProvider(gitproviderinterface.GitProviderInterface):
 
         return proc.returncode
 
-def get_generic_git_provider(parsed_uri:ParseResult) -> GenericGitProvider or None:
+def get_generic_git_provider(parsed_uri: ParseResult) -> GenericGitProvider or None:
     # the only requirement we have is one of the path components ends in .git
     # this could be relaxed further 
     if parsed_uri.netloc and parsed_uri.scheme and parsed_uri.path:
         for element in parsed_uri.path.split('/'):
             if element.strip().endswith(".git"):
-                return GenericGitProvider
+                return GenericGitProvider()
     return None

scripts/o3de/o3de/github_utils.py

@@ -25,7 +25,7 @@ logger = logging.getLogger('o3de.github_utils')
 logging.basicConfig(format=LOG_FORMAT)
 
 class GitHubProvider(gitproviderinterface.GitProviderInterface):
-    def get_specific_file_uri(parsed_uri:ParseResult):
+    def get_specific_file_uri(self, parsed_uri: ParseResult):
         components = parsed_uri.path.split('/')
         components = [ele for ele in components if ele.strip()]
 
@@ -49,7 +49,7 @@ class GitHubProvider(gitproviderinterface.GitProviderInterface):
 
         return parsed_uri
 
-    def clone_from_git(uri:ParseResult, download_path: pathlib.Path, force_overwrite: bool = False, ref: str = None) -> int:
+    def clone_from_git(self, uri: ParseResult, download_path: pathlib.Path, force_overwrite: bool = False, ref: str = None) -> int:
         """
         :param uri: uniform resource identifier
         :param download_path: location path on disk to download file
@@ -87,7 +87,7 @@ class GitHubProvider(gitproviderinterface.GitProviderInterface):
 
         return proc.returncode
 
-def get_github_provider(parsed_uri:ParseResult) -> GitHubProvider or None:
+def get_github_provider(parsed_uri: ParseResult) -> GitHubProvider or None:
     if 'github.com' in parsed_uri.netloc:
         components = parsed_uri.path.split('/')
         components = [ele for ele in components if ele.strip()]

scripts/o3de/o3de/gitproviderinterface.py

@@ -15,7 +15,7 @@ from urllib.parse import ParseResult
 
 class GitProviderInterface(abc.ABC):
     @abc.abstractmethod
-    def get_specific_file_uri(parsed_uri:ParseResult):
+    def get_specific_file_uri(self, parsed_uri: ParseResult):
         """
         Gets a uri that can be used to download a resource if the provide allows it, otherwise returns the unmodified uri
         :param uri: uniform resource identifier to get a downloadable link for
@@ -24,7 +24,7 @@ class GitProviderInterface(abc.ABC):
         pass
 
     @abc.abstractmethod
-    def clone_from_git(uri:ParseResult, download_path: pathlib.Path, force_overwrite: bool = False, ref: str = None) -> int:
+    def clone_from_git(self, uri: ParseResult, download_path: pathlib.Path, force_overwrite: bool = False, ref: str = None) -> int:
         """
         Clones a git repository from a uri into a given folder
         :param uri: uniform resource identifier

scripts/o3de/o3de/manifest.py

@@ -260,17 +260,30 @@ def get_manifest_restricted() -> list:
     return json_data['restricted'] if 'restricted' in json_data else []
 
 
-def get_manifest_repos() -> list:
+def get_manifest_repos(project_path: pathlib.Path = None) -> list:
     repos = set()
 
-    engine_json_data = get_engine_json_data(engine_path=get_this_engine_path())
-    if engine_json_data:
-        repos.update(set(engine_json_data.get('repos', [])))
+    if project_path:
+        project_json_data = get_project_json_data(project_path=project_path)
+        if project_json_data:
+            repos.update(set(project_json_data.get('repos', [])))
+
+        # if a project is provided we only want the repos from the project's engine
+        engine_path = get_project_engine_path(project_path=project_path)
+    else:
+        # no project path provided, use the current engine's repos
+        engine_path = get_this_engine_path()
+
+    if engine_path:
+        engine_json_data = get_engine_json_data(engine_path=engine_path)
+        if engine_json_data:
+            repos.update(set(engine_json_data.get('repos', [])))
 
     manifest_json_data = load_o3de_manifest()
     if manifest_json_data:
         repos.update(set(manifest_json_data.get('repos', [])))
 
+
     return list(repos)
 
 
@@ -424,6 +437,10 @@ def get_project_enabled_gems(project_path: pathlib.Path, include_dependencies:bo
     gems listed in project.json and the deprecated enabled_gems.json
     """
     project_json_data = get_project_json_data(project_path=project_path)
+    if not project_json_data:
+        logger.error(f"Failed to get project json data for the project at '{project_path}'")
+        return None
+
     active_gem_names = project_json_data.get('gem_names',[])
     enabled_gems_file = get_enabled_gem_cmake_file(project_path=project_path)
     if enabled_gems_file and enabled_gems_file.is_file():
@@ -482,7 +499,7 @@ def get_project_enabled_gems(project_path: pathlib.Path, include_dependencies:bo
             gem_name = gem.gem_json_data['gem_name']
             gem_name_with_specifier = gem_names_with_version_specifiers.get(gem_name,gem_name)
             if gem_name_with_specifier in result or include_dependencies:
-                result[gem_name_with_specifier] = gem.gem_json_data['path'].as_posix()
+                result[gem_name_with_specifier] = gem.gem_json_data['path'].as_posix() if gem.gem_json_data['path'] else None
     else:
         # Likely there is no resolution because gems are missing or wrong version
         # Provide the paths for the gems that are available

scripts/o3de/o3de/utils.py

@@ -206,7 +206,7 @@ def backup_folder(folder: str or pathlib.Path) -> None:
                 renamed = True
 
 
-def get_git_provider(parsed_uri:ParseResult):
+def get_git_provider(parsed_uri: ParseResult):
     """
     Returns a git provider if one exists given the passed uri
     :param parsed_uri: uniform resource identifier of a possible git repository
@@ -222,7 +222,7 @@ def get_git_provider(parsed_uri:ParseResult):
     return git_provider
 
 
-def download_file(parsed_uri:ParseResult, download_path: pathlib.Path, force_overwrite: bool = False, object_name: str = "", download_progress_callback = None) -> int:
+def download_file(parsed_uri: ParseResult, download_path: pathlib.Path, force_overwrite: bool = False, object_name: str = "", download_progress_callback = None) -> int:
     """
     Download file
     :param parsed_uri: uniform resource identifier to zip file to download
@@ -231,25 +231,16 @@ def download_file(parsed_uri:ParseResult, download_path: pathlib.Path, force_ove
     :param object_name: name of the object being downloaded
     :param download_progress_callback: callback called with the download progress as a percentage, returns true to request to cancel the download
     """
-    file_exists = False
-    if download_path.is_file():
-        if not force_overwrite:
-            file_exists = True
-        else:
-            try:
-                os.unlink(download_path)
-            except OSError:
-                logger.error(f'Could not remove existing download path {download_path}.')
-                return 1
+    file_exists = download_path.is_file()
 
     if parsed_uri.scheme in ['http', 'https', 'ftp', 'ftps']:
         try:
             current_request = urllib.request.Request(parsed_uri.geturl())
             resume_position = 0
-            if not force_overwrite:
-                if file_exists:
-                    resume_position = os.path.getsize(download_path)
-                    current_request.add_header("If-Range", "bytes=%d-" % resume_position)
+            if file_exists and not force_overwrite:
+                resume_position = os.path.getsize(download_path)
+                current_request.add_header("If-Range", "bytes=%d-" % resume_position)
+
             with urllib.request.urlopen(current_request) as s:
                 download_file_size = int(s.headers.get('content-length',0))
 
@@ -265,6 +256,14 @@ def download_file(parsed_uri:ParseResult, download_path: pathlib.Path, force_ove
                 else:
                     logger.error(f'HTTP status {e.code} opening {parsed_uri.geturl()}')
                     return 1
+                
+                # remove the file only after we have a response from the server and something to replace it with
+                if file_exists and force_overwrite:
+                    try:
+                        os.unlink(download_path)
+                    except OSError:
+                        logger.error(f'Could not remove existing download path {download_path}.')
+                        return 1
 
                 def print_progress(downloaded, total_size):
                     end_ch = '\r'

scripts/o3de/tests/test_repo.py

@@ -78,7 +78,9 @@ TEST_O3DE_REPO_JSON_VERSION_2_PAYLOAD = '''
     "summary": "Studios Repository for the testgem3 Gem.",
     "additional_info": "",
     "last_updated": "2023-01-19",
-    "gems":[],
+    "gems":[
+        "https://legacygemrepo.com"
+    ],
     "gems_data": [{
         "gem_name": "testgem3",
         "display_name": "testgem3 2",