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

Warn when trying to uv sync a package without build configuration #7420

Merged
merged 7 commits into from
Sep 16, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
48 changes: 40 additions & 8 deletions crates/uv-workspace/src/pyproject.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ pub struct PyProjectToml {
#[serde(skip)]
pub raw: String,

/// Used to determine whether a `build-system` is present.
/// Used to determine whether a `build-system` section is present.
#[serde(default, skip_serializing)]
build_system: Option<serde::de::IgnoredAny>,
}
Expand All @@ -70,18 +70,43 @@ impl PyProjectToml {
/// non-package ("virtual") project.
pub fn is_package(&self) -> bool {
// If `tool.uv.package` is set, defer to that explicit setting.
if let Some(is_package) = self
.tool
.as_ref()
.and_then(|tool| tool.uv.as_ref())
.and_then(|uv| uv.package)
{
if let Some(is_package) = self.is_uv_package() {
return is_package;
}

// Otherwise, a project is assumed to be a package if `build-system` is present.
self.build_system.is_some()
lucab marked this conversation as resolved.
Show resolved Hide resolved
}

/// Returns whether the project should be considered a packaged script.
lucab marked this conversation as resolved.
Show resolved Hide resolved
///
/// This detects if a project is declared as a uv package, or if it has
/// a scripts-related section defined.
pub fn is_packaged_script(&self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

I'd be surprised if we needed this exact combination again. I'd just add a function like has_scripts and && that with is_package.

if let Some(is_package) = self.is_uv_package() {
return is_package;
};

if let Some(ref project) = self.project {
let is_script = project.gui_scripts.is_some() || project.scripts.is_some();
return is_script;
};

false
}

/// Returns whether the project has a `build-system` section.
pub fn has_build_system(&self) -> bool {
self.build_system.is_some()
}

/// Returns the value of `tool.uv.package`, if explicitly set.
fn is_uv_package(&self) -> Option<bool> {
lucab marked this conversation as resolved.
Show resolved Hide resolved
self.tool
.as_ref()
.and_then(|tool| tool.uv.as_ref())
.and_then(|uv| uv.package)
}
}

// Ignore raw document in comparison.
Expand All @@ -102,7 +127,7 @@ impl AsRef<[u8]> for PyProjectToml {
/// PEP 621 project metadata (`project`).
///
/// See <https://packaging.python.org/en/latest/specifications/pyproject-toml>.
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)]
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: IgnoredAny doesn't implement Eq, but the trait here seemed unused and fine to drop. Alternatively, we can either 1) store TOML tables in here, or 2) fully deserialize both scripts fields.

#[serde(rename_all = "kebab-case")]
pub struct Project {
/// The name of the project
Expand All @@ -113,6 +138,13 @@ pub struct Project {
pub requires_python: Option<VersionSpecifiers>,
/// The optional dependencies of the project.
pub optional_dependencies: Option<BTreeMap<ExtraName, Vec<String>>>,

/// Used to determine whether a `gui-scripts` section is present.
#[serde(default, skip_serializing)]
pub(crate) gui_scripts: Option<serde::de::IgnoredAny>,
/// Used to determine whether a `scripts` section is present.
#[serde(default, skip_serializing)]
pub(crate) scripts: Option<serde::de::IgnoredAny>,
}

#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)]
Expand Down
6 changes: 6 additions & 0 deletions crates/uv/src/commands/project/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,12 @@ pub(crate) async fn run(
.await?
};

if project.workspace().pyproject_toml().is_packaged_script()
Copy link
Contributor Author

@lucab lucab Sep 16, 2024

Choose a reason for hiding this comment

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

Note: sync is performed on CLI add/remove/sync/run, but it seems to annoying to warn on each command. Thus this new warning is only displayed at uv run time, which should be the most effective place. But can be expanded if we prefer.

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to omit at add / remove. We might want it at sync, otherwise if you sync then active the environment (instead of using uv run), you'll never see it.

Maybe with some special-casing per https://github.com/astral-sh/uv/pull/7420/files#r1761054908 we can make the warnings quieter in uv run.

&& !project.workspace().pyproject_toml().has_build_system()
{
warn_user!("No `build-system` section found for the package; consider adding one to correctly build the project");
}

if no_sync {
debug!("Skipping environment synchronization due to `--no-sync`");
} else {
Expand Down
42 changes: 42 additions & 0 deletions crates/uv/tests/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1950,3 +1950,45 @@ fn run_invalid_project_table() -> Result<()> {

Ok(())
}

#[test]
fn run_script_without_build_system() -> Result<()> {
// Check warning message for https://github.com/astral-sh/uv/issues/6998.
lucab marked this conversation as resolved.
Show resolved Hide resolved

let context = TestContext::new("3.12");

let pyproject_toml = context.temp_dir.child("pyproject.toml");
pyproject_toml.write_str(indoc! { r#"
[project]
name = "foo"
version = "0.1.0"
requires-python = ">=3.12"
dependencies = []

[project.scripts]
entry = "foo:custom_entry"
"#
})?;
lucab marked this conversation as resolved.
Show resolved Hide resolved

let test_script = context.temp_dir.child("src/__init__.py");
test_script.write_str(indoc! { r#"
def custom_entry():
print!("Hello")
"#
})?;

uv_snapshot!(context.filters(), context.run().arg("entry"), @r###"
success: false
exit_code: 2
----- stdout -----

----- stderr -----
warning: No `build-system` section found for the package; consider adding one to correctly build the project
lucab marked this conversation as resolved.
Show resolved Hide resolved
Resolved 1 package in [TIME]
Audited in [TIME]
error: Failed to spawn: `entry`
Caused by: No such file or directory (os error 2)
"###);

Ok(())
}
Loading