Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve static metadata extraction for Poetry projects #4182

Merged
merged 2 commits into from
Jun 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 34 additions & 3 deletions crates/pypi-types/src/metadata.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ pub enum MetadataError {
UnsupportedMetadataVersion(String),
#[error("The following field was marked as dynamic: {0}")]
DynamicField(&'static str),
#[error("The project uses Poetry's syntax to declare its dependencies, despite including a `project` table in `pyproject.toml`")]
PoetrySyntax,
}

impl From<Pep508Error<VerbatimParsedUrl>> for MetadataError {
Expand Down Expand Up @@ -210,6 +212,15 @@ impl Metadata23 {
}
}

// If dependencies are declared with Poetry, and `project.dependencies` is omitted, treat
// the dependencies as dynamic. The inclusion of a `project` table without defining
// `project.dependencies` is almost certainly an error.
Comment on lines +215 to +217
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should warn, that's a spec violation

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a spec violation in Poetry, right? What the user wrote is totally fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging for now but will follow-up based on your reply.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should warn because using poetry <2 and [project] together without setting dynamic has unintended consequences, either on their own is technically compliant

if project.dependencies.is_none()
&& pyproject_toml.tool.and_then(|tool| tool.poetry).is_some()
{
return Err(MetadataError::PoetrySyntax);
}

let name = project.name;
let version = project
.version
Expand Down Expand Up @@ -257,11 +268,11 @@ impl Metadata23 {
}

/// A `pyproject.toml` as specified in PEP 517.
#[derive(Deserialize, Debug, Clone)]
#[derive(Deserialize, Debug)]
#[serde(rename_all = "kebab-case")]
struct PyProjectToml {
/// Project metadata
project: Option<Project>,
tool: Option<Tool>,
}

/// PEP 621 project metadata.
Expand All @@ -270,7 +281,7 @@ struct PyProjectToml {
/// relevant for dependency resolution.
///
/// See <https://packaging.python.org/en/latest/specifications/pyproject-toml>.
#[derive(Deserialize, Debug, Clone)]
#[derive(Deserialize, Debug)]
#[serde(rename_all = "kebab-case")]
struct Project {
/// The name of the project
Expand All @@ -288,6 +299,17 @@ struct Project {
dynamic: Option<Vec<String>>,
}

#[derive(Deserialize, Debug)]
#[serde(rename_all = "kebab-case")]
struct Tool {
poetry: Option<ToolPoetry>,
}

#[derive(Deserialize, Debug)]
#[serde(rename_all = "kebab-case")]
#[allow(clippy::empty_structs_with_brackets)]
struct ToolPoetry {}

/// Python Package Metadata 1.0 and later as specified in
/// <https://peps.python.org/pep-0241/>.
///
Expand Down Expand Up @@ -369,6 +391,15 @@ impl RequiresDist {
}
}

// If dependencies are declared with Poetry, and `project.dependencies` is omitted, treat
// the dependencies as dynamic. The inclusion of a `project` table without defining
// `project.dependencies` is almost certainly an error.
if project.dependencies.is_none()
&& pyproject_toml.tool.and_then(|tool| tool.poetry).is_some()
{
return Err(MetadataError::PoetrySyntax);
}

let name = project.name;

// Extract the requirements.
Expand Down
8 changes: 4 additions & 4 deletions crates/uv-distribution/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,12 @@ pub enum Error {
NotFound(PathBuf),
#[error("The source distribution is missing a `PKG-INFO` file")]
MissingPkgInfo,
#[error("The source distribution does not support static metadata in `PKG-INFO`")]
DynamicPkgInfo(#[source] pypi_types::MetadataError),
#[error("Failed to extract static metadata from `PKG-INFO`")]
PkgInfo(#[source] pypi_types::MetadataError),
#[error("The source distribution is missing a `pyproject.toml` file")]
MissingPyprojectToml,
#[error("The source distribution does not support static metadata in `pyproject.toml`")]
DynamicPyprojectToml(#[source] pypi_types::MetadataError),
#[error("Failed to extract static metadata from `pyproject.toml`")]
PyprojectToml(#[source] pypi_types::MetadataError),
#[error("Unsupported scheme in URL: {0}")]
UnsupportedScheme(String),
#[error(transparent)]
Expand Down
31 changes: 24 additions & 7 deletions crates/uv-distribution/src/source/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1411,7 +1411,16 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> {

return Ok(Some(metadata));
}
Err(err @ (Error::MissingPkgInfo | Error::DynamicPkgInfo(_))) => {
Err(
err @ (Error::MissingPkgInfo
| Error::PkgInfo(
pypi_types::MetadataError::Pep508Error(_)
| pypi_types::MetadataError::DynamicField(_)
| pypi_types::MetadataError::FieldNotFound(_)
| pypi_types::MetadataError::UnsupportedMetadataVersion(_)
| pypi_types::MetadataError::PoetrySyntax,
)),
) => {
debug!("No static `PKG-INFO` available for: {source} ({err:?})");
}
Err(err) => return Err(err),
Expand All @@ -1427,7 +1436,16 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> {

return Ok(Some(metadata));
}
Err(err @ (Error::MissingPyprojectToml | Error::DynamicPyprojectToml(_))) => {
Err(
err @ (Error::MissingPyprojectToml
| Error::PyprojectToml(
pypi_types::MetadataError::Pep508Error(_)
| pypi_types::MetadataError::DynamicField(_)
| pypi_types::MetadataError::FieldNotFound(_)
| pypi_types::MetadataError::UnsupportedMetadataVersion(_)
| pypi_types::MetadataError::PoetrySyntax,
)),
) => {
debug!("No static `pyproject.toml` available for: {source} ({err:?})");
}
Err(err) => return Err(err),
Expand Down Expand Up @@ -1602,7 +1620,7 @@ async fn read_pkg_info(
};

// Parse the metadata.
let metadata = Metadata23::parse_pkg_info(&content).map_err(Error::DynamicPkgInfo)?;
let metadata = Metadata23::parse_pkg_info(&content).map_err(Error::PkgInfo)?;

Ok(metadata)
}
Expand All @@ -1627,8 +1645,7 @@ async fn read_pyproject_toml(
};

// Parse the metadata.
let metadata =
Metadata23::parse_pyproject_toml(&content).map_err(Error::DynamicPyprojectToml)?;
let metadata = Metadata23::parse_pyproject_toml(&content).map_err(Error::PyprojectToml)?;

Ok(metadata)
}
Expand All @@ -1646,8 +1663,8 @@ async fn read_requires_dist(project_root: &Path) -> Result<pypi_types::RequiresD
};

// Parse the metadata.
let requires_dist = pypi_types::RequiresDist::parse_pyproject_toml(&content)
.map_err(Error::DynamicPyprojectToml)?;
let requires_dist =
pypi_types::RequiresDist::parse_pyproject_toml(&content).map_err(Error::PyprojectToml)?;

Ok(requires_dist)
}
Expand Down
104 changes: 104 additions & 0 deletions crates/uv/tests/pip_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -615,6 +615,110 @@ build-backend = "poetry.core.masonry.api"
Ok(())
}

/// Compile a `pyproject.toml` file with a `poetry` section and a `project` section without a
/// `dependencies` field, which should be treated as an empty list.
#[test]
fn compile_pyproject_toml_poetry_empty_dependencies() -> Result<()> {
let context = TestContext::new("3.12");
let pyproject_toml = context.temp_dir.child("pyproject.toml");
pyproject_toml.write_str(
r#"[project]
name = "poetry-editable"
version = "0.1.0"
description = ""
authors = ["Astral Software Inc. <hey@astral.sh>"]

[tool.poetry]
name = "poetry-editable"
version = "0.1.0"
description = ""
authors = ["Astral Software Inc. <hey@astral.sh>"]

[tool.poetry.dependencies]
python = "^3.10"
anyio = "^3"

[build-system]
requires = ["poetry-core"]
build-backend = "poetry.core.masonry.api"
"#,
)?;

uv_snapshot!(context.compile()
.arg("pyproject.toml"), @r###"
success: true
exit_code: 0
----- stdout -----
# This file was autogenerated by uv via the following command:
# uv pip compile --cache-dir [CACHE_DIR] --exclude-newer 2024-03-25T00:00:00Z pyproject.toml
anyio==3.7.1
# via poetry-editable (pyproject.toml)
idna==3.6
# via anyio
sniffio==1.3.1
# via anyio

----- stderr -----
Resolved 3 packages in [TIME]
"###
);

Ok(())
}

/// Compile a `pyproject.toml` file with a `poetry` section and a `project` section with an invalid
/// `dependencies` field.
#[test]
fn compile_pyproject_toml_poetry_invalid_dependencies() -> Result<()> {
let context = TestContext::new("3.12");
let pyproject_toml = context.temp_dir.child("pyproject.toml");
pyproject_toml.write_str(
r#"[project]
name = "poetry-editable"
version = "0.1.0"
description = ""
authors = ["Astral Software Inc. <hey@astral.sh>"]

[tool.poetry]
name = "poetry-editable"
version = "0.1.0"
description = ""
authors = ["Astral Software Inc. <hey@astral.sh>"]

[project.dependencies]
python = "^3.12"
msgspec = "^0.18.4"

[tool.poetry.dependencies]
python = "^3.10"
anyio = "^3"

[build-system]
requires = ["poetry-core"]
build-backend = "poetry.core.masonry.api"
"#,
)?;

uv_snapshot!(context.compile()
.arg("pyproject.toml"), @r###"
success: false
exit_code: 2
----- stdout -----

----- stderr -----
error: Failed to extract static metadata from `pyproject.toml`
Caused by: TOML parse error at line 13, column 1
|
13 | [project.dependencies]
| ^^^^^^^^^^^^^^^^^^^^^^
invalid type: map, expected a sequence

"###
);

Ok(())
}

/// Compile a `pyproject.toml` file that uses setuptools as the build backend.
#[test]
fn compile_pyproject_toml_setuptools() -> Result<()> {
Expand Down
Loading