From 4ee14db4d3d0b1b05e3ce528df48bdb2fd23b9ad Mon Sep 17 00:00:00 2001 From: Tim de Jager Date: Mon, 20 Oct 2025 11:00:05 +0200 Subject: [PATCH 1/6] fix: some misc backend issues --- .../src/pixi_build_ros/config.py | 4 +++- .../src/pixi_build_ros/metadata_provider.py | 21 ++++++++++++++++--- .../src/pixi_build_ros/ros_generator.py | 1 + .../pixi-build-ros/templates/build_catkin.sh | 3 +-- .../tests/test_meta_data_provider.py | 8 ++++--- .../src/intermediate_backend.rs | 8 +++++-- 6 files changed, 34 insertions(+), 11 deletions(-) diff --git a/backends/pixi-build-ros/src/pixi_build_ros/config.py b/backends/pixi-build-ros/src/pixi_build_ros/config.py index dfd7ffee..b3995b4d 100644 --- a/backends/pixi-build-ros/src/pixi_build_ros/config.py +++ b/backends/pixi-build-ros/src/pixi_build_ros/config.py @@ -65,7 +65,9 @@ class ROSBackendConfig(pydantic.BaseModel, extra="forbid", arbitrary_types_allow # Environment variables to set during the build env: dict[str, str] | None = None # Directory for debug files of this script - debug_dir: Path | None = pydantic.Field(default=None, alias="debug-dir") + debug_dir: Path | None = pydantic.Field( + default=None, validation_alias=pydantic.AliasChoices("debug-dir", "debug_dir") + ) # Extra input globs to include in the build hash extra_input_globs: list[str] | None = pydantic.Field(default=None, alias="extra-input-globs") diff --git a/backends/pixi-build-ros/src/pixi_build_ros/metadata_provider.py b/backends/pixi-build-ros/src/pixi_build_ros/metadata_provider.py index 6664ff0e..51fcf2d5 100644 --- a/backends/pixi-build-ros/src/pixi_build_ros/metadata_provider.py +++ b/backends/pixi-build-ros/src/pixi_build_ros/metadata_provider.py @@ -47,6 +47,7 @@ class PackageXmlMetadataProvider(MetadataProvider): # type: ignore[misc] # Met def __init__( # type: ignore[no-untyped-def] # no typing for args and kwargs self, package_xml_path: str, + manifest_root: str, *args, extra_input_globs: list[str] | None = None, **kwargs, @@ -56,9 +57,11 @@ def __init__( # type: ignore[no-untyped-def] # no typing for args and kwargs Args: package_xml_path: Path to the package.xml file + manifest_root: Path to the manifest root directory """ super().__init__(*args, **kwargs) self.package_xml_path = package_xml_path + self.manifest_root = manifest_root self._package_data: PackageData | None = None self._extra_input_globs = list(extra_input_globs or []) # Early load the package.xml data to ensure it's valid @@ -144,8 +147,18 @@ def license(self) -> str | None: return None def license_file(self) -> str | None: - """Return package.xml as the license files.""" - return "package.xml" + """Return package.xml as the license files, relative to manifest_root.""" + from pathlib import Path + + package_xml_abs = Path(self.package_xml_path).resolve() + manifest_root_abs = Path(self.manifest_root).resolve() + + try: + relative_path = package_xml_abs.relative_to(manifest_root_abs) + return str(relative_path) + except ValueError: + # If package_xml is not relative to manifest_root (i.e., higher in the directory tree), return None + return None def summary(self) -> str | None: """Return the description as summary from package.xml.""" @@ -184,6 +197,7 @@ class ROSPackageXmlMetadataProvider(PackageXmlMetadataProvider): def __init__( self, package_xml_path: str, + manifest_root: str, distro_name: str | None = None, *, extra_input_globs: list[str] | None = None, @@ -193,9 +207,10 @@ def __init__( Args: package_xml_path: Path to the package.xml file + manifest_root: Path to the manifest root directory distro: ROS distro. If None, will use the base package name without distro prefix. """ - super().__init__(package_xml_path, extra_input_globs=extra_input_globs) + super().__init__(package_xml_path, manifest_root, extra_input_globs=extra_input_globs) self._distro_name: str | None = distro_name def name(self) -> str | None: diff --git a/backends/pixi-build-ros/src/pixi_build_ros/ros_generator.py b/backends/pixi-build-ros/src/pixi_build_ros/ros_generator.py index cf15a75c..f3f272fe 100644 --- a/backends/pixi-build-ros/src/pixi_build_ros/ros_generator.py +++ b/backends/pixi-build-ros/src/pixi_build_ros/ros_generator.py @@ -50,6 +50,7 @@ def generate_recipe( package_xml_path = manifest_root / "package.xml" metadata_provider = ROSPackageXmlMetadataProvider( str(package_xml_path), + str(manifest_root), backend_config.distro.name, extra_input_globs=list(backend_config.extra_input_globs or []), ) diff --git a/backends/pixi-build-ros/templates/build_catkin.sh b/backends/pixi-build-ros/templates/build_catkin.sh index e0968210..4f6044f0 100644 --- a/backends/pixi-build-ros/templates/build_catkin.sh +++ b/backends/pixi-build-ros/templates/build_catkin.sh @@ -14,8 +14,7 @@ if [ "${PKG_NAME}" == "ros-noetic-catkin" ]; then CATKIN_BUILD_BINARY_PACKAGE="OFF" fi -rm -rf build -mkdir build +mkdir -p build cd build # necessary for correctly linking SIP files (from python_qt_bindings) diff --git a/backends/pixi-build-ros/tests/test_meta_data_provider.py b/backends/pixi-build-ros/tests/test_meta_data_provider.py index 108c20d6..71c74e01 100644 --- a/backends/pixi-build-ros/tests/test_meta_data_provider.py +++ b/backends/pixi-build-ros/tests/test_meta_data_provider.py @@ -10,25 +10,27 @@ def test_metadata_provider(package_xmls: Path): """Test the MetaDataProvider class.""" package_xml_path = package_xmls / "custom_ros.xml" - metadata_provider = PackageXmlMetadataProvider(str(package_xml_path)) + metadata_provider = PackageXmlMetadataProvider(str(package_xml_path), str(package_xmls)) assert metadata_provider.name() == "custom_ros" assert metadata_provider.version() == "0.0.1" assert metadata_provider.license() == "LicenseRef-Apache License 2.0" assert metadata_provider.description() == "Demo" assert metadata_provider.homepage() == "https://test.io/custom_ros" assert metadata_provider.repository() == "https://github.com/test/custom_ros" + assert metadata_provider.license_file() == "custom_ros.xml" def test_ros_metadata_provider(package_xmls: Path): """Test the RosMetaDataProvider class.""" package_xml_path = package_xmls / "custom_ros.xml" - metadata_provider = ROSPackageXmlMetadataProvider(str(package_xml_path), distro_name="noetic") + metadata_provider = ROSPackageXmlMetadataProvider(str(package_xml_path), str(package_xmls), distro_name="noetic") assert metadata_provider.name() == "ros-noetic-custom-ros" assert metadata_provider.version() == "0.0.1" assert metadata_provider.license() == "LicenseRef-Apache License 2.0" assert metadata_provider.description() == "Demo" assert metadata_provider.homepage() == "https://test.io/custom_ros" assert metadata_provider.repository() == "https://github.com/test/custom_ros" + assert metadata_provider.license_file() == "custom_ros.xml" def test_metadata_provider_raises_on_broken_xml(package_xmls: Path): @@ -36,7 +38,7 @@ def test_metadata_provider_raises_on_broken_xml(package_xmls: Path): broken_xml_path = package_xmls / "broken.xml" with pytest.raises(RuntimeError) as exc_info: - ROSPackageXmlMetadataProvider(str(broken_xml_path), distro_name="noetic") + ROSPackageXmlMetadataProvider(str(broken_xml_path), str(package_xmls), distro_name="noetic") # Verify the exception contains location information error = exc_info.value diff --git a/crates/pixi-build-backend/src/intermediate_backend.rs b/crates/pixi-build-backend/src/intermediate_backend.rs index 55e9e7a6..f070c884 100644 --- a/crates/pixi-build-backend/src/intermediate_backend.rs +++ b/crates/pixi-build-backend/src/intermediate_backend.rs @@ -130,9 +130,13 @@ impl IntermediateBackend { (source_dir, manifest_rel_path) } Some(source_dir) => { - let manifest_rel_path = pathdiff::diff_paths(manifest_path, &source_dir) + let manifest_rel_path = pathdiff::diff_paths(&manifest_path, &source_dir) .ok_or_else(|| { - miette::miette!("the manifest is not relative to the source directory") + miette::miette!( + "the manifest: {} is not relative to the source directory: {}", + manifest_path.display(), + source_dir.display() + ) })?; (source_dir, manifest_rel_path) } From 8e0defc9da83b09b50214eeed72797f7369d1741 Mon Sep 17 00:00:00 2001 From: Tim de Jager Date: Mon, 20 Oct 2025 11:36:05 +0200 Subject: [PATCH 2/6] fix test --- backends/pixi-build-ros/tests/test_meta_data_provider.py | 1 + 1 file changed, 1 insertion(+) diff --git a/backends/pixi-build-ros/tests/test_meta_data_provider.py b/backends/pixi-build-ros/tests/test_meta_data_provider.py index 690d6054..474f3823 100644 --- a/backends/pixi-build-ros/tests/test_meta_data_provider.py +++ b/backends/pixi-build-ros/tests/test_meta_data_provider.py @@ -87,6 +87,7 @@ def test_metadata_provider_includes_package_mapping_files_in_input_globs(): # Create metadata provider metadata_provider = ROSPackageXmlMetadataProvider( str(package_xml_path), + str(temp_path), distro_name="noetic", package_mapping_files=package_mapping_files, ) From f20208afbac6442d8b1650c350aec88457fdeed8 Mon Sep 17 00:00:00 2001 From: Tim de Jager Date: Mon, 20 Oct 2025 12:38:58 +0200 Subject: [PATCH 3/6] fix: remove license stuff --- .../src/pixi_build_ros/metadata_provider.py | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/backends/pixi-build-ros/src/pixi_build_ros/metadata_provider.py b/backends/pixi-build-ros/src/pixi_build_ros/metadata_provider.py index a8b84be4..73ffca1b 100644 --- a/backends/pixi-build-ros/src/pixi_build_ros/metadata_provider.py +++ b/backends/pixi-build-ros/src/pixi_build_ros/metadata_provider.py @@ -152,17 +152,8 @@ def license(self) -> str | None: def license_file(self) -> str | None: """Return package.xml as the license files, relative to manifest_root.""" - from pathlib import Path - - package_xml_abs = Path(self.package_xml_path).resolve() - manifest_root_abs = Path(self.manifest_root).resolve() - - try: - relative_path = package_xml_abs.relative_to(manifest_root_abs) - return str(relative_path) - except ValueError: - # If package_xml is not relative to manifest_root (i.e., higher in the directory tree), return None - return None + # TODO: This does not work currently, so return None + return None def summary(self) -> str | None: """Return the description as summary from package.xml.""" From f904db1d431e39f38d5a1312c6ed00d6c44b868b Mon Sep 17 00:00:00 2001 From: Tim de Jager Date: Mon, 20 Oct 2025 14:09:23 +0200 Subject: [PATCH 4/6] fix: return None for license_file() as it doesn't work currently - Modified license_file() method to return None with TODO comment - Updated test assertions to expect None instead of filename - Removed broken license_file tests - Updated snapshots to reflect missing license_file field --- .../tests/__snapshots__/test_package_xml.ambr | 2 +- .../tests/__snapshots__/test_version_constraints.ambr | 6 +++--- backends/pixi-build-ros/tests/test_meta_data_provider.py | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/backends/pixi-build-ros/tests/__snapshots__/test_package_xml.ambr b/backends/pixi-build-ros/tests/__snapshots__/test_package_xml.ambr index aca58249..4133fae1 100644 --- a/backends/pixi-build-ros/tests/__snapshots__/test_package_xml.ambr +++ b/backends/pixi-build-ros/tests/__snapshots__/test_package_xml.ambr @@ -124,7 +124,7 @@ about: homepage: https://test.io/custom_ros license: LicenseRef-Apache License 2.0 - license_file: package.xml + license_file: null summary: Demo description: Demo documentation: null diff --git a/backends/pixi-build-ros/tests/__snapshots__/test_version_constraints.ambr b/backends/pixi-build-ros/tests/__snapshots__/test_version_constraints.ambr index 1f182c1b..820fc309 100644 --- a/backends/pixi-build-ros/tests/__snapshots__/test_version_constraints.ambr +++ b/backends/pixi-build-ros/tests/__snapshots__/test_version_constraints.ambr @@ -68,7 +68,7 @@ about: homepage: null license: LicenseRef-Apache License 2.0 - license_file: package.xml + license_file: null summary: Demo description: Demo documentation: null @@ -145,7 +145,7 @@ about: homepage: null license: LicenseRef-Apache License 2.0 - license_file: package.xml + license_file: null summary: Demo description: Demo documentation: null @@ -222,7 +222,7 @@ about: homepage: null license: LicenseRef-Apache License 2.0 - license_file: package.xml + license_file: null summary: Demo description: Demo documentation: null diff --git a/backends/pixi-build-ros/tests/test_meta_data_provider.py b/backends/pixi-build-ros/tests/test_meta_data_provider.py index 474f3823..735397e3 100644 --- a/backends/pixi-build-ros/tests/test_meta_data_provider.py +++ b/backends/pixi-build-ros/tests/test_meta_data_provider.py @@ -17,7 +17,7 @@ def test_metadata_provider(package_xmls: Path): assert metadata_provider.description() == "Demo" assert metadata_provider.homepage() == "https://test.io/custom_ros" assert metadata_provider.repository() == "https://github.com/test/custom_ros" - assert metadata_provider.license_file() == "custom_ros.xml" + assert metadata_provider.license_file() is None def test_ros_metadata_provider(package_xmls: Path): @@ -30,7 +30,7 @@ def test_ros_metadata_provider(package_xmls: Path): assert metadata_provider.description() == "Demo" assert metadata_provider.homepage() == "https://test.io/custom_ros" assert metadata_provider.repository() == "https://github.com/test/custom_ros" - assert metadata_provider.license_file() == "custom_ros.xml" + assert metadata_provider.license_file() is None def test_metadata_provider_raises_on_broken_xml(package_xmls: Path): From 8f63b9b53fd1f5837632d9d98ba86070498fa74c Mon Sep 17 00:00:00 2001 From: Tim de Jager Date: Mon, 20 Oct 2025 15:54:03 +0200 Subject: [PATCH 5/6] test: verify RustBackendConfig only accepts kebab-case (debug-dir) The RustBackendConfig struct uses #[serde(rename_all = "kebab-case")], which means it already only accepts debug-dir (with hyphen), not debug_dir (with underscore). This test documents and verifies this behavior. --- crates/pixi-build-rust/src/config.rs | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/crates/pixi-build-rust/src/config.rs b/crates/pixi-build-rust/src/config.rs index 157cda01..313b9aff 100644 --- a/crates/pixi-build-rust/src/config.rs +++ b/crates/pixi-build-rust/src/config.rs @@ -95,6 +95,29 @@ mod tests { serde_json::from_value::(json_data).unwrap(); } + #[test] + fn test_debug_dir_only_accepts_kebab_case() { + // The struct uses #[serde(rename_all = "kebab-case")], so it only accepts debug-dir + let json_with_hyphen = json!({ + "debug-dir": "/path/to/debug" + }); + let config = serde_json::from_value::(json_with_hyphen).unwrap(); + assert_eq!(config.debug_dir, Some(PathBuf::from("/path/to/debug"))); + + // Test that debug_dir (underscore) is rejected + let json_with_underscore = json!({ + "debug_dir": "/path/to/debug" + }); + let result = serde_json::from_value::(json_with_underscore); + assert!(result.is_err()); + assert!( + result + .unwrap_err() + .to_string() + .contains("unknown field `debug_dir`") + ); + } + #[test] fn test_merge_with_target_config() { let mut base_env = indexmap::IndexMap::new(); From 122391a4d8d8345fed64f4a11df470f612b55434 Mon Sep 17 00:00:00 2001 From: Tim de Jager Date: Mon, 20 Oct 2025 15:58:07 +0200 Subject: [PATCH 6/6] feat: accept both debug-dir and debug_dir in all backend configs All backend configurations now accept both kebab-case (debug-dir) and snake_case (debug_dir) for the debug_dir field using serde's alias attribute. This provides better flexibility for users who may be familiar with either naming convention. Changes: - Added #[serde(alias = "debug_dir")] to debug_dir fields in: - RustBackendConfig - PythonBackendConfig - CMakeBackendConfig - MojoBackendConfig - RattlerBuildBackendConfig - TestBackendConfig - Updated PyBackendConfig custom deserializer to check for both formats - Updated test to verify both formats are accepted --- .../tests/integration/protocol.rs | 1 + crates/pixi-build-cmake/src/config.rs | 1 + crates/pixi-build-mojo/src/config.rs | 1 + crates/pixi-build-python/src/config.rs | 1 + crates/pixi-build-rattler-build/src/config.rs | 1 + crates/pixi-build-rust/src/config.rs | 17 ++++++----------- py-pixi-build-backend/src/types/config.rs | 5 +++-- 7 files changed, 14 insertions(+), 13 deletions(-) diff --git a/crates/pixi-build-backend/tests/integration/protocol.rs b/crates/pixi-build-backend/tests/integration/protocol.rs index 248c5975..86c6f9ae 100644 --- a/crates/pixi-build-backend/tests/integration/protocol.rs +++ b/crates/pixi-build-backend/tests/integration/protocol.rs @@ -26,6 +26,7 @@ mod imp { #[serde(rename_all = "kebab-case")] pub struct TestBackendConfig { /// If set, internal state will be logged as files in that directory + #[serde(alias = "debug_dir")] pub debug_dir: Option, } diff --git a/crates/pixi-build-cmake/src/config.rs b/crates/pixi-build-cmake/src/config.rs index 75eefc53..476bfe10 100644 --- a/crates/pixi-build-cmake/src/config.rs +++ b/crates/pixi-build-cmake/src/config.rs @@ -14,6 +14,7 @@ pub struct CMakeBackendConfig { #[serde(default)] pub env: IndexMap, /// If set, internal state will be logged as files in that directory + #[serde(alias = "debug_dir")] pub debug_dir: Option, /// Extra input globs to include in addition to the default ones #[serde(default)] diff --git a/crates/pixi-build-mojo/src/config.rs b/crates/pixi-build-mojo/src/config.rs index eaca8963..458c0aec 100644 --- a/crates/pixi-build-mojo/src/config.rs +++ b/crates/pixi-build-mojo/src/config.rs @@ -17,6 +17,7 @@ pub struct MojoBackendConfig { pub env: IndexMap, /// Dir that can be specified for outputting pixi debug state. + #[serde(alias = "debug_dir")] pub debug_dir: Option, /// Extra input globs to include in addition to the default ones. diff --git a/crates/pixi-build-python/src/config.rs b/crates/pixi-build-python/src/config.rs index bf6d9263..84b1a11b 100644 --- a/crates/pixi-build-python/src/config.rs +++ b/crates/pixi-build-python/src/config.rs @@ -17,6 +17,7 @@ pub struct PythonBackendConfig { #[serde(default)] pub env: IndexMap, /// If set, internal state will be logged as files in that directory + #[serde(alias = "debug_dir")] pub debug_dir: Option, /// Extra input globs to include in addition to the default ones #[serde(default)] diff --git a/crates/pixi-build-rattler-build/src/config.rs b/crates/pixi-build-rattler-build/src/config.rs index e61f2e7e..ffde090a 100644 --- a/crates/pixi-build-rattler-build/src/config.rs +++ b/crates/pixi-build-rattler-build/src/config.rs @@ -6,6 +6,7 @@ use std::path::{Path, PathBuf}; #[serde(rename_all = "kebab-case", deny_unknown_fields)] pub struct RattlerBuildBackendConfig { /// If set, internal state will be logged as files in that directory + #[serde(alias = "debug_dir")] pub debug_dir: Option, /// Extra input globs to include in addition to the default ones #[serde(default)] diff --git a/crates/pixi-build-rust/src/config.rs b/crates/pixi-build-rust/src/config.rs index 313b9aff..ef370fb2 100644 --- a/crates/pixi-build-rust/src/config.rs +++ b/crates/pixi-build-rust/src/config.rs @@ -14,6 +14,7 @@ pub struct RustBackendConfig { #[serde(default)] pub env: IndexMap, /// If set, internal state will be logged as files in that directory + #[serde(alias = "debug_dir")] pub debug_dir: Option, /// Extra input globs to include in addition to the default ones #[serde(default)] @@ -96,26 +97,20 @@ mod tests { } #[test] - fn test_debug_dir_only_accepts_kebab_case() { - // The struct uses #[serde(rename_all = "kebab-case")], so it only accepts debug-dir + fn test_debug_dir_accepts_both_formats() { + // Test with debug-dir (kebab-case) - the canonical format let json_with_hyphen = json!({ "debug-dir": "/path/to/debug" }); let config = serde_json::from_value::(json_with_hyphen).unwrap(); assert_eq!(config.debug_dir, Some(PathBuf::from("/path/to/debug"))); - // Test that debug_dir (underscore) is rejected + // Test with debug_dir (underscore) - should also work due to alias let json_with_underscore = json!({ "debug_dir": "/path/to/debug" }); - let result = serde_json::from_value::(json_with_underscore); - assert!(result.is_err()); - assert!( - result - .unwrap_err() - .to_string() - .contains("unknown field `debug_dir`") - ); + let config = serde_json::from_value::(json_with_underscore).unwrap(); + assert_eq!(config.debug_dir, Some(PathBuf::from("/path/to/debug"))); } #[test] diff --git a/py-pixi-build-backend/src/types/config.rs b/py-pixi-build-backend/src/types/config.rs index 63195d89..2cc0d956 100644 --- a/py-pixi-build-backend/src/types/config.rs +++ b/py-pixi-build-backend/src/types/config.rs @@ -1,7 +1,7 @@ use std::path::{Path, PathBuf}; use pixi_build_backend::generated_recipe::BackendConfig; -use pyo3::{Py, PyAny, Python, pyclass, pymethods}; +use pyo3::{pyclass, pymethods, Py, PyAny, Python}; use pythonize::pythonize; use serde::Deserialize; use serde::Deserializer; @@ -26,9 +26,10 @@ impl<'de> Deserialize<'de> for PyBackendConfig { Python::attach(|py| { let model = pythonize(py, &data).map_err(serde::de::Error::custom)?; + // Support both debug_dir and debug-dir let debug_dir: Option = data .as_object_mut() - .and_then(|obj| obj.get("debug_dir")) + .and_then(|obj| obj.get("debug-dir").or_else(|| obj.get("debug_dir"))) .and_then(|v| v.as_str().map(PathBuf::from)); Ok(PyBackendConfig {