From c82cb42973d4de97639bb054b261472b94d7bb4e Mon Sep 17 00:00:00 2001 From: Ruben Arts Date: Wed, 13 Nov 2024 09:03:22 +0100 Subject: [PATCH 01/13] test: add failing test to verify the upgraded dependency location --- tests/integration_python/test_main_cli.py | 43 +++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/tests/integration_python/test_main_cli.py b/tests/integration_python/test_main_cli.py index 2fa34d9786..76094aa7d2 100644 --- a/tests/integration_python/test_main_cli.py +++ b/tests/integration_python/test_main_cli.py @@ -505,3 +505,46 @@ def test_upgrade_pypi_and_conda_package(pixi: Path, tmp_path: Path) -> None: assert "1.*" not in numpy_pypi numpy_conda = parsed_manifest["tool"]["pixi"]["dependencies"]["numpy"] assert numpy_conda != "1.*" + + +@pytest.mark.slow +def test_upgrade_dependency_location(pixi: Path, tmp_path: Path) -> None: + # Test based on https://github.com/prefix-dev/pixi/issues/2470 + # Making sure pixi places the upgraded package in the correct location + manifest_path = tmp_path / "pyproject.toml" + pyproject = """ + [project] + dependencies = [] + name = "test-upgrade" + requires-python = ">= 3.11" + + [tool.pixi.project] + channels = ["conda-forge"] + platforms = ["linux-64"] + + [tool.pixi.pypi-dependencies] + polars = "==0.*" + """ + + manifest_path.write_text(pyproject) + + # Upgrade numpy, both conda and pypi should be upgraded + verify_cli_command( + [pixi, "upgrade", "--manifest-path", manifest_path], + stderr_contains=["polars"], + ) + parsed_manifest = tomllib.loads(manifest_path.read_text()) + # Check that the requires-python is modified + requires_python = parsed_manifest["project"]["requires-python"] + assert "3.11" not in requires_python + + # Check that `tool.pixi.dependencies.python` doesn't exist + assert "python" not in parsed_manifest.get("tool", {}).get("pixi", {}).get("dependencies", {}) + + # Check that the pypi-dependency is upgraded + polars_pypi = parsed_manifest["tool"]["pixi"]["pypi-dependencies"]["polars"] + assert polars_pypi + assert polars_pypi != "==0.*" + + # Check that the pypi-dependency doesn't exist in the project dependencies + assert "polars" not in parsed_manifest["project"]["dependencies"] From a1d1b3a22a9ae79139a0da70da3d2a332ee27cb9 Mon Sep 17 00:00:00 2001 From: Julian Hofer Date: Thu, 28 Nov 2024 15:33:14 +0100 Subject: [PATCH 02/13] Make match a bit more robust --- crates/pixi_manifest/src/target.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/pixi_manifest/src/target.rs b/crates/pixi_manifest/src/target.rs index 1395239012..f8f38697cd 100644 --- a/crates/pixi_manifest/src/target.rs +++ b/crates/pixi_manifest/src/target.rs @@ -237,16 +237,16 @@ impl WorkspaceTarget { ) -> Result { if self.has_pypi_dependency(requirement, false) { match dependency_overwrite_behavior { - DependencyOverwriteBehavior::OverwriteIfExplicit - if requirement.version_or_url.is_none() => - { - return Ok(false) + DependencyOverwriteBehavior::OverwriteIfExplicit => { + if requirement.version_or_url.is_none() { + return Ok(false); + } } DependencyOverwriteBehavior::IgnoreDuplicate => return Ok(false), DependencyOverwriteBehavior::Error => { return Err(DependencyError::Duplicate(requirement.name.to_string())); } - _ => {} + DependencyOverwriteBehavior::Overwrite => {} } } From ff6dc4b32cf9f9abe707e5cd27fc6e24567579a5 Mon Sep 17 00:00:00 2001 From: Julian Hofer Date: Thu, 28 Nov 2024 17:30:36 +0100 Subject: [PATCH 03/13] Add method `pypi_dependency_location` --- crates/pixi_manifest/src/lib.rs | 10 ++- crates/pixi_manifest/src/manifests/source.rs | 95 ++++++++++++++++---- crates/pixi_manifest/src/toml/document.rs | 40 ++++++++- src/global/project/manifest.rs | 2 +- 4 files changed, 124 insertions(+), 23 deletions(-) diff --git a/crates/pixi_manifest/src/lib.rs b/crates/pixi_manifest/src/lib.rs index c178b5376f..66dc1e392f 100644 --- a/crates/pixi_manifest/src/lib.rs +++ b/crates/pixi_manifest/src/lib.rs @@ -76,11 +76,13 @@ pub enum DependencyOverwriteBehavior { } pub enum PypiDependencyLocation { - // The [pypi-dependencies] or [tool.pixi.pypi-dependencies] table - Pixi, - // The [project.optional-dependencies] table in a 'pyproject.toml' manifest + /// [pypi-dependencies] in pixi.toml or [tool.pixi.pypi-dependencies] in pyproject.toml + PixiPypiDependencies, + /// [project.dependencies] in pyproject.toml + Dependencies, + /// [project.optional-dependencies] table in pyproject.toml OptionalDependencies, - // The [dependency-groups] table in a 'pyproject.toml' manifest + /// [dependency-groups] in pyproject.toml DependencyGroups, } diff --git a/crates/pixi_manifest/src/manifests/source.rs b/crates/pixi_manifest/src/manifests/source.rs index 123ffe12a5..c5bb1733c6 100644 --- a/crates/pixi_manifest/src/manifests/source.rs +++ b/crates/pixi_manifest/src/manifests/source.rs @@ -56,7 +56,14 @@ impl ManifestSource { } } - fn manifest(&mut self) -> &mut TomlDocument { + fn mut_manifest(&mut self) -> &mut TomlDocument { + match self { + ManifestSource::PyProjectToml(document) => document, + ManifestSource::PixiToml(document) => document, + } + } + + fn manifest(&self) -> &TomlDocument { match self { ManifestSource::PyProjectToml(document) => document, ManifestSource::PixiToml(document) => document, @@ -80,8 +87,8 @@ impl ManifestSource { .with_feature_name(Some(feature_name)) .with_table(table); - self.manifest() - .get_or_insert_toml_array(table_name.to_string().as_str(), array_name) + self.mut_manifest() + .get_or_insert_mut_toml_array(table_name.to_string().as_str(), array_name) } fn as_table_mut(&mut self) -> &mut Table { @@ -105,7 +112,9 @@ impl ManifestSource { // arrays let remove_requirement = |source: &mut ManifestSource, table, array_name| -> Result<(), TomlError> { - let array = source.manifest().get_toml_array(table, array_name)?; + let array = source + .mut_manifest() + .get_mut_toml_array(table, array_name)?; if let Some(array) = array { array.retain(|x| { let req: pep508_rs::Requirement = x @@ -118,7 +127,7 @@ impl ManifestSource { }); if array.is_empty() { source - .manifest() + .mut_manifest() .get_or_insert_nested_table(table)? .remove(array_name); } @@ -146,7 +155,7 @@ impl ManifestSource { .with_platform(platform.as_ref()) .with_table(Some(consts::PYPI_DEPENDENCIES)); - self.manifest() + self.mut_manifest() .get_or_insert_nested_table(table_name.to_string().as_str()) .map(|t| t.remove(dep.as_source()))?; Ok(()) @@ -169,7 +178,7 @@ impl ManifestSource { .with_platform(platform.as_ref()) .with_table(Some(spec_type.name())); - self.manifest() + self.mut_manifest() .get_or_insert_nested_table(table_name.to_string().as_str()) .map(|t| t.remove(dep.as_source()))?; Ok(()) @@ -195,7 +204,7 @@ impl ManifestSource { .with_feature_name(Some(feature_name)) .with_table(Some(spec_type.name())); - self.manifest() + self.mut_manifest() .get_or_insert_nested_table(dependency_table.to_string().as_str()) .map(|t| t.insert(name.as_normalized(), Item::Value(spec.to_toml_value())))?; @@ -234,7 +243,7 @@ impl ManifestSource { // - When a specific platform is requested, as markers are not supported (https://github.com/prefix-dev/pixi/issues/2149) // - When an editable install is requested if matches!(self, ManifestSource::PixiToml(_)) - || matches!(location, Some(PypiDependencyLocation::Pixi)) + || matches!(location, Some(PypiDependencyLocation::PixiPypiDependencies)) || platform.is_some() || editable.is_some_and(|e| e) { @@ -250,7 +259,7 @@ impl ManifestSource { .with_feature_name(Some(feature_name)) .with_table(Some(consts::PYPI_DEPENDENCIES)); - self.manifest() + self.mut_manifest() .get_or_insert_nested_table(dependency_table.to_string().as_str())? .insert( requirement.name.as_ref(), @@ -266,8 +275,8 @@ impl ManifestSource { let add_requirement = |source: &mut ManifestSource, table, array| -> Result<(), TomlError> { source - .manifest() - .get_or_insert_toml_array(table, array)? + .mut_manifest() + .get_or_insert_mut_toml_array(table, array)? .push(requirement.to_string()); Ok(()) }; @@ -285,6 +294,60 @@ impl ManifestSource { Ok(()) } + pub fn pypi_dependency_location( + &self, + dep: &PyPiPackageName, + platform: Option, + feature_name: &FeatureName, + ) -> PypiDependencyLocation { + // For both 'pyproject.toml' and 'pixi.toml' manifest, + // try and to get `pypi-dependency` + let table_name = TableName::new() + .with_prefix(self.table_prefix()) + .with_feature_name(Some(feature_name)) + .with_platform(platform.as_ref()) + .with_table(Some(consts::PYPI_DEPENDENCIES)); + + let pypi_dependency_table = self + .manifest() + .get_nested_table(table_name.to_string().as_str()) + .ok(); + + if pypi_dependency_table + .and_then(|table| table.get(dep.as_source())) + .is_some() + { + return PypiDependencyLocation::PixiPypiDependencies; + } + + if self + .manifest() + .get_toml_array("project", "dependencies") + .is_ok() + { + return PypiDependencyLocation::Dependencies; + } + let name = feature_name.to_string(); + + if self + .manifest() + .get_toml_array("project.optional-dependencies", &name) + .is_ok() + { + return PypiDependencyLocation::OptionalDependencies; + } + + if self + .manifest() + .get_toml_array("dependency-groups", &name) + .is_ok() + { + return PypiDependencyLocation::DependencyGroups; + } + + todo!() + } + /// Removes a task from the TOML manifest pub fn remove_task( &mut self, @@ -301,7 +364,7 @@ impl ManifestSource { .with_feature_name(Some(feature_name)) .with_table(Some("tasks")); - self.manifest() + self.mut_manifest() .get_or_insert_nested_table(task_table.to_string().as_str())? .remove(name); @@ -323,7 +386,7 @@ impl ManifestSource { .with_feature_name(Some(feature_name)) .with_table(Some("tasks")); - self.manifest() + self.mut_manifest() .get_or_insert_nested_table(task_table.to_string().as_str())? .insert(name, task.into()); @@ -363,7 +426,7 @@ impl ManifestSource { .with_table(Some("environments")); // Get the environment table - self.manifest() + self.mut_manifest() .get_or_insert_nested_table(env_table.to_string().as_str())? .insert(&name.into(), item); @@ -379,7 +442,7 @@ impl ManifestSource { .with_table(Some("environments")); Ok(self - .manifest() + .mut_manifest() .get_or_insert_nested_table(env_table.to_string().as_str())? .remove(name) .is_some()) diff --git a/crates/pixi_manifest/src/toml/document.rs b/crates/pixi_manifest/src/toml/document.rs index 84128969a8..ebcca6d6e3 100644 --- a/crates/pixi_manifest/src/toml/document.rs +++ b/crates/pixi_manifest/src/toml/document.rs @@ -36,6 +36,26 @@ impl TomlDocument { self.0.entry(key).or_insert(item) } + /// Retrieve a reference to a target table `table_name` + /// in dotted form (e.g. `table1.table2`) from the root of the document. + pub fn get_nested_table<'a>( + &'a self, + table_name: &str, + ) -> Result<&'a dyn TableLike, TomlError> { + let parts: Vec<&str> = table_name.split('.').collect(); + + let mut current_table = self.0.as_table() as &dyn TableLike; + + for part in parts { + current_table = current_table + .get(part) + .ok_or_else(|| TomlError::table_error(part, table_name))? + .as_table_like() + .ok_or_else(|| TomlError::table_error(part, table_name))?; + } + Ok(current_table) + } + /// Retrieve a mutable reference to a target table `table_name` /// in dotted form (e.g. `table1.table2`) from the root of the document. /// If the table is not found, it is inserted into the document. @@ -108,7 +128,7 @@ impl TomlDocument { /// in table `table_name` in dotted form (e.g. `table1.table2.array`). /// /// If the array is not found, it is inserted into the document. - pub fn get_or_insert_toml_array<'a>( + pub fn get_or_insert_mut_toml_array<'a>( &'a mut self, table_name: &str, array_name: &str, @@ -124,7 +144,7 @@ impl TomlDocument { /// in table `table_name` in dotted form (e.g. `table1.table2.array`). /// /// If the array is not found, returns None. - pub fn get_toml_array<'a>( + pub fn get_mut_toml_array<'a>( &'a mut self, table_name: &str, array_name: &str, @@ -135,6 +155,22 @@ impl TomlDocument { .and_then(|a| a.as_array_mut()); Ok(array) } + + /// Retrieves a reference to a target array `array_name` + /// in table `table_name` in dotted form (e.g. `table1.table2.array`). + /// + /// If the array is not found, returns None. + pub fn get_toml_array<'a>( + &'a self, + table_name: &str, + array_name: &str, + ) -> Result, TomlError> { + let array = self + .get_nested_table(table_name)? + .get(array_name) + .and_then(|a| a.as_array()); + Ok(array) + } } #[cfg(test)] diff --git a/src/global/project/manifest.rs b/src/global/project/manifest.rs index 53499d686e..fc6a561c54 100644 --- a/src/global/project/manifest.rs +++ b/src/global/project/manifest.rs @@ -92,7 +92,7 @@ impl Manifest { // Update self.document let channels_array = self .document - .get_or_insert_toml_array(&format!("envs.{env_name}"), "channels")?; + .get_or_insert_mut_toml_array(&format!("envs.{env_name}"), "channels")?; for channel in channels { channels_array.push(channel.as_str()); } From 41536d70bc08e20bf598480393e6def58c8d02b9 Mon Sep 17 00:00:00 2001 From: Julian Hofer Date: Fri, 29 Nov 2024 14:45:31 +0100 Subject: [PATCH 04/13] Actually use `pypi_dependency_location` --- crates/pixi_manifest/src/manifests/source.rs | 35 +++++++++++++++----- src/cli/add.rs | 7 ++-- src/cli/upgrade.rs | 9 ++++- src/project/mod.rs | 21 ++++++------ 4 files changed, 50 insertions(+), 22 deletions(-) diff --git a/crates/pixi_manifest/src/manifests/source.rs b/crates/pixi_manifest/src/manifests/source.rs index c5bb1733c6..f36bdc93fe 100644 --- a/crates/pixi_manifest/src/manifests/source.rs +++ b/crates/pixi_manifest/src/manifests/source.rs @@ -294,12 +294,31 @@ impl ManifestSource { Ok(()) } + /// Determines the location of a PyPi dependency within the manifest. + /// + /// This method checks various sections of the manifest to locate the specified + /// PyPi dependency. It searches in the following order: + /// 1. `pypi-dependencies` table in the manifest. + /// 2. `project.dependencies` array in the manifest. + /// 3. `project.optional-dependencies` array in the manifest. + /// 4. `dependency-groups` array in the manifest. + /// + /// # Arguments + /// + /// * `dep` - The name of the PyPi package to locate. + /// * `platform` - An optional platform specification. + /// * `feature_name` - The name of the feature to which the dependency belongs. + /// + /// # Returns + /// + /// An `Option` containing the `PypiDependencyLocation` if the dependency is found, + /// or `None` if it is not found in any of the checked sections. pub fn pypi_dependency_location( &self, - dep: &PyPiPackageName, + package_name: &PyPiPackageName, platform: Option, feature_name: &FeatureName, - ) -> PypiDependencyLocation { + ) -> Option { // For both 'pyproject.toml' and 'pixi.toml' manifest, // try and to get `pypi-dependency` let table_name = TableName::new() @@ -314,10 +333,10 @@ impl ManifestSource { .ok(); if pypi_dependency_table - .and_then(|table| table.get(dep.as_source())) + .and_then(|table| table.get(package_name.as_source())) .is_some() { - return PypiDependencyLocation::PixiPypiDependencies; + return Some(PypiDependencyLocation::PixiPypiDependencies); } if self @@ -325,7 +344,7 @@ impl ManifestSource { .get_toml_array("project", "dependencies") .is_ok() { - return PypiDependencyLocation::Dependencies; + return Some(PypiDependencyLocation::Dependencies); } let name = feature_name.to_string(); @@ -334,7 +353,7 @@ impl ManifestSource { .get_toml_array("project.optional-dependencies", &name) .is_ok() { - return PypiDependencyLocation::OptionalDependencies; + return Some(PypiDependencyLocation::OptionalDependencies); } if self @@ -342,10 +361,10 @@ impl ManifestSource { .get_toml_array("dependency-groups", &name) .is_ok() { - return PypiDependencyLocation::DependencyGroups; + return Some(PypiDependencyLocation::DependencyGroups); } - todo!() + None } /// Removes a task from the TOML manifest diff --git a/src/cli/add.rs b/src/cli/add.rs index 5315700d42..259ec5139d 100644 --- a/src/cli/add.rs +++ b/src/cli/add.rs @@ -109,7 +109,11 @@ pub async fn execute(args: Args) -> miette::Result<()> { } DependencyType::PypiDependency => { let match_specs = IndexMap::default(); - let pypi_deps = dependency_config.pypi_deps(&project)?; + let pypi_deps = dependency_config + .pypi_deps(&project)? + .into_iter() + .map(|(name, req)| (name, (req, None))) + .collect(); (match_specs, pypi_deps) } }; @@ -124,7 +128,6 @@ pub async fn execute(args: Args) -> miette::Result<()> { &args.dependency_config.feature, &args.dependency_config.platforms, args.editable, - &None, dry_run, ) .await?; diff --git a/src/cli/upgrade.rs b/src/cli/upgrade.rs index d4c7f53332..893e6b32c1 100644 --- a/src/cli/upgrade.rs +++ b/src/cli/upgrade.rs @@ -191,6 +191,14 @@ pub async fn execute(args: Args) -> miette::Result<()> { )), _ => None, }) + .map(|(name, req)| { + let location = project.manifest.document.pypi_dependency_location( + &name, + None, + &args.specs.feature, + ); + (name, (req, location)) + }) .collect(); let update_deps = project @@ -201,7 +209,6 @@ pub async fn execute(args: Args) -> miette::Result<()> { &args.specs.feature, &[], false, - &None, args.dry_run, ) .await?; diff --git a/src/project/mod.rs b/src/project/mod.rs index 4804a9514c..3932e0bd86 100644 --- a/src/project/mod.rs +++ b/src/project/mod.rs @@ -661,12 +661,11 @@ impl Project { pub async fn update_dependencies( &mut self, match_specs: IndexMap, - pypi_deps: IndexMap, + pypi_deps: IndexMap)>, prefix_update_config: &PrefixUpdateConfig, feature_name: &FeatureName, platforms: &[Platform], editable: bool, - location: &Option, dry_run: bool, ) -> Result, miette::Error> { let mut conda_specs_to_add_constraints_for = IndexMap::new(); @@ -691,18 +690,18 @@ impl Project { } } - for (name, spec) in pypi_deps { + for (name, (spec, location)) in pypi_deps { let added = self.manifest.add_pep508_dependency( &spec, platforms, feature_name, Some(editable), DependencyOverwriteBehavior::Overwrite, - location, + &location, )?; if added { if spec.version_or_url.is_none() { - pypi_specs_to_add_constraints_for.insert(name.clone(), spec); + pypi_specs_to_add_constraints_for.insert(name.clone(), (spec, location)); } pypi_packages.insert(name.as_normalized().clone()); } @@ -793,7 +792,6 @@ impl Project { feature_name, platforms, editable, - location, )?; implicit_constraints.extend(pypi_constraints); } @@ -931,16 +929,17 @@ impl Project { /// Update the pypi specs of newly added packages based on the contents of /// the updated lock-file. - #[allow(clippy::too_many_arguments)] fn update_pypi_specs_from_lock_file( &mut self, updated_lock_file: &LockFile, - pypi_specs_to_add_constraints_for: IndexMap, + pypi_specs_to_add_constraints_for: IndexMap< + PyPiPackageName, + (Requirement, Option), + >, affect_environment_and_platforms: Vec<(String, Platform)>, feature_name: &FeatureName, platforms: &[Platform], editable: bool, - location: &Option, ) -> miette::Result> { let mut implicit_constraints = HashMap::new(); @@ -962,7 +961,7 @@ impl Project { let pinning_strategy = self.config().pinning_strategy.unwrap_or_default(); // Determine the versions of the packages in the lock-file - for (name, req) in pypi_specs_to_add_constraints_for { + for (name, (req, location)) in pypi_specs_to_add_constraints_for { let version_constraint = pinning_strategy.determine_version_constraint( pypi_records .iter() @@ -991,7 +990,7 @@ impl Project { feature_name, Some(editable), DependencyOverwriteBehavior::Overwrite, - location, + &location, )?; } } From ddf567c6a1b72c81bee585577821680facc846d5 Mon Sep 17 00:00:00 2001 From: Julian Hofer Date: Fri, 29 Nov 2024 14:54:29 +0100 Subject: [PATCH 05/13] Rename to `manifest_mut` --- crates/pixi_manifest/src/manifests/source.rs | 26 ++++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/crates/pixi_manifest/src/manifests/source.rs b/crates/pixi_manifest/src/manifests/source.rs index f36bdc93fe..cfd9dd18c3 100644 --- a/crates/pixi_manifest/src/manifests/source.rs +++ b/crates/pixi_manifest/src/manifests/source.rs @@ -56,7 +56,7 @@ impl ManifestSource { } } - fn mut_manifest(&mut self) -> &mut TomlDocument { + fn manifest_mut(&mut self) -> &mut TomlDocument { match self { ManifestSource::PyProjectToml(document) => document, ManifestSource::PixiToml(document) => document, @@ -87,7 +87,7 @@ impl ManifestSource { .with_feature_name(Some(feature_name)) .with_table(table); - self.mut_manifest() + self.manifest_mut() .get_or_insert_mut_toml_array(table_name.to_string().as_str(), array_name) } @@ -113,7 +113,7 @@ impl ManifestSource { let remove_requirement = |source: &mut ManifestSource, table, array_name| -> Result<(), TomlError> { let array = source - .mut_manifest() + .manifest_mut() .get_mut_toml_array(table, array_name)?; if let Some(array) = array { array.retain(|x| { @@ -127,7 +127,7 @@ impl ManifestSource { }); if array.is_empty() { source - .mut_manifest() + .manifest_mut() .get_or_insert_nested_table(table)? .remove(array_name); } @@ -155,7 +155,7 @@ impl ManifestSource { .with_platform(platform.as_ref()) .with_table(Some(consts::PYPI_DEPENDENCIES)); - self.mut_manifest() + self.manifest_mut() .get_or_insert_nested_table(table_name.to_string().as_str()) .map(|t| t.remove(dep.as_source()))?; Ok(()) @@ -178,7 +178,7 @@ impl ManifestSource { .with_platform(platform.as_ref()) .with_table(Some(spec_type.name())); - self.mut_manifest() + self.manifest_mut() .get_or_insert_nested_table(table_name.to_string().as_str()) .map(|t| t.remove(dep.as_source()))?; Ok(()) @@ -204,7 +204,7 @@ impl ManifestSource { .with_feature_name(Some(feature_name)) .with_table(Some(spec_type.name())); - self.mut_manifest() + self.manifest_mut() .get_or_insert_nested_table(dependency_table.to_string().as_str()) .map(|t| t.insert(name.as_normalized(), Item::Value(spec.to_toml_value())))?; @@ -259,7 +259,7 @@ impl ManifestSource { .with_feature_name(Some(feature_name)) .with_table(Some(consts::PYPI_DEPENDENCIES)); - self.mut_manifest() + self.manifest_mut() .get_or_insert_nested_table(dependency_table.to_string().as_str())? .insert( requirement.name.as_ref(), @@ -275,7 +275,7 @@ impl ManifestSource { let add_requirement = |source: &mut ManifestSource, table, array| -> Result<(), TomlError> { source - .mut_manifest() + .manifest_mut() .get_or_insert_mut_toml_array(table, array)? .push(requirement.to_string()); Ok(()) @@ -383,7 +383,7 @@ impl ManifestSource { .with_feature_name(Some(feature_name)) .with_table(Some("tasks")); - self.mut_manifest() + self.manifest_mut() .get_or_insert_nested_table(task_table.to_string().as_str())? .remove(name); @@ -405,7 +405,7 @@ impl ManifestSource { .with_feature_name(Some(feature_name)) .with_table(Some("tasks")); - self.mut_manifest() + self.manifest_mut() .get_or_insert_nested_table(task_table.to_string().as_str())? .insert(name, task.into()); @@ -445,7 +445,7 @@ impl ManifestSource { .with_table(Some("environments")); // Get the environment table - self.mut_manifest() + self.manifest_mut() .get_or_insert_nested_table(env_table.to_string().as_str())? .insert(&name.into(), item); @@ -461,7 +461,7 @@ impl ManifestSource { .with_table(Some("environments")); Ok(self - .mut_manifest() + .manifest_mut() .get_or_insert_nested_table(env_table.to_string().as_str())? .remove(name) .is_some()) From 6465e4f4df1cb483941f9b572dc18d8e3f7dda23 Mon Sep 17 00:00:00 2001 From: Julian Hofer Date: Fri, 29 Nov 2024 14:56:45 +0100 Subject: [PATCH 06/13] Rename to `get_or_insert_toml_array_mut` --- crates/pixi_manifest/src/manifests/source.rs | 4 ++-- crates/pixi_manifest/src/toml/document.rs | 2 +- src/global/project/manifest.rs | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/pixi_manifest/src/manifests/source.rs b/crates/pixi_manifest/src/manifests/source.rs index cfd9dd18c3..7e7b77e862 100644 --- a/crates/pixi_manifest/src/manifests/source.rs +++ b/crates/pixi_manifest/src/manifests/source.rs @@ -88,7 +88,7 @@ impl ManifestSource { .with_table(table); self.manifest_mut() - .get_or_insert_mut_toml_array(table_name.to_string().as_str(), array_name) + .get_or_insert_toml_array_mut(table_name.to_string().as_str(), array_name) } fn as_table_mut(&mut self) -> &mut Table { @@ -276,7 +276,7 @@ impl ManifestSource { |source: &mut ManifestSource, table, array| -> Result<(), TomlError> { source .manifest_mut() - .get_or_insert_mut_toml_array(table, array)? + .get_or_insert_toml_array_mut(table, array)? .push(requirement.to_string()); Ok(()) }; diff --git a/crates/pixi_manifest/src/toml/document.rs b/crates/pixi_manifest/src/toml/document.rs index ebcca6d6e3..2cda247cc8 100644 --- a/crates/pixi_manifest/src/toml/document.rs +++ b/crates/pixi_manifest/src/toml/document.rs @@ -128,7 +128,7 @@ impl TomlDocument { /// in table `table_name` in dotted form (e.g. `table1.table2.array`). /// /// If the array is not found, it is inserted into the document. - pub fn get_or_insert_mut_toml_array<'a>( + pub fn get_or_insert_toml_array_mut<'a>( &'a mut self, table_name: &str, array_name: &str, diff --git a/src/global/project/manifest.rs b/src/global/project/manifest.rs index fc6a561c54..301099abd0 100644 --- a/src/global/project/manifest.rs +++ b/src/global/project/manifest.rs @@ -92,7 +92,7 @@ impl Manifest { // Update self.document let channels_array = self .document - .get_or_insert_mut_toml_array(&format!("envs.{env_name}"), "channels")?; + .get_or_insert_toml_array_mut(&format!("envs.{env_name}"), "channels")?; for channel in channels { channels_array.push(channel.as_str()); } From 372fa5fea8d2e1be254bd4ce62a161c5d16a3e55 Mon Sep 17 00:00:00 2001 From: Julian Hofer Date: Fri, 29 Nov 2024 15:05:45 +0100 Subject: [PATCH 07/13] Extract function and types --- src/cli/upgrade.rs | 92 +++++++++++++++++++++++++--------------------- src/project/mod.rs | 13 +++++-- 2 files changed, 61 insertions(+), 44 deletions(-) diff --git a/src/cli/upgrade.rs b/src/cli/upgrade.rs index 893e6b32c1..fcd8bc4fcf 100644 --- a/src/cli/upgrade.rs +++ b/src/cli/upgrade.rs @@ -1,6 +1,7 @@ use std::cmp::Ordering; use crate::cli::cli_config::ProjectConfig; +use crate::project::{MatchSpecs, PypiDeps}; use crate::Project; use clap::Parser; use fancy_display::FancyDisplay; @@ -65,19 +66,63 @@ pub async fn execute(args: Args) -> miette::Result<()> { ) }; - // TODO: Also support build and host + let (match_specs, pypi_deps) = parse_specs(feature, &args, &project)?; + + let update_deps = project + .update_dependencies( + match_specs, + pypi_deps, + &args.prefix_update_config, + &args.specs.feature, + &[], + false, + args.dry_run, + ) + .await?; + + // Is there something to report? + if let Some(update_deps) = update_deps { + let diff = update_deps.lock_file_diff; + // Format as json? + if args.json { + let json_diff = LockFileJsonDiff::new(&project, diff); + let json = serde_json::to_string_pretty(&json_diff).expect("failed to convert to json"); + println!("{}", json); + } else { + diff.print() + .into_diagnostic() + .context("failed to print lock-file diff")?; + } + } else { + eprintln!( + "{}All packages are already up-to-date", + console::style(console::Emoji("✔ ", "")).green() + ); + } + + Project::warn_on_discovered_from_env(args.project_config.manifest_path.as_deref()); + Ok(()) +} + +/// Parses the specifications for dependencies from the given feature, arguments, and project. +/// +/// This function processes the dependencies and PyPi dependencies specified in the feature, +/// filters them based on the provided arguments, and returns the resulting match specifications +/// and PyPi dependencies. +fn parse_specs( + feature: &pixi_manifest::Feature, + args: &Args, + project: &Project, +) -> miette::Result<(MatchSpecs, PypiDeps)> { let spec_type = SpecType::Run; let match_spec_iter = feature .dependencies(spec_type, None) .into_iter() .flat_map(|deps| deps.into_owned()); - let pypi_deps_iter = feature .pypi_dependencies(None) .into_iter() .flat_map(|deps| deps.into_owned()); - - // If the user specified a package name, check to see if it is even there. if let Some(package_names) = &args.specs.packages { let available_packages = match_spec_iter .clone() @@ -93,7 +138,6 @@ pub async fn execute(args: Args) -> miette::Result<()> { ensure_package_exists(package, &available_packages)? } } - let match_specs = match_spec_iter // Don't upgrade excluded packages .filter(|(name, _)| match &args.specs.exclude { @@ -152,7 +196,6 @@ pub async fn execute(args: Args) -> miette::Result<()> { } }) .collect(); - let pypi_deps = pypi_deps_iter // Don't upgrade excluded packages .filter(|(name, _)| match &args.specs.exclude { @@ -194,47 +237,14 @@ pub async fn execute(args: Args) -> miette::Result<()> { .map(|(name, req)| { let location = project.manifest.document.pypi_dependency_location( &name, - None, + None, // TODO: add support for platforms &args.specs.feature, ); (name, (req, location)) }) .collect(); - let update_deps = project - .update_dependencies( - match_specs, - pypi_deps, - &args.prefix_update_config, - &args.specs.feature, - &[], - false, - args.dry_run, - ) - .await?; - - // Is there something to report? - if let Some(update_deps) = update_deps { - let diff = update_deps.lock_file_diff; - // Format as json? - if args.json { - let json_diff = LockFileJsonDiff::new(&project, diff); - let json = serde_json::to_string_pretty(&json_diff).expect("failed to convert to json"); - println!("{}", json); - } else { - diff.print() - .into_diagnostic() - .context("failed to print lock-file diff")?; - } - } else { - eprintln!( - "{}All packages are already up-to-date", - console::style(console::Emoji("✔ ", "")).green() - ); - } - - Project::warn_on_discovered_from_env(args.project_config.manifest_path.as_deref()); - Ok(()) + Ok((match_specs, pypi_deps)) } /// Ensures the existence of the specified package diff --git a/src/project/mod.rs b/src/project/mod.rs index 3932e0bd86..f1e2f0df7e 100644 --- a/src/project/mod.rs +++ b/src/project/mod.rs @@ -154,6 +154,13 @@ impl Borrow for Project { } } +pub type PypiDeps = indexmap::IndexMap< + PyPiPackageName, + (Requirement, Option), +>; + +pub type MatchSpecs = indexmap::IndexMap; + impl Project { /// Constructs a new instance from an internal manifest representation pub(crate) fn from_manifest(manifest: Manifest) -> Self { @@ -660,8 +667,8 @@ impl Project { #[allow(clippy::too_many_arguments)] pub async fn update_dependencies( &mut self, - match_specs: IndexMap, - pypi_deps: IndexMap)>, + match_specs: MatchSpecs, + pypi_deps: PypiDeps, prefix_update_config: &PrefixUpdateConfig, feature_name: &FeatureName, platforms: &[Platform], @@ -839,7 +846,7 @@ impl Project { fn unlock_packages( &self, lock_file: &LockFile, - conda_packages: HashSet, + conda_packages: HashSet, pypi_packages: HashSet, affected_environments: HashSet<(&str, Platform)>, ) -> LockFile { From 904316f69881f3518372db88d1586a8019553c0f Mon Sep 17 00:00:00 2001 From: Julian Hofer Date: Mon, 2 Dec 2024 13:37:51 +0100 Subject: [PATCH 08/13] Fix indentation --- tests/integration_python/test_main_cli.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/integration_python/test_main_cli.py b/tests/integration_python/test_main_cli.py index 3f35f3dd91..2557f053f2 100644 --- a/tests/integration_python/test_main_cli.py +++ b/tests/integration_python/test_main_cli.py @@ -534,17 +534,17 @@ def test_upgrade_dependency_location(pixi: Path, tmp_path: Path) -> None: # Making sure pixi places the upgraded package in the correct location manifest_path = tmp_path / "pyproject.toml" pyproject = """ - [project] - dependencies = [] - name = "test-upgrade" - requires-python = ">= 3.11" +[project] +dependencies = [] +name = "test-upgrade" +requires-python = ">= 3.11" - [tool.pixi.project] - channels = ["conda-forge"] - platforms = ["linux-64"] +[tool.pixi.project] +channels = ["conda-forge"] +platforms = ["linux-64"] - [tool.pixi.pypi-dependencies] - polars = "==0.*" +[tool.pixi.pypi-dependencies] +polars = "==0.*" """ manifest_path.write_text(pyproject) From e0eae34b79335e60f692eb65652d136c3a1cf46b Mon Sep 17 00:00:00 2001 From: Julian Hofer Date: Mon, 2 Dec 2024 15:16:51 +0100 Subject: [PATCH 09/13] Remove commented code and update implementation --- crates/pixi_manifest/src/manifests/source.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/crates/pixi_manifest/src/manifests/source.rs b/crates/pixi_manifest/src/manifests/source.rs index 7e7b77e862..d3d317f593 100644 --- a/crates/pixi_manifest/src/manifests/source.rs +++ b/crates/pixi_manifest/src/manifests/source.rs @@ -195,9 +195,6 @@ impl ManifestSource { platform: Option, feature_name: &FeatureName, ) -> Result<(), TomlError> { - // let dependency_table = - // self.get_or_insert_toml_table(platform, feature_name, spec_type.name())?; - let dependency_table = TableName::new() .with_prefix(self.table_prefix()) .with_platform(platform.as_ref()) @@ -208,8 +205,6 @@ impl ManifestSource { .get_or_insert_nested_table(dependency_table.to_string().as_str()) .map(|t| t.insert(name.as_normalized(), Item::Value(spec.to_toml_value())))?; - // dependency_table.insert(name.as_normalized(), - // Item::Value(spec.to_toml_value())); Ok(()) } @@ -280,7 +275,9 @@ impl ManifestSource { .push(requirement.to_string()); Ok(()) }; - if feature_name.is_default() { + if feature_name.is_default() + || matches!(location, Some(PypiDependencyLocation::Dependencies)) + { add_requirement(self, "project", "dependencies")? } else if matches!(location, Some(PypiDependencyLocation::OptionalDependencies)) { add_requirement( From 08855a22ef1be59849011e693bd6b0d680802b68 Mon Sep 17 00:00:00 2001 From: Julian Hofer Date: Mon, 2 Dec 2024 15:45:34 +0100 Subject: [PATCH 10/13] Extend test and make sure that python isn't added to pyproject.toml --- crates/pixi_manifest/src/lib.rs | 2 +- src/cli/upgrade.rs | 16 ++++++++++++ tests/integration_python/common.py | 10 ++++++++ tests/integration_python/test_main_cli.py | 30 +++++++++++++---------- tests/integration_python/test_run_cli.py | 11 +-------- 5 files changed, 45 insertions(+), 24 deletions(-) diff --git a/crates/pixi_manifest/src/lib.rs b/crates/pixi_manifest/src/lib.rs index 66dc1e392f..2f7d58c2ec 100644 --- a/crates/pixi_manifest/src/lib.rs +++ b/crates/pixi_manifest/src/lib.rs @@ -35,7 +35,7 @@ pub use features_ext::FeaturesExt; pub use has_features_iter::HasFeaturesIter; pub use has_manifest_ref::HasManifestRef; use itertools::Itertools; -pub use manifests::{Manifest, ManifestKind, WorkspaceManifest}; +pub use manifests::{Manifest, ManifestKind, ManifestSource, WorkspaceManifest}; use miette::Diagnostic; pub use preview::{KnownPreviewFeature, Preview, PreviewFeature}; pub use pypi::pypi_requirement::PyPiRequirement; diff --git a/src/cli/upgrade.rs b/src/cli/upgrade.rs index fcd8bc4fcf..179ee7834b 100644 --- a/src/cli/upgrade.rs +++ b/src/cli/upgrade.rs @@ -195,6 +195,22 @@ fn parse_specs( None } }) + // Only upgrade in pyproject.toml if it is explicitly mentioned in `tool.pixi.dependencies.python` + .filter(|(name, _)| { + if name.as_normalized() == "python" { + if let pixi_manifest::ManifestSource::PyProjectToml(document) = + project.manifest.document.clone() + { + if document + .get_nested_table("[tool.pixi.dependencies.python]") + .is_err() + { + return false; + } + } + } + true + }) .collect(); let pypi_deps = pypi_deps_iter // Don't upgrade excluded packages diff --git a/tests/integration_python/common.py b/tests/integration_python/common.py index 7751085fa1..f68c104484 100644 --- a/tests/integration_python/common.py +++ b/tests/integration_python/common.py @@ -7,6 +7,16 @@ PIXI_VERSION = "0.38.0" +ALL_PLATFORMS = '["linux-64", "osx-64", "win-64", "linux-ppc64le", "linux-aarch64"]' + +EMPTY_BOILERPLATE_PROJECT = f""" +[project] +name = "test" +channels = [] +platforms = {ALL_PLATFORMS} +""" + + class ExitCode(IntEnum): SUCCESS = 0 FAILURE = 1 diff --git a/tests/integration_python/test_main_cli.py b/tests/integration_python/test_main_cli.py index 2557f053f2..ea9eb77605 100644 --- a/tests/integration_python/test_main_cli.py +++ b/tests/integration_python/test_main_cli.py @@ -1,6 +1,7 @@ import os from pathlib import Path -from .common import verify_cli_command, ExitCode, PIXI_VERSION + +from .common import verify_cli_command, ExitCode, PIXI_VERSION, ALL_PLATFORMS import tomllib import json import pytest @@ -529,19 +530,19 @@ def test_upgrade_pypi_and_conda_package(pixi: Path, tmp_path: Path) -> None: @pytest.mark.slow -def test_upgrade_dependency_location(pixi: Path, tmp_path: Path) -> None: +def test_upgrade_dependency_location_pixi(pixi: Path, tmp_path: Path) -> None: # Test based on https://github.com/prefix-dev/pixi/issues/2470 # Making sure pixi places the upgraded package in the correct location manifest_path = tmp_path / "pyproject.toml" - pyproject = """ + pyproject = f""" [project] -dependencies = [] name = "test-upgrade" -requires-python = ">= 3.11" +dependencies = ["numpy==1.*"] +requires-python = ">3.10" [tool.pixi.project] channels = ["conda-forge"] -platforms = ["linux-64"] +platforms = {ALL_PLATFORMS} [tool.pixi.pypi-dependencies] polars = "==0.*" @@ -555,21 +556,24 @@ def test_upgrade_dependency_location(pixi: Path, tmp_path: Path) -> None: stderr_contains=["polars"], ) parsed_manifest = tomllib.loads(manifest_path.read_text()) - # Check that the requires-python is modified - requires_python = parsed_manifest["project"]["requires-python"] - assert "3.11" not in requires_python - # Check that `tool.pixi.dependencies.python` doesn't exist + # Check that `tool.pixi.dependencies.python` isn't added assert "python" not in parsed_manifest.get("tool", {}).get("pixi", {}).get("dependencies", {}) - # Check that the pypi-dependency is upgraded + # Check that the pypi-dependencies are upgraded polars_pypi = parsed_manifest["tool"]["pixi"]["pypi-dependencies"]["polars"] - assert polars_pypi assert polars_pypi != "==0.*" - # Check that the pypi-dependency doesn't exist in the project dependencies + # Check that project.dependencies are upgraded + numpy_pypi = parsed_manifest["project"]["dependencies"][0] + assert numpy_pypi != "==1.*" + + # Check that the polars doesn't exist in the project dependencies assert "polars" not in parsed_manifest["project"]["dependencies"] + # Check that numpy doesn't exist in the pypi-dependencies + assert "numpy" not in parsed_manifest["tool"]["pixi"]["pypi-dependencies"] + def test_upgrade_keep_info(pixi: Path, tmp_path: Path, multiple_versions_channel_1: str) -> None: manifest_path = tmp_path / "pixi.toml" diff --git a/tests/integration_python/test_run_cli.py b/tests/integration_python/test_run_cli.py index 619bf7f554..ed628cf073 100644 --- a/tests/integration_python/test_run_cli.py +++ b/tests/integration_python/test_run_cli.py @@ -1,18 +1,9 @@ import json from pathlib import Path -from .common import verify_cli_command, ExitCode, default_env_path +from .common import EMPTY_BOILERPLATE_PROJECT, verify_cli_command, ExitCode, default_env_path import tempfile import os -ALL_PLATFORMS = '["linux-64", "osx-64", "win-64", "linux-ppc64le", "linux-aarch64"]' - -EMPTY_BOILERPLATE_PROJECT = f""" -[project] -name = "test" -channels = [] -platforms = {ALL_PLATFORMS} -""" - def test_run_in_shell_environment(pixi: Path, tmp_path: Path) -> None: manifest = tmp_path.joinpath("pixi.toml") From e4819b75a9e19334e2f8919de1ed9cd6751126cf Mon Sep 17 00:00:00 2001 From: Julian Hofer Date: Mon, 2 Dec 2024 15:54:03 +0100 Subject: [PATCH 11/13] Add "requires-python" expectation --- tests/integration_python/test_main_cli.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/integration_python/test_main_cli.py b/tests/integration_python/test_main_cli.py index ea9eb77605..a4aefcc0e3 100644 --- a/tests/integration_python/test_main_cli.py +++ b/tests/integration_python/test_main_cli.py @@ -557,6 +557,9 @@ def test_upgrade_dependency_location_pixi(pixi: Path, tmp_path: Path) -> None: ) parsed_manifest = tomllib.loads(manifest_path.read_text()) + # Check that `requrires-python` is the same + assert parsed_manifest["project"]["requires-python"] == ">3.10" + # Check that `tool.pixi.dependencies.python` isn't added assert "python" not in parsed_manifest.get("tool", {}).get("pixi", {}).get("dependencies", {}) From e0de82f920cce2503de7326dd9ac24f65dc47d39 Mon Sep 17 00:00:00 2001 From: Julian Hofer Date: Mon, 2 Dec 2024 16:40:39 +0100 Subject: [PATCH 12/13] Update tests --- tests/integration_python/test_main_cli.py | 34 ++++++++++++++--------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/tests/integration_python/test_main_cli.py b/tests/integration_python/test_main_cli.py index a4aefcc0e3..5625cc7101 100644 --- a/tests/integration_python/test_main_cli.py +++ b/tests/integration_python/test_main_cli.py @@ -538,7 +538,13 @@ def test_upgrade_dependency_location_pixi(pixi: Path, tmp_path: Path) -> None: [project] name = "test-upgrade" dependencies = ["numpy==1.*"] -requires-python = ">3.10" +requires-python = "==3.13" + +[project.optional-dependencies] +cli = ["rich==12"] + +[dependency-groups] +test = ["pytest==6"] [tool.pixi.project] channels = ["conda-forge"] @@ -546,6 +552,9 @@ def test_upgrade_dependency_location_pixi(pixi: Path, tmp_path: Path) -> None: [tool.pixi.pypi-dependencies] polars = "==0.*" + +[tool.pixi.environments] +test = ["test"] """ manifest_path.write_text(pyproject) @@ -558,24 +567,23 @@ def test_upgrade_dependency_location_pixi(pixi: Path, tmp_path: Path) -> None: parsed_manifest = tomllib.loads(manifest_path.read_text()) # Check that `requrires-python` is the same - assert parsed_manifest["project"]["requires-python"] == ">3.10" + assert parsed_manifest["project"]["requires-python"] == "==3.13" # Check that `tool.pixi.dependencies.python` isn't added assert "python" not in parsed_manifest.get("tool", {}).get("pixi", {}).get("dependencies", {}) - # Check that the pypi-dependencies are upgraded - polars_pypi = parsed_manifest["tool"]["pixi"]["pypi-dependencies"]["polars"] - assert polars_pypi != "==0.*" - # Check that project.dependencies are upgraded - numpy_pypi = parsed_manifest["project"]["dependencies"][0] - assert numpy_pypi != "==1.*" - - # Check that the polars doesn't exist in the project dependencies - assert "polars" not in parsed_manifest["project"]["dependencies"] + project_dependencies = parsed_manifest["project"]["dependencies"] + numpy_pypi = project_dependencies[0] + assert "numpy" in numpy_pypi + assert "==1.*" not in numpy_pypi + assert "polars" not in project_dependencies - # Check that numpy doesn't exist in the pypi-dependencies - assert "numpy" not in parsed_manifest["tool"]["pixi"]["pypi-dependencies"] + # Check that the pypi-dependencies are upgraded + pypi_dependencies = parsed_manifest["tool"]["pixi"]["pypi-dependencies"] + polars_pypi = pypi_dependencies["polars"] + assert polars_pypi != "==0.*" + assert "numpy" not in pypi_dependencies def test_upgrade_keep_info(pixi: Path, tmp_path: Path, multiple_versions_channel_1: str) -> None: From a77b29bf065b7c516bdeb1045f3a7636fba61ee1 Mon Sep 17 00:00:00 2001 From: Ruben Arts Date: Tue, 3 Dec 2024 11:11:00 +0100 Subject: [PATCH 13/13] fmt after merge --- tests/integration_python/test_main_cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration_python/test_main_cli.py b/tests/integration_python/test_main_cli.py index c02ac7c107..682fd7f209 100644 --- a/tests/integration_python/test_main_cli.py +++ b/tests/integration_python/test_main_cli.py @@ -598,7 +598,7 @@ def test_upgrade_dependency_location_pixi(pixi: Path, tmp_path: Path) -> None: assert polars_pypi != "==0.*" assert "numpy" not in pypi_dependencies - + def test_upgrade_keep_info( pixi: Path, tmp_pixi_workspace: Path, multiple_versions_channel_1: str ) -> None: