Skip to content

Commit 95e681d

Browse files
baszalmstraclaudeHofer-Julian
authored
fix: evaluate conditionals properly (#450)
* fix: properly handle target-specific dependencies in backends The backends were incorrectly trying to evaluate rattler-build selector expressions (like `host_platform == 'linux-64'`) using simple string comparison against platform strings (like `"linux-64"`), which always failed. This meant target-specific dependencies were never properly resolved. Changes: - Use ProjectModel::dependencies() with platform parameter to get properly filtered dependencies instead of manually resolving selectors - Add Contains trait and implementations for checking package names in dependency maps - Add Dependencies::build_and_host_names() helper method for getting package names from build and host dependencies - Update all backends (python, rust, cmake, mojo) to use the new approach - Remove unused ConditionalRequirements::resolve() calls - Clean up unused imports This fixes the issue reported in prefix-dev/pixi#4802 where target-specific dependencies were not being injected into the generated recipes. * refactor: simplify compiler requirements function to accept Dependencies directly - Replace generic add_compilers_to_requirements_by_name with add_compilers_to_requirements_with_dependencies - Pass Dependencies struct directly instead of using Contains trait - Fix Python check in cmake backend to only check host dependencies - Add unit tests for target-specific dependency handling - Tests verify that ProjectModel.dependencies() correctly filters deps by platform * test: improve target-specific dependency tests to verify actual conditions - Check that conditional items contain the correct package name (openssl/gcc) - Verify the condition matches the expected platform selector - Tests now properly validate that linux-64 conditions include linux-64 deps - Tests now properly validate that unix conditions include unix deps * refactor: remove unused Contains trait and simplify dependency checks - Remove unused add_compilers_to_requirements and add_compilers_and_stdlib_to_requirements - Remove Contains trait and all implementations - Remove Contains imports from all backend files - Use contains_key with SourcePackageName for dependency checks - Fix has_openssl to only check host dependencies - Fix conditional tests to match exact condition strings - Remove unused boltons dependencies from test fixtures * fix: use host_platform for all target-specific dependency conditionals - Change build, host, and run dependencies to all use host_platform - Update tests to expect host_platform in conditions - This ensures that target selectors match the actual target platform * refactor: rename add_compilers_to_requirements_with_dependencies to add_compilers_to_requirements Since we removed the old add_compilers_to_requirements function, we can now use the simpler name for the Dependencies-based version. * fixes * fix formatting and clippy * Small improvements --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Julian Hofer <julianhofer@gnome.org>
1 parent 3adcfa3 commit 95e681d

File tree

8 files changed

+289
-109
lines changed

8 files changed

+289
-109
lines changed

crates/pixi-build-backend/src/compilers.rs

Lines changed: 7 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,9 @@
33
44
use std::{collections::HashSet, fmt::Display, ops::Deref};
55

6-
use indexmap::IndexMap;
76
use itertools::Itertools;
87
use rattler_build::NormalizedKey;
9-
use rattler_conda_types::{PackageName, Platform};
8+
use rattler_conda_types::Platform;
109
use recipe_stage0::{
1110
matchspec::PackageDependency,
1211
recipe::{Item, Value},
@@ -82,40 +81,21 @@ pub fn compiler_requirement(language: &Language) -> Item<PackageDependency> {
8281
/// # Arguments
8382
/// * `compilers` - List of compiler names (e.g., ["c", "cxx", "rust", "cuda"])
8483
/// * `requirements` - Mutable reference to the requirements to modify
85-
/// * `resolved_build_requirements` - IndexMap of already resolved build
86-
/// requirements
84+
/// * `dependencies` - The Dependencies struct containing build/host/run dependencies
8785
/// * `host_platform` - The target platform for determining default compiler
8886
/// names
89-
/// * `variants` - The variants available in the recipe, used to determine if
90-
/// stdlib is needed
91-
pub fn add_compilers_and_stdlib_to_requirements(
87+
pub fn add_compilers_to_requirements<S>(
9288
compilers: &[String],
9389
requirements: &mut Vec<Item<PackageDependency>>,
94-
resolved_build_requirements: &IndexMap<PackageName, PackageDependency>,
95-
host_platform: &Platform,
96-
variants: &HashSet<NormalizedKey>,
97-
) {
98-
add_compilers_to_requirements(
99-
compilers,
100-
requirements,
101-
resolved_build_requirements,
102-
host_platform,
103-
);
104-
add_stdlib_to_requirements(compilers, requirements, variants);
105-
}
106-
107-
pub fn add_compilers_to_requirements(
108-
compilers: &[String],
109-
requirements: &mut Vec<Item<PackageDependency>>,
110-
resolved_build_requirements: &IndexMap<PackageName, PackageDependency>,
90+
dependencies: &crate::traits::targets::Dependencies<S>,
11191
host_platform: &Platform,
11292
) {
11393
for compiler_str in compilers {
114-
// Check if the specific compiler is already present
94+
// Check if the specific compiler is already present in build dependencies
11595
let language_compiler = default_compiler(host_platform, compiler_str);
96+
let source_package_name = pixi_build_types::SourcePackageName::from(language_compiler);
11697

117-
if !resolved_build_requirements.contains_key(&PackageName::new_unchecked(language_compiler))
118-
{
98+
if !dependencies.build.contains_key(&source_package_name) {
11999
let template = format!("${{{{ compiler('{compiler_str}') }}}}");
120100
requirements.push(Item::Value(Value::Template(template)));
121101
}

crates/pixi-build-backend/src/specs_conversion.rs

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -105,15 +105,16 @@ pub fn from_targets_v1_to_conditional_requirements(targets: &TargetsV1) -> Condi
105105
for (selector, target) in specific_targets {
106106
let package_requirements = target_to_package_spec(target);
107107

108-
// add the binary requirements
108+
// Add the binary requirements
109+
// Use host_platform for all dependency types to match the actual target platform
109110
build_items.extend(
110111
package_requirements
111112
.build
112113
.into_iter()
113114
.map(|spec| spec.1)
114115
.map(|spec| {
115116
Conditional {
116-
condition: to_rattler_build_selector(selector, PlatformKind::Build),
117+
condition: to_rattler_build_selector(selector, PlatformKind::Host),
117118
then: ListOrItem(vec![spec]),
118119
else_value: ListOrItem::default(),
119120
}
@@ -141,7 +142,7 @@ pub fn from_targets_v1_to_conditional_requirements(targets: &TargetsV1) -> Condi
141142
.map(|spec| spec.1)
142143
.map(|spec| {
143144
Conditional {
144-
condition: to_rattler_build_selector(selector, PlatformKind::Target),
145+
condition: to_rattler_build_selector(selector, PlatformKind::Host),
145146
then: ListOrItem(vec![spec]),
146147
else_value: ListOrItem::default(),
147148
}

crates/pixi-build-backend/src/traits/targets.rs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,16 @@ impl<'a, S> Dependencies<'a, S> {
5959
pub fn contains(&self, name: &SourcePackageName) -> bool {
6060
self.run.contains_key(name) || self.host.contains_key(name) || self.build.contains_key(name)
6161
}
62+
63+
/// Return an iterator of all package names from build and host dependencies.
64+
/// This is useful for checking build tools and compilers.
65+
pub fn build_and_host_names(&self) -> impl Iterator<Item = &str> {
66+
self.build
67+
.keys()
68+
.chain(self.host.keys())
69+
.map(|name| name.as_ref() as &str)
70+
.unique()
71+
}
6272
}
6373

6474
/// A trait that represent a project target.

crates/pixi-build-cmake/src/main.rs

Lines changed: 20 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,14 @@ use build_script::{BuildPlatform, BuildScriptContext};
55
use config::CMakeBackendConfig;
66
use miette::IntoDiagnostic;
77
use pixi_build_backend::{
8-
compilers::add_compilers_and_stdlib_to_requirements,
98
generated_recipe::{DefaultMetadataProvider, GenerateRecipe, GeneratedRecipe, PythonParams},
109
intermediate_backend::IntermediateBackendInstantiator,
10+
traits::ProjectModel,
1111
};
12-
use pixi_build_types::ProjectModelV1;
12+
use pixi_build_types::{ProjectModelV1, SourcePackageName};
1313
use rattler_build::{NormalizedKey, recipe::variable::Variable};
14-
use rattler_conda_types::{ChannelUrl, PackageName, Platform};
15-
use recipe_stage0::recipe::{ConditionalRequirements, Script};
14+
use rattler_conda_types::{ChannelUrl, Platform};
15+
use recipe_stage0::recipe::Script;
1616
use std::collections::HashSet;
1717
use std::path::PathBuf;
1818
use std::{
@@ -61,13 +61,11 @@ impl GenerateRecipe for CMakeGenerator {
6161

6262
let requirements = &mut generated_recipe.recipe.requirements;
6363

64-
let resolved_requirements = ConditionalRequirements::resolve(
65-
requirements.build.as_ref(),
66-
requirements.host.as_ref(),
67-
requirements.run.as_ref(),
68-
requirements.run_constraints.as_ref(),
69-
Some(host_platform),
70-
);
64+
// Get the platform-specific dependencies from the project model.
65+
// This properly handles target selectors like [target.linux-64] by using
66+
// the ProjectModel trait's platform-aware API instead of trying to evaluate
67+
// rattler-build selectors with simple string comparison.
68+
let model_dependencies = model.dependencies(Some(host_platform));
7169

7270
// Get the list of compilers from config, defaulting to ["cxx"] if not specified
7371
let compilers = config
@@ -76,26 +74,32 @@ impl GenerateRecipe for CMakeGenerator {
7674
.unwrap_or_else(|| vec!["cxx".to_string()]);
7775

7876
// Add configured compilers to build requirements
79-
add_compilers_and_stdlib_to_requirements(
77+
pixi_build_backend::compilers::add_compilers_to_requirements(
8078
&compilers,
8179
&mut requirements.build,
82-
&resolved_requirements.build,
80+
&model_dependencies,
8381
&host_platform,
82+
);
83+
pixi_build_backend::compilers::add_stdlib_to_requirements(
84+
&compilers,
85+
&mut requirements.build,
8486
variants,
8587
);
8688

8789
// add necessary build tools
8890
for tool in ["cmake", "ninja"] {
89-
let tool_name = PackageName::new_unchecked(tool);
90-
if !resolved_requirements.build.contains_key(&tool_name) {
91+
let tool_name = SourcePackageName::from(tool);
92+
if !model_dependencies.build.contains_key(&tool_name) {
9193
requirements.build.push(tool.parse().into_diagnostic()?);
9294
}
9395
}
9496

9597
// Check if the host platform has a host python dependency
9698
// This is used to determine if we need to the cmake argument for the python
9799
// executable
98-
let has_host_python = resolved_requirements.contains(&PackageName::new_unchecked("python"));
100+
let has_host_python = model_dependencies
101+
.host
102+
.contains_key(&SourcePackageName::from("python"));
99103

100104
let build_script = BuildScriptContext {
101105
build_platform: if Platform::current().is_windows() {

crates/pixi-build-mojo/src/main.rs

Lines changed: 23 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,14 @@ use config::{MojoBackendConfig, clean_project_name};
66
use miette::{Error, IntoDiagnostic};
77
use pixi_build_backend::generated_recipe::DefaultMetadataProvider;
88
use pixi_build_backend::{
9-
compilers::add_compilers_and_stdlib_to_requirements,
109
generated_recipe::{GenerateRecipe, GeneratedRecipe, PythonParams},
1110
intermediate_backend::IntermediateBackendInstantiator,
11+
traits::ProjectModel,
1212
};
1313
use pixi_build_types::ProjectModelV1;
1414
use rattler_build::NormalizedKey;
15-
use rattler_conda_types::{ChannelUrl, PackageName, Platform};
16-
use recipe_stage0::recipe::{ConditionalRequirements, Script};
15+
use rattler_conda_types::{ChannelUrl, Platform};
16+
use recipe_stage0::recipe::Script;
1717
use std::collections::HashSet;
1818
use std::path::PathBuf;
1919
use std::{collections::BTreeSet, path::Path, sync::Arc};
@@ -68,13 +68,12 @@ impl GenerateRecipe for MojoGenerator {
6868

6969
// Add compiler
7070
let requirements = &mut generated_recipe.recipe.requirements;
71-
let resolved_requirements = ConditionalRequirements::resolve(
72-
requirements.build.as_ref(),
73-
requirements.host.as_ref(),
74-
requirements.run.as_ref(),
75-
requirements.run_constraints.as_ref(),
76-
Some(host_platform),
77-
);
71+
72+
// Get the platform-specific dependencies from the project model.
73+
// This properly handles target selectors like [target.linux-64] by using
74+
// the ProjectModel trait's platform-aware API instead of trying to evaluate
75+
// rattler-build selectors with simple string comparison.
76+
let model_dependencies = model.dependencies(Some(host_platform));
7877

7978
// Get the list of compilers from config, defaulting to ["mojo"] if not specified
8079
let mut compilers = config
@@ -84,17 +83,18 @@ impl GenerateRecipe for MojoGenerator {
8483

8584
// Handle mojo compiler specially if it's in the list
8685
if let Some(idx) = compilers.iter().position(|name| name == "mojo") {
87-
let mojo_compiler_pkg = "mojo-compiler".to_string();
86+
let mojo_compiler_pkg = "mojo-compiler";
8887
// All of these packages also contain the mojo compiler and maintain backward compat.
8988
// They should be removable at a future point.
9089
let alt_names = ["max", "mojo", "modular"];
9190

92-
if !resolved_requirements
93-
.build
94-
.contains_key(&PackageName::new_unchecked(&mojo_compiler_pkg))
95-
&& !alt_names
96-
.iter()
97-
.any(|alt| resolved_requirements.build.contains_key(*alt))
91+
let mojo_pkg_name = pixi_build_types::SourcePackageName::from(mojo_compiler_pkg);
92+
if !model_dependencies.build.contains_key(&mojo_pkg_name)
93+
&& !alt_names.iter().any(|alt| {
94+
model_dependencies
95+
.build
96+
.contains_key(&pixi_build_types::SourcePackageName::from(*alt))
97+
})
9898
{
9999
requirements
100100
.build
@@ -105,11 +105,15 @@ impl GenerateRecipe for MojoGenerator {
105105
compilers.swap_remove(idx);
106106
}
107107

108-
add_compilers_and_stdlib_to_requirements(
108+
pixi_build_backend::compilers::add_compilers_to_requirements(
109109
&compilers,
110110
&mut requirements.build,
111-
&resolved_requirements.build,
111+
&model_dependencies,
112112
&host_platform,
113+
);
114+
pixi_build_backend::compilers::add_stdlib_to_requirements(
115+
&compilers,
116+
&mut requirements.build,
113117
variants,
114118
);
115119

crates/pixi-build-python/src/build_script.rs

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
use std::path::PathBuf;
22

33
use minijinja::Environment;
4-
use rattler_conda_types::PackageName;
5-
use recipe_stage0::{matchspec::PackageDependency, requirements::PackageSpecDependencies};
64
use serde::Serialize;
75

86
const UV: &str = "uv";
@@ -31,12 +29,15 @@ impl Installer {
3129
}
3230
}
3331

34-
pub fn determine_installer(
35-
dependencies: &PackageSpecDependencies<PackageDependency>,
32+
/// Determine the installer from an iterator of dependency package names.
33+
/// Checks if "uv" is present in the package names.
34+
pub fn determine_installer_from_names<'a>(
35+
mut package_names: impl Iterator<Item = &'a str>,
3636
) -> Installer {
37-
// Determine the installer to use
38-
let uv = PackageName::new_unchecked(UV.to_string());
39-
if dependencies.contains(&uv) {
37+
// Check all dependency names for "uv" package
38+
let has_uv = package_names.any(|name| name == UV);
39+
40+
if has_uv {
4041
Installer::Uv
4142
} else {
4243
Installer::Pip

crates/pixi-build-python/src/main.rs

Lines changed: 22 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -7,15 +7,15 @@ use config::PythonBackendConfig;
77
use miette::IntoDiagnostic;
88
use pixi_build_backend::variants::NormalizedKey;
99
use pixi_build_backend::{
10-
compilers::add_compilers_and_stdlib_to_requirements,
1110
generated_recipe::{GenerateRecipe, GeneratedRecipe, PythonParams},
1211
intermediate_backend::IntermediateBackendInstantiator,
12+
traits::ProjectModel,
1313
};
1414
use pixi_build_types::ProjectModelV1;
1515
use pyproject_toml::PyProjectToml;
16-
use rattler_conda_types::{ChannelUrl, PackageName, Platform, package::EntryPoint};
16+
use rattler_conda_types::{ChannelUrl, Platform, package::EntryPoint};
1717
use recipe_stage0::matchspec::PackageDependency;
18-
use recipe_stage0::recipe::{self, ConditionalRequirements, NoArchKind, Python, Script};
18+
use recipe_stage0::recipe::{self, NoArchKind, Python, Script};
1919
use std::collections::HashSet;
2020
use std::{
2121
collections::BTreeSet,
@@ -93,27 +93,25 @@ impl GenerateRecipe for PythonGenerator {
9393

9494
let requirements = &mut generated_recipe.recipe.requirements;
9595

96-
let resolved_requirements = ConditionalRequirements::resolve(
97-
requirements.build.as_ref(),
98-
requirements.host.as_ref(),
99-
requirements.run.as_ref(),
100-
requirements.run_constraints.as_ref(),
101-
Some(host_platform),
102-
);
96+
// Get the platform-specific dependencies from the project model.
97+
// This properly handles target selectors like [target.linux-64] by using
98+
// the ProjectModel trait's platform-aware API instead of trying to evaluate
99+
// rattler-build selectors with simple string comparison.
100+
let model_dependencies = model.dependencies(Some(host_platform));
103101

104102
// Ensure the python build tools are added to the `host` requirements.
105103
// Please note: this is a subtle difference for python, where the build tools
106104
// are added to the `host` requirements, while for cmake/rust they are
107105
// added to the `build` requirements.
108-
let installer = Installer::determine_installer(&resolved_requirements);
106+
// We only check build and host dependencies for the installer.
107+
let installer =
108+
Installer::determine_installer_from_names(model_dependencies.build_and_host_names());
109109

110110
let installer_name = installer.package_name().to_string();
111+
let installer_pkg = pixi_build_types::SourcePackageName::from(installer_name.as_str());
111112

112113
// add installer in the host requirements
113-
if !resolved_requirements
114-
.host
115-
.contains_key(&PackageName::new_unchecked(&installer_name))
116-
{
114+
if !model_dependencies.host.contains_key(&installer_pkg) {
117115
requirements
118116
.host
119117
.push(installer_name.parse().into_diagnostic()?);
@@ -128,28 +126,27 @@ impl GenerateRecipe for PythonGenerator {
128126
python_requirement_str.parse().into_diagnostic()
129127
};
130128

129+
let python_pkg = pixi_build_types::SourcePackageName::from("python");
131130
// add python in both host and run requirements
132-
if !resolved_requirements
133-
.host
134-
.contains_key(&PackageName::new_unchecked("python"))
135-
{
131+
if !model_dependencies.host.contains_key(&python_pkg) {
136132
requirements.host.push(get_python_requirement()?);
137133
}
138-
if !resolved_requirements
139-
.run
140-
.contains_key(&PackageName::new_unchecked("python"))
141-
{
134+
if !model_dependencies.run.contains_key(&python_pkg) {
142135
requirements.run.push(get_python_requirement()?);
143136
}
144137

145138
// Get the list of compilers from config, defaulting to no compilers for pure
146139
// Python packages and add them to the build requirements.
147140
let compilers = config.compilers.clone().unwrap_or_default();
148-
add_compilers_and_stdlib_to_requirements(
141+
pixi_build_backend::compilers::add_compilers_to_requirements(
149142
&compilers,
150143
&mut requirements.build,
151-
&resolved_requirements.build,
144+
&model_dependencies,
152145
&host_platform,
146+
);
147+
pixi_build_backend::compilers::add_stdlib_to_requirements(
148+
&compilers,
149+
&mut requirements.build,
153150
variants,
154151
);
155152

0 commit comments

Comments
 (0)