chore: improve version parsing a little (#389)

Co-authored-by: Ruben Arts <ruben.arts@hotmail.com>

Author: remimimimimi

backends/pixi-build-ros/src/pixi_build_ros/utils.py

@@ -2,6 +2,7 @@
 import os
 from itertools import chain
 from pathlib import Path
+from typing import Any
 
 from catkin_pkg.package import Package as CatkinPackage, parse_package_string, Dependency
 
@@ -85,14 +86,13 @@ def load_package_map_data(package_map_sources: list[PackageMappingSource]) -> di
     return result
 
 
-def rosdep_to_conda_package_name(
-    dep_name: str,
+def rosdep_to_conda_package_spec(
+    dep: Dependency,
     distro: Distro,
     host_platform: Platform,
     package_map_data: dict[str, PackageMapEntry],
-    spec_str: str = "",
 ) -> list[str]:
-    """Convert a ROS dependency name to a conda package name."""
+    """Convert a ROS dependency name to a conda package spec."""
     if host_platform.is_linux:
         target_platform = "linux"
     elif host_platform.is_windows:
@@ -102,23 +102,25 @@ def rosdep_to_conda_package_name(
     else:
         raise RuntimeError(f"Unsupported platform: {host_platform}")
 
-    if dep_name not in package_map_data:
+    spec_str = rosdep_nameless_matchspec(dep)
+
+    if dep.name not in package_map_data:
         # Package name isn't found in the package map, so we are going to assume it is a ROS package.
-        return [f"ros-{distro.name}-{dep_name.replace('_', '-')}{spec_str or ''}"]
+        return [f"ros-{distro.name}-{dep.name.replace('_', '-')}{spec_str or ''}"]
 
     # Dependency found in package map
 
     # Case 1: It's a custom ROS dependency
-    if "ros" in package_map_data[dep_name]:
-        return [f"ros-{distro.name}-{dep.replace('_', '-')}{spec_str}" for dep in package_map_data[dep_name]["ros"]]
+    if "ros" in package_map_data[dep.name]:
+        return [f"ros-{distro.name}-{dep.replace('_', '-')}{spec_str}" for dep in package_map_data[dep.name]["ros"]]
 
     # Case 2: It's a custom package name
-    elif "conda" in package_map_data[dep_name] or "robostack" in package_map_data[dep_name]:
+    elif "conda" in package_map_data[dep.name] or "robostack" in package_map_data[dep.name]:
         # determine key
-        key = "robostack" if "robostack" in package_map_data[dep_name] else "conda"
+        key = "robostack" if "robostack" in package_map_data[dep.name] else "conda"
 
         # Get the conda packages for the dependency
-        conda_packages = package_map_data[dep_name].get(key, [])
+        conda_packages = package_map_data[dep.name].get(key, [])
 
         if isinstance(conda_packages, dict):
             # TODO: Handle different platforms
@@ -148,61 +150,72 @@ def rosdep_to_conda_package_name(
                 else:
                     raise ValueError(
                         f"Version specifier can only be used for a package without constraint already present, "
-                        f"but found {conda_packages[0]} for {dep_name} "
+                        f"but found {conda_packages[0]} for {dep.name} "
                         f"in the package map."
                     )
             else:
                 raise ValueError(
                     f"Version specifier can only be used for one package, "
-                    f"but found {len(conda_packages)} packages for {dep_name} "
+                    f"but found {len(conda_packages)} packages for {dep.name} "
                     f"in the package map."
                 )
 
         return conda_packages + additional_packages
     else:
-        raise ValueError(f"Unknown package map entry: {dep_name}.")
-
-
-def _format_version_constraints_to_string(dependency: Dependency) -> str:
-    """Format the version constraints from a ros package.xml to a string"""
-    for version in [
-        dependency.version_eq,
-        dependency.version_gte,
-        dependency.version_gt,
-        dependency.version_lte,
-        dependency.version_lt,
-    ]:
+        raise ValueError(f"Unknown package map entry: {dep.name}.")
+
+
+def rosdep_nameless_matchspec(dep: Dependency) -> str:
+    """Format the version constraints from a ros package.xml to a nameless matchspec"""
+    right_ineq = [dep.version_lt, dep.version_lte]
+    left_ineq = [dep.version_gt, dep.version_gte]
+    eq = dep.version_eq
+
+    for version in left_ineq + right_ineq + [eq]:
         if version is None:
             continue
         elif version == "":
             raise ValueError(
-                f"Incorrect version specification in package.xml: '{dependency.name}': version is empty string (\"\")"
+                f"Incorrect version specification in package.xml: '{dep.name}': version is empty string (\"\")"
             )
         try:
             # check if we can parse the version
             Version(version)
         except TypeError as e:
             raise ValueError(
-                f"Incorrect version specification in package.xml: '{dependency.name}' at version '{version}' "
+                f"Incorrect version specification in package.xml: '{dep.name}' at version '{version}' "
             ) from e
 
-    if dependency.version_eq:
-        return f" =={dependency.version_eq}"
+    def not_none(p: Any) -> bool:
+        return p is not None
+
+    if all(map(not_none, right_ineq)):
+        raise ValueError(f"Dependency {dep.name} cannot be specified by both `<` and `<=`")
+    if all(map(not_none, left_ineq)):
+        raise ValueError(f"Dependency {dep.name} cannot be specified by both `>` and `>=`")
+
+    some_inequality = any(map(lambda p: p is not None, right_ineq + left_ineq))
+    if eq and some_inequality:
+        raise ValueError(f"Dependency {dep.name} cannot be specified by both `=` and some inequality")
 
-    version_string_list = []
-    if dependency.version_gte:
-        version_string_list.append(f">={dependency.version_gte}")
-    if dependency.version_gt:
-        version_string_list.append(f">{dependency.version_gt}")
-    if dependency.version_lte:
-        version_string_list.append(f"<={dependency.version_lte}")
-    if dependency.version_lt:
-        version_string_list.append(f"<{dependency.version_lt}")
+    if eq:
+        return f"=={eq}"
 
-    if not version_string_list:
-        return ""
+    pair = []
 
-    return " " + ",".join(version_string_list)
+    if dep.version_gt:
+        pair.append(f">{dep.version_gt}")
+    if dep.version_gte:
+        pair.append(f">={dep.version_gte}")
+
+    if dep.version_lt:
+        pair.append(f"<{dep.version_lt}")
+    if dep.version_lte:
+        pair.append(f"<={dep.version_lte}")
+
+    res = ",".join(pair)
+
+    return " " + res if len(pair) > 0 else res
 
 
 def package_xml_to_conda_requirements(
@@ -220,30 +233,22 @@ def package_xml_to_conda_requirements(
     build_deps += pkg.build_export_depends
     # Also add test dependencies, because they might be needed during build (i.e. for pytest/catch2 etc in CMake macros)
     build_deps += pkg.test_depends
-    build_deps = [
-        PackageNameWithSpec(name=d.name, spec=_format_version_constraints_to_string(d))
-        for d in build_deps
-        if d.evaluated_condition
-    ]
+    build_deps = [d for d in build_deps if d.evaluated_condition]
     # Add the ros_workspace dependency as a default build dependency for ros2 packages
     if not distro.check_ros1():
-        build_deps += [PackageNameWithSpec(name="ros_workspace")]
+        build_deps += [Dependency(name="ros_workspace")]
     conda_build_deps_chain = [
-        rosdep_to_conda_package_name(dep.name, distro, host_platform, package_map_data, dep.spec) for dep in build_deps
+        rosdep_to_conda_package_spec(dep, distro, host_platform, package_map_data) for dep in build_deps
     ]
     conda_build_deps = list(chain.from_iterable(conda_build_deps_chain))
 
     run_deps = pkg.run_depends
     run_deps += pkg.exec_depends
     run_deps += pkg.build_export_depends
     run_deps += pkg.buildtool_export_depends
-    run_deps = [
-        PackageNameWithSpec(d.name, spec=_format_version_constraints_to_string(d))
-        for d in run_deps
-        if d.evaluated_condition
-    ]
+    run_deps = [d for d in run_deps if d.evaluated_condition]
     conda_run_deps_chain = [
-        rosdep_to_conda_package_name(dep.name, distro, host_platform, package_map_data, dep.spec) for dep in run_deps
+        rosdep_to_conda_package_spec(dep, distro, host_platform, package_map_data) for dep in run_deps
     ]
     conda_run_deps = list(chain.from_iterable(conda_run_deps_chain))
 

backends/pixi-build-ros/tests/test_package_xml.py

@@ -2,6 +2,8 @@
 import tempfile
 from pathlib import Path
 
+from catkin_pkg.package import Dependency
+
 import pytest
 from pixi_build_backend.types.intermediate_recipe import Script
 
@@ -10,7 +12,7 @@
 from pixi_build_ros.utils import (
     convert_package_xml_to_catkin_package,
     package_xml_to_conda_requirements,
-    rosdep_to_conda_package_name,
+    rosdep_to_conda_package_spec,
     PackageMapEntry,
 )
 from pixi_build_backend.types.platform import Platform
@@ -197,7 +199,7 @@ def test_robostack_target_platform_linux(package_map: dict[str, PackageMapEntry]
     linux_platform = Platform("linux-64")
 
     # Test packages with platform-specific mappings
-    acl_packages = rosdep_to_conda_package_name("acl", distro, linux_platform, package_map)
+    acl_packages = rosdep_to_conda_package_spec(Dependency(name="acl"), distro, linux_platform, package_map)
     assert acl_packages == ["libacl"], f"Expected ['libacl'] for acl on Linux, got {acl_packages}"
 
 
@@ -207,7 +209,7 @@ def test_robostack_target_platform_osx(package_map: dict[str, PackageMapEntry],
     osx_platform = Platform("osx-64")
 
     # Test packages with platform-specific mappings
-    acl_packages = rosdep_to_conda_package_name("acl", distro, osx_platform, package_map)
+    acl_packages = rosdep_to_conda_package_spec(Dependency(name="acl"), distro, osx_platform, package_map)
     assert acl_packages == [], f"Expected [] for acl on macOS, got {acl_packages}"
 
 
@@ -217,7 +219,7 @@ def test_robostack_target_platform_windows(package_map: dict[str, PackageMapEntr
     win_platform = Platform("win-64")
 
     # Test packages with platform-specific mappings
-    binutils_packages = rosdep_to_conda_package_name("binutils", distro, win_platform, package_map)
+    binutils_packages = rosdep_to_conda_package_spec(Dependency(name="binutils"), distro, win_platform, package_map)
     assert binutils_packages == [], f"Expected [] for binutils on Windows, got {binutils_packages}"
 
 
@@ -228,9 +230,9 @@ def test_robostack_target_platform_cross_platform(package_map: dict[str, Package
     win_platform = Platform("win-64")
 
     # libudev-dev has different packages for each platform
-    linux_udev = rosdep_to_conda_package_name("libudev-dev", distro, linux_platform, package_map)
-    osx_udev = rosdep_to_conda_package_name("libudev-dev", distro, osx_platform, package_map)
-    win_udev = rosdep_to_conda_package_name("libudev-dev", distro, win_platform, package_map)
+    linux_udev = rosdep_to_conda_package_spec(Dependency(name="libudev-dev"), distro, linux_platform, package_map)
+    osx_udev = rosdep_to_conda_package_spec(Dependency(name="libudev-dev"), distro, osx_platform, package_map)
+    win_udev = rosdep_to_conda_package_spec(Dependency(name="libudev-dev"), distro, win_platform, package_map)
 
     assert linux_udev == [
         "libusb",
@@ -240,9 +242,9 @@ def test_robostack_target_platform_cross_platform(package_map: dict[str, Package
     assert win_udev == ["libusb"], f"Expected ['libusb'] for libudev-dev on Windows, got {win_udev}"
 
     # libomp-dev has different OpenMP implementations per platform
-    linux_omp = rosdep_to_conda_package_name("libomp-dev", distro, linux_platform, package_map)
-    osx_omp = rosdep_to_conda_package_name("libomp-dev", distro, osx_platform, package_map)
-    win_omp = rosdep_to_conda_package_name("libomp-dev", distro, win_platform, package_map)
+    linux_omp = rosdep_to_conda_package_spec(Dependency(name="libomp-dev"), distro, linux_platform, package_map)
+    osx_omp = rosdep_to_conda_package_spec(Dependency(name="libomp-dev"), distro, osx_platform, package_map)
+    win_omp = rosdep_to_conda_package_spec(Dependency(name="libomp-dev"), distro, win_platform, package_map)
 
     assert linux_omp == ["libgomp"], f"Expected ['libgomp'] for libomp-dev on Linux, got {linux_omp}"
     assert osx_omp == ["llvm-openmp"], f"Expected ['llvm-openmp'] for libomp-dev on macOS, got {osx_omp}"
@@ -256,9 +258,9 @@ def test_robostack_require_opengl_handling(package_map: dict[str, PackageMapEntr
     win_platform = Platform("win-64")
 
     # opengl package has REQUIRE_OPENGL handling
-    linux_opengl = rosdep_to_conda_package_name("opengl", distro, linux_platform, package_map)
-    osx_opengl = rosdep_to_conda_package_name("opengl", distro, osx_platform, package_map)
-    win_opengl = rosdep_to_conda_package_name("opengl", distro, win_platform, package_map)
+    linux_opengl = rosdep_to_conda_package_spec(Dependency(name="opengl"), distro, linux_platform, package_map)
+    osx_opengl = rosdep_to_conda_package_spec(Dependency(name="opengl"), distro, osx_platform, package_map)
+    win_opengl = rosdep_to_conda_package_spec(Dependency(name="opengl"), distro, win_platform, package_map)
 
     # According to the code, REQUIRE_OPENGL should be replaced with actual packages on Linux
     # and should add xorg packages for linux/osx/unix platforms
@@ -279,12 +281,66 @@ def test_robostack_require_opengl_handling(package_map: dict[str, PackageMapEntr
 def test_spec_with_entry_in_map(package_map: dict[str, PackageMapEntry], distro: Distro):
     """Test using a specifier string with a package which has already defined one in the package map"""
     with pytest.raises(ValueError) as excinfo:
-        rosdep_to_conda_package_name("xtensor", distro, Platform.current(), package_map, spec_str="==2.0")
+        rosdep_to_conda_package_spec(
+            Dependency(name="xtensor", version_eq="2.0"), distro, Platform.current(), package_map
+        )
     assert "Version specifier can only be used for a package without constraint already present" in str(excinfo)
 
 
 def test_spec_with_multiple_entries_in_map(package_map: dict[str, PackageMapEntry], distro: Distro):
     """Test using a specifier string with a package which has multiple packages defined one in the package map"""
     with pytest.raises(ValueError) as excinfo:
-        rosdep_to_conda_package_name("boost", distro, Platform.current(), package_map, spec_str="==2.0")
+        rosdep_to_conda_package_spec(
+            Dependency(name="boost", version_eq="2.0"), distro, Platform.current(), package_map
+        )
     assert "Version specifier can only be used for one package" in str(excinfo)
+
+
+def test_rosdep_to_conda_package_spec_doesnt_add_matchspec_for_special_rosdeps():
+    distro = Distro("jazzy")
+    host_platform = Platform("linux-64")
+    dep = Dependency(name="ament_cmake", version_eq="1.2.3")
+
+    packages = rosdep_to_conda_package_spec(dep, distro, host_platform, {})
+
+    assert packages == ["ros-jazzy-ament-cmake==1.2.3"]
+
+
+def test_rosdep_to_conda_package_spec_adds_matchspec_for_distro_packages():
+    distro = Distro("jazzy")
+    host_platform = Platform("linux-64")
+    dep = Dependency(name="rclcpp", version_gte="18.0.0", version_lt="20.0.0")
+
+    packages = rosdep_to_conda_package_spec(dep, distro, host_platform, {})
+
+    assert packages == ["ros-jazzy-rclcpp >=18.0.0,<20.0.0"]
+
+
+def test_rosdep_to_conda_package_spec_adds_matchspec_for_ros_package_map_entries():
+    distro = Distro("jazzy")
+    host_platform = Platform("linux-64")
+    package_map: dict[str, PackageMapEntry] = {"custom_ros_dep": {"ros": ["foo_util"]}}
+    dep = Dependency(name="custom_ros_dep", version_gte="3.1")
+
+    packages = rosdep_to_conda_package_spec(dep, distro, host_platform, package_map)
+
+    assert packages == ["ros-jazzy-foo-util >=3.1"]
+
+
+def test_rosdep_to_conda_package_spec_adds_matchspec_for_unknown_dependencies():
+    distro = Distro("jazzy")
+    host_platform = Platform("linux-64")
+    dep = Dependency(name="customlib", version_gt="1.0.0")
+
+    packages = rosdep_to_conda_package_spec(dep, distro, host_platform, {})
+
+    assert packages == ["ros-jazzy-customlib >1.0.0"]
+
+
+def test_rosdep_to_conda_package_spec_exception():
+    distro = Distro("jazzy")
+    host_platform = Platform("linux-64")
+    dep = Dependency(name="rclcpp", version_gt="18.0.0", version_gte="20.0.0")
+
+    with pytest.raises(ValueError, match=".* `>` and `>=`"):
+        rosdep_to_conda_package_spec(dep, distro, host_platform, {})

py-pixi-build-backend/src/types/generated_recipe.rs

@@ -254,7 +254,6 @@ impl GenerateRecipe for PyGenerateRecipe {
     ) -> miette::Result<BTreeSet<String>> {
         Python::attach(|py| {
             let workdir = workdir.as_ref();
-            let editable = editable;
 
             // we don't pass the wrapper but the python inner model directly
             let py_object = config.model.clone();