Merge pull request #8505 from fstagni/fix_install_not_release

aldbr · web-flow · commit e5bd32698484 · 2026-04-21T11:44:04.000+02:00
fix: install non-released versions: handle space
diff --git a/src/DIRAC/FrameworkSystem/Client/SystemAdministratorClientCLI.py b/src/DIRAC/FrameworkSystem/Client/SystemAdministratorClientCLI.py
@@ -1,6 +1,6 @@
 #!/usr/bin/env python
 ########################################################################
-""" System Administrator Client Command Line Interface """
+"""System Administrator Client Command Line Interface"""
 
 import atexit
 import datetime
@@ -1066,13 +1066,11 @@ def do_update(self, args):
 
           update <version>
         """
-        try:
-            version = args.split()[0]
-        except Exception as x:
-            gLogger.notice("ERROR: wrong input:", str(x))
+        version = args.strip()
+        if not version:
+            gLogger.notice("ERROR: no version specified")
             gLogger.notice(self.do_update.__doc__)
             return
-
         client = SystemAdministratorClient(self.host, self.port)
         gLogger.notice("Software update can take a while, please wait ...")
         result = client.updateSoftware(version, timeout=600)
diff --git a/src/DIRAC/FrameworkSystem/Service/SystemAdministratorHandler.py b/src/DIRAC/FrameworkSystem/Service/SystemAdministratorHandler.py
@@ -1,34 +1,33 @@
 """SystemAdministrator service is a tool to control and monitor the DIRAC services and agents"""
 
-import socket
-import os
-import re
-import time
 import getpass
 import importlib
-import shutil
+import os
 import platform
-import tempfile
+import re
+import shutil
+import socket
 import subprocess
+import tempfile
+import time
 from datetime import datetime, timedelta
-import requests
 
 import psutil
-from packaging.version import Version, InvalidVersion
-
+import requests
 from diraccfg import CFG
+from packaging.version import InvalidVersion, Version
 
-from DIRAC import S_OK, S_ERROR, gConfig, rootPath, gLogger, convertToPy3VersionNumber
+from DIRAC import S_ERROR, S_OK, convertToPy3VersionNumber, gConfig, gLogger, rootPath
+from DIRAC.ConfigurationSystem.Client import PathFinder
 from DIRAC.Core.DISET.RequestHandler import RequestHandler
+from DIRAC.Core.Security.Locations import getHostCertificateAndKeyLocation
+from DIRAC.Core.Security.X509Chain import X509Chain  # pylint: disable=import-error
 from DIRAC.Core.Utilities import Os
 from DIRAC.Core.Utilities.Extensions import extensionsByPriority, getExtensionMetadata
 from DIRAC.Core.Utilities.File import mkLink
-from DIRAC.Core.Utilities.TimeUtilities import fromString, hour, day
 from DIRAC.Core.Utilities.Subprocess import shellCall
 from DIRAC.Core.Utilities.ThreadScheduler import gThreadScheduler
-from DIRAC.Core.Security.Locations import getHostCertificateAndKeyLocation
-from DIRAC.Core.Security.X509Chain import X509Chain  # pylint: disable=import-error
-from DIRAC.ConfigurationSystem.Client import PathFinder
+from DIRAC.Core.Utilities.TimeUtilities import day, fromString, hour
 from DIRAC.FrameworkSystem.Client.ComponentInstaller import gComponentInstaller
 from DIRAC.FrameworkSystem.Client.ComponentMonitoringClient import ComponentMonitoringClient
 
@@ -48,6 +47,76 @@ def loadDIRACCFG():
     return S_OK((cfgPath, diracCFG))
 
 
+def _normalise_version(version):
+    """Validate and normalise a raw version string supplied by the operator.
+
+    :param str version: Raw string as received from the client (may contain surrounding
+        whitespace or use the spaced ``pkg @ url`` pip syntax).
+    :returns: A 4-tuple ``(version, primaryExtension, released_version, isPrerelease)`` where
+
+        - *version* is the normalised version string ready to be passed to pip,
+        - *primaryExtension* is the package name when the caller used
+          ``extension==version`` syntax, or ``None`` otherwise,
+        - *released_version* is ``True`` when installing a PEP 440 release and
+          ``False`` when installing from a VCS URL,
+        - *isPrerelease* is ``True`` when the PEP 440 version is a pre-release.
+
+    :rtype: tuple(str, str or None, bool, bool)
+    :raises ValueError: When the version string is empty, or is not a valid PEP 440
+        version and does not contain a recognised VCS URL.
+    """
+    version = version.strip()
+    if not version:
+        raise ValueError("No version specified")
+
+    primaryExtension = None
+    if "==" in version:
+        primaryExtension, version = version.split("==", 1)
+
+    released_version = True
+    isPrerelease = False
+
+    # Special aliases: install DIRAC from the integration branch
+    if version.lower() in ("integration", "devel", "master", "main"):
+        released_version = False
+        version = "DIRAC[server] @ git+https://github.com/DIRACGrid/DIRAC.git@integration"
+        return version, primaryExtension, released_version, isPrerelease
+
+    # Try to parse as a PEP 440 version number
+    try:
+        parsed = Version(version)
+        isPrerelease = parsed.is_prerelease
+        version = f"v{parsed}"
+    except InvalidVersion:
+        if "https://" in version:
+            # Treat as a VCS URL (e.g. "DIRAC[server] @ git+https://...")
+            released_version = False
+        else:
+            raise ValueError(f"Invalid version passed {version!r}")
+
+    return version, primaryExtension, released_version, isPrerelease
+
+
+def _directory_label(version, released_version):
+    """Derive the filesystem directory label for a given version.
+
+    For released versions this is the version string itself.  For VCS URLs
+    (pip ``pkg @ url`` syntax) it is the URL part, stripped of any
+    ``#egg=...`` fragment and surrounding whitespace.
+
+    :param str version: Normalised version string as returned by :func:`_normalise_version`.
+    :param bool released_version: ``True`` when *version* is a PEP 440 release string.
+    :returns: A filesystem-safe label derived from *version*.
+    :rtype: str
+    """
+    if released_version:
+        return version
+    # version is "pkg @ git+https://host/repo.git@branch"
+    # Split on the *first* "@" (the pip separator) only, then strip spaces
+    # and drop any "#egg=..." fragment so the branch name is preserved.
+    return version.split("@", 1)[1].strip().split("#")[0]
+
+
 class SystemAdministratorHandler(RequestHandler):
     @classmethod
     def initializeHandler(cls, serviceInfo):
@@ -263,29 +332,11 @@ def export_updateSoftware(self, version):
         - a git tag/branch like "DIRAC[server] @ git+https://github.com/fstagni/DIRAC.git@test_branch"
         """
         # Validate and normalise the requested version
-        primaryExtension = None
-        if "==" in version:
-            primaryExtension, version = version.split("==")
-
-        released_version = True
-        isPrerelease = False
-
-        # Special cases (e.g. installing the integration/main branch)
-        if version.lower() in ["integration", "devel", "master", "main"]:
-            released_version = False
-            version = "DIRAC[server] @ git+https://github.com/DIRACGrid/DIRAC.git@integration"
-
-        if released_version:
-            try:
-                version = Version(version)
-                isPrerelease = version.is_prerelease
-                version = f"v{version}"
-            except InvalidVersion:
-                if "https://" in version:
-                    released_version = False
-                else:
-                    self.log.exception("Invalid version passed", version)
-                    return S_ERROR(f"Invalid version passed {version!r}")
+        try:
+            version, primaryExtension, released_version, isPrerelease = _normalise_version(version)
+        except ValueError as e:
+            self.log.exception("Invalid version passed", version)
+            return S_ERROR(str(e))
 
         # Find what to install
         otherExtensions = []
@@ -311,7 +362,7 @@ def export_updateSoftware(self, version):
             installer.flush()
             self.log.info("Downloaded DIRACOS installer to", installer.name)
 
-            directory = version if released_version else version.split("@")[1].split("#")[0]
+            directory = _directory_label(version, released_version)
             newProPrefix = os.path.join(
                 rootPath,
                 "versions",
diff --git a/src/DIRAC/FrameworkSystem/Service/test/Test_SystemAdministratorHandler.py b/src/DIRAC/FrameworkSystem/Service/test/Test_SystemAdministratorHandler.py
@@ -0,0 +1,177 @@
+"""Unit tests for SystemAdministratorHandler helper functions and
+the SystemAdministratorClientCLI do_update input validation.
+"""
+
+import pytest
+from unittest.mock import MagicMock, patch
+
+from DIRAC.FrameworkSystem.Service.SystemAdministratorHandler import (
+    _directory_label,
+    _normalise_version,
+)
+
+
+# ---------------------------------------------------------------------------
+# Tests: _normalise_version
+# ---------------------------------------------------------------------------
+
+
+class TestNormaliseVersion:
+    def test_empty_string_raises(self):
+        with pytest.raises(ValueError, match="No version specified"):
+            _normalise_version("")
+
+    def test_whitespace_only_raises(self):
+        with pytest.raises(ValueError, match="No version specified"):
+            _normalise_version("   ")
+
+    def test_released_version(self):
+        version, primary, released, pre = _normalise_version("9.0.18")
+        assert released is True
+        assert pre is False
+        assert version == "v9.0.18"
+        assert primary is None
+
+    def test_released_prerelease_version(self):
+        version, primary, released, pre = _normalise_version("9.0.18a1")
+        assert released is True
+        assert pre is True
+        assert version == "v9.0.18a1"
+
+    def test_extension_syntax_splits_primary(self):
+        version, primary, released, pre = _normalise_version("MyExtension==9.0.18")
+        assert primary == "MyExtension"
+        assert version == "v9.0.18"
+        assert released is True
+
+    @pytest.mark.parametrize("keyword", ["integration", "devel", "master", "main"])
+    def test_special_keywords(self, keyword):
+        version, primary, released, pre = _normalise_version(keyword)
+        assert released is False
+        assert pre is False
+        assert "DIRACGrid/DIRAC" in version
+        assert "@integration" in version
+
+    def test_git_url_without_spaces(self):
+        raw = "DIRAC[server]@git+https://github.com/fstagni/DIRAC.git@test_branch"
+        version, primary, released, pre = _normalise_version(raw)
+        assert released is False
+        assert version == raw
+
+    def test_git_url_with_spaces_around_at(self):
+        """Leading/trailing whitespace is stripped; internal pip spaces are kept."""
+        raw = "DIRAC[server] @ git+https://github.com/fstagni/DIRAC.git@test_branch"
+        version, primary, released, pre = _normalise_version(f"  {raw}  ")
+        assert released is False
+        assert version == raw
+
+    def test_invalid_version_no_url_raises(self):
+        with pytest.raises(ValueError, match="Invalid version passed"):
+            _normalise_version("not-a-valid-version")
+
+
+# ---------------------------------------------------------------------------
+# Tests: _directory_label
+# ---------------------------------------------------------------------------
+
+
+class TestDirectoryLabel:
+    def test_released_version_uses_version_directly(self):
+        assert _directory_label("v9.0.18", released_version=True) == "v9.0.18"
+
+    def test_git_url_without_spaces(self):
+        """Branch name after the second '@' must be preserved."""
+        version = "DIRAC[server]@git+https://github.com/fstagni/DIRAC.git@test_branch"
+        assert (
+            _directory_label(version, released_version=False) == "git+https://github.com/fstagni/DIRAC.git@test_branch"
+        )
+
+    def test_git_url_with_spaces_around_pip_separator(self):
+        """Spaces around the pip '@' separator are stripped; branch part kept."""
+        version = "DIRAC[server] @ git+https://github.com/fstagni/DIRAC.git@test_branch"
+        assert (
+            _directory_label(version, released_version=False) == "git+https://github.com/fstagni/DIRAC.git@test_branch"
+        )
+
+    def test_git_url_with_hash_fragment(self):
+        """#egg= fragment must be stripped from the directory label."""
+        version = "DIRAC[server]@git+https://github.com/DIRACGrid/DIRAC.git@integration#egg=DIRAC"
+        assert (
+            _directory_label(version, released_version=False)
+            == "git+https://github.com/DIRACGrid/DIRAC.git@integration"
+        )
+
+
+# ---------------------------------------------------------------------------
+# Tests: CLI do_update input validation
+# ---------------------------------------------------------------------------
+
+
+class TestDoUpdate:
+    """Test SystemAdministratorClientCLI.do_update input validation."""
+
+    def _make_cli(self):
+        from DIRAC.FrameworkSystem.Client.SystemAdministratorClientCLI import SystemAdministratorClientCLI
+
+        cli = SystemAdministratorClientCLI.__new__(SystemAdministratorClientCLI)
+        cli.host = "localhost"
+        cli.port = 9162
+        return cli
+
+    def test_empty_args_prints_usage_and_returns(self):
+        cli = self._make_cli()
+        with (
+            patch(
+                "DIRAC.FrameworkSystem.Client.SystemAdministratorClientCLI.SystemAdministratorClient"
+            ) as mock_client_cls,
+            patch("DIRAC.FrameworkSystem.Client.SystemAdministratorClientCLI.gLogger") as mock_logger,
+        ):
+            cli.do_update("")
+            mock_client_cls.assert_not_called()
+            assert mock_logger.notice.called
+
+    def test_whitespace_only_args_does_not_contact_server(self):
+        cli = self._make_cli()
+        with (
+            patch(
+                "DIRAC.FrameworkSystem.Client.SystemAdministratorClientCLI.SystemAdministratorClient"
+            ) as mock_client_cls,
+            patch("DIRAC.FrameworkSystem.Client.SystemAdministratorClientCLI.gLogger"),
+        ):
+            cli.do_update("   ")
+            mock_client_cls.assert_not_called()
+
+    def test_valid_version_calls_client(self):
+        cli = self._make_cli()
+        with (
+            patch(
+                "DIRAC.FrameworkSystem.Client.SystemAdministratorClientCLI.SystemAdministratorClient"
+            ) as mock_client_cls,
+            patch("DIRAC.FrameworkSystem.Client.SystemAdministratorClientCLI.gLogger"),
+        ):
+            mock_instance = MagicMock()
+            mock_instance.updateSoftware.return_value = {"OK": True, "Value": None}
+            mock_client_cls.return_value = mock_instance
+
+            cli.do_update("9.0.18")
+
+            mock_client_cls.assert_called_once_with(cli.host, cli.port)
+            mock_instance.updateSoftware.assert_called_once_with("9.0.18", timeout=600)
+
+    def test_git_url_with_spaces_passes_full_version_to_server(self):
+        """The version body (including internal spaces) is forwarded as-is."""
+        cli = self._make_cli()
+        with (
+            patch(
+                "DIRAC.FrameworkSystem.Client.SystemAdministratorClientCLI.SystemAdministratorClient"
+            ) as mock_client_cls,
+            patch("DIRAC.FrameworkSystem.Client.SystemAdministratorClientCLI.gLogger"),
+        ):
+            mock_instance = MagicMock()
+            mock_instance.updateSoftware.return_value = {"OK": True, "Value": None}
+            mock_client_cls.return_value = mock_instance
+
+            raw = "DIRAC[server] @ git+https://github.com/fstagni/DIRAC.git@test_branch"
+            cli.do_update(raw)
+
+            mock_instance.updateSoftware.assert_called_once_with(raw, timeout=600)
diff --git a/src/DIRAC/FrameworkSystem/Service/test/__init__.py b/src/DIRAC/FrameworkSystem/Service/test/__init__.py
