fix(sdist): handle parent workspaces and refactor sdist generation

[messense]

This PR fixes and refactors sdist generation, with a focus on path dependencies that come from parent workspaces.

Fixes

When building a source distribution for a crate that is excluded from its parent workspace but depends on sibling crates in the parent workspace, maturin could panic or produce an sdist that failed to build.

This PR fixes those cases by:

• handling parent-workspace manifests that live outside the sdist root
• inlining workspace-inherited package fields for cross-workspace path dependencies when the parent workspace manifest is not packaged
• preserving inherited workspace dependencies, including git dependencies
• inlining inherited workspace lints ([lints] workspace = true)
• dropping explicit package.build = ... entries when the referenced build script was excluded from the sdist

Refactoring / simplification

This branch also includes a broader cleanup of the sdist implementation:

• split source_distribution.rs into focused submodules
• extracted pure path helpers and unpacking logic
• cached workspace inheritance data for reused path dependencies
• simplified target stripping to rely on cargo metadata for target discovery and source paths instead of reimplementing Cargo's target inference rules
• used Cargo-provided metadata where possible (for example, resolved publish values)

Result

The generated sdist is now more robust for excluded crates that depend on crates from a parent workspace, and the implementation is smaller and closer to Cargo's own model.

Closes #2766

[Copilot]

Pull request overview

Fixes a panic during maturin sdist when a path dependency’s workspace manifest lives outside the computed sdist root (e.g., excluded crate depending on a sibling workspace member), by avoiding an unconditional strip_prefix(...).unwrap() and adding logic to inline workspace-inherited Cargo.toml fields when the parent workspace manifest can’t be included.

Changes:

• Skip adding a dependency workspace Cargo.toml when it’s outside the sdist root (avoid StripPrefixError panic).
• Add workspace-inheritance inlining using cargo metadata for path dependencies whose workspace manifest is outside the sdist.
• Add a regression test and a new fixture workspace to reproduce the parent-workspace path dependency scenario.

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.

File	Description
src/source_distribution.rs	Avoid panic when workspace manifest is outside sdist root; adds workspace-inheritance inlining and refactors Cargo.toml rewriting helpers.
tests/run.rs	Adds regression test for parent-workspace path dependency sdist scenario.
test-crates/parent_workspace_sdist/**	New test workspace + crates to reproduce the issue.

Comments suppressed due to low confidence (3)

src/source_distribution.rs:98

• PR description says sdists with *.workspace = true will still fail when the workspace manifest is outside the sdist, but the code now attempts to inline workspace-inherited fields via resolved_package. Either update the PR description/release notes to reflect the new behavior, or scope the implementation back to “avoid panic only”.

    /// Resolved package metadata from `cargo metadata`, used to inline
    /// workspace-inherited fields when the workspace manifest is outside the sdist root.
    resolved_package: Option<cargo_metadata::Package>,

src/source_distribution.rs:275

• resolve_workspace_inheritance introduces fairly intricate rewriting of workspace-inherited package fields and workspace = true dependencies, but there are no unit tests covering the rewritten TOML output (especially around dependency renames and non-registry workspace deps). Adding targeted tests here would help prevent regressions and catch cases like alias/package-name handling.

/// Inlines workspace-inherited fields in a path dependency's `Cargo.toml`
/// using resolved values from `cargo metadata`.
///
/// This is needed when the dependency's workspace manifest falls outside the
/// sdist root (e.g. a crate excluded from a parent workspace that depends on
/// sibling workspace members).  Without this, `field.workspace = true` entries
/// would fail to resolve when building from the sdist.
fn resolve_workspace_inheritance(document: &mut DocumentMut, resolved: &cargo_metadata::Package) {
    // Resolve `[package]` fields that support `field.workspace = true`
    if let Some(package) = document.get_mut("package").and_then(|p| p.as_table_mut()) {
        let version_str = resolved.version.to_string();
        let edition_str = resolved.edition.to_string();
        let rust_version_str = resolved.rust_version.as_ref().map(|v| v.to_string());
        let string_fields: &[(&str, Option<&str>)] = &[
            ("version", Some(&version_str)),
            ("edition", Some(&edition_str)),
            ("description", resolved.description.as_deref()),
            ("license", resolved.license.as_deref()),
            ("repository", resolved.repository.as_deref()),
            ("homepage", resolved.homepage.as_deref()),
            ("documentation", resolved.documentation.as_deref()),
            ("rust-version", rust_version_str.as_deref()),
        ];

tests/run.rs:913

• This regression test only asserts the sdist file list; it doesn’t validate that the generated shared_crate/Cargo.toml is actually buildable (e.g., edition.workspace = true should be inlined to edition = "2021" when the parent workspace manifest isn’t included). Consider using the expected_cargo_toml hook to assert the rewritten manifest contents for at least the inherited fields.

/// Test sdist with path dependency from parent workspace.
/// Reproduces the scenario where a crate is excluded from a parent workspace
/// but depends on sibling crates that ARE in the parent workspace.
/// Before the fix, this would panic with StripPrefixError.
#[test]
fn lib_with_parent_workspace_path_dep_sdist() {
    handle_result(other::test_source_distribution(
        "test-crates/parent_workspace_sdist/crates/pysof",
        SdistGenerator::Cargo,
        expect![[r#"
            {
                "pysof-0.1.0/PKG-INFO",
                "pysof-0.1.0/pyproject.toml",
                "pysof-0.1.0/pysof/.gitignore",
                "pysof-0.1.0/pysof/Cargo.lock",
                "pysof-0.1.0/pysof/Cargo.toml",
                "pysof-0.1.0/pysof/src/lib.rs",
                "pysof-0.1.0/shared_crate/Cargo.toml",
                "pysof-0.1.0/shared_crate/src/lib.rs",
            }
        "#]],
        None,
        "sdist-lib-with-parent-workspace-path-dep",
    ))
}

[Copilot]

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

src/source_distribution.rs:434

• When target is None, this can still match a target-specific dependency entry from cargo metadata (i.e. where d.target.is_some()), depending on iteration order. That can cause the base [dependencies] workspace entries to be resolved using the wrong target cfg. Consider adding an explicit branch: if target.is_none(), require d.target.is_none() (and similarly keep the existing equality check when target.is_some()).

fn find_resolved_dep(
    resolved: &cargo_metadata::Package,
    name: &str,
    target: Option<&str>,
) -> Option<ResolvedDep> {
    resolved.dependencies.iter().find_map(|d| {
        // Match by name, considering renames
        let matches_name = d.rename.as_deref() == Some(name) || d.name == name;
        if !matches_name {
            return None;
        }
        // If a target is specified, match against it
        if let Some(target_str) = target
            && d.target.as_ref().map(|t| t.to_string()).as_deref() != Some(target_str)
        {
            return None;
        }

src/source_distribution.rs:1787

• On Windows (or any platform with path prefixes), if common_path_prefix(from, to) returns None (e.g. different drive letters / prefixes), to_rest can remain absolute; result.push(to_rest) then produces an absolute path. Emitting an absolute path = \"...\" into rewritten Cargo.toml is usually undesirable for sdists and can lead to non-reproducible builds. Consider detecting the 'no common prefix' case and returning an error (change return type to Result<PathBuf>) or otherwise handling differing prefixes explicitly (e.g. refuse to rewrite to a relative path when impossible).

fn relative_path(from: &Path, to: &Path) -> PathBuf {
    let common = common_path_prefix(from, to).unwrap_or_default();
    let from_rest = from.strip_prefix(&common).unwrap_or(Path::new(""));
    let to_rest = to.strip_prefix(&common).unwrap_or(to);
    let mut result = PathBuf::new();
    for _ in from_rest.components() {
        result.push("..");
    }
    result.push(to_rest);
    result
}

src/source_distribution.rs:1860

• This unit test hard-codes Unix-style absolute paths (e.g. /a/b/c) and expectations using / separators. If CI runs tests on Windows, these cases are likely to fail or behave differently due to prefixes/drive letters and separator handling. Consider making the test platform-aware (e.g. #[cfg(unix)] + a separate Windows test), or construct paths using PathBuf components and compare using PathBuf values appropriate for the platform.

    #[test]
    fn test_relative_path() {
        let test_cases = vec![
            ("/a/b/c", "/a/b/d", "../d"),
            ("/a/b", "/a/b/c/d", "c/d"),
            ("/a/b/c", "/a/b", ".."),
            ("/a/b/c", "/a/x/y", "../../x/y"),
            ("/a/b", "/a/b", ""),
        ];
        for (from, to, expected) in test_cases {
            assert_eq!(
                relative_path(Path::new(from), Path::new(to)),
                PathBuf::from(expected),
                "relative_path({from:?}, {to:?}) should equal {expected:?}",
            );
        }
    }

src/source_distribution.rs:260

• The new resolve_workspace_inheritance behavior is central to the fix, but there are no direct unit tests asserting that field.workspace = true entries (e.g. edition.workspace = true) are actually rewritten to concrete values, and that dep = { workspace = true } entries are replaced with the expected TOML. Adding a focused test that feeds a small DocumentMut containing workspace-inherited fields and verifies the rewritten output would make regressions much easier to catch.

/// Inlines workspace-inherited fields in a path dependency's `Cargo.toml`
/// using resolved values from `cargo metadata`.
///
/// This is needed when the dependency's workspace manifest falls outside the
/// sdist root (e.g. a crate excluded from a parent workspace that depends on
/// sibling workspace members).  Without this, `field.workspace = true` entries
/// would fail to resolve when building from the sdist.
fn resolve_workspace_inheritance(document: &mut DocumentMut, resolved: &cargo_metadata::Package) {

[Copilot]

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

src/source_distribution.rs:1846

• test_relative_path hard-codes POSIX-style absolute paths and expected strings with / separators. On Windows, PathBuf comparisons are separator-sensitive and relative_path will likely produce ..\\d-style paths, so this test can fail depending on platform. Prefer constructing inputs/expected values using PathBuf/Path::join (or compare normalized slash strings) to make the test platform-independent.

    fn test_relative_path() {
        let test_cases = vec![
            ("/a/b/c", "/a/b/d", "../d"),
            ("/a/b", "/a/b/c/d", "c/d"),
            ("/a/b/c", "/a/b", ".."),
            ("/a/b/c", "/a/x/y", "../../x/y"),
            ("/a/b", "/a/b", ""),
        ];

tests/run.rs:903

• The comment says "readme is removed" but the expectation below asserts readme = "README.md" is present. It looks like the intent is that the workspace = true marker is removed and the path rewritten, not that the readme field is removed entirely.

    // shared_crate uses `edition.workspace = true` and `readme.workspace = true`
    // from the parent workspace. Since the parent workspace Cargo.toml is NOT
    // included in the sdist, these must be inlined to their resolved values.
    // `readme` is removed (the file is copied separately and the path rewritten),
    // and `edition` is inlined to "2021".

[Copilot]

Pull request overview

Copilot reviewed 16 out of 17 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

tests/run.rs:903

• The comment says "readme is removed", but this test’s expected Cargo.toml output includes readme = "README.md". Please update the comment to match the actual behavior (i.e., readme is rewritten to a concrete path/filename rather than removed).

    // shared_crate uses `edition.workspace = true` and `readme.workspace = true`
    // from the parent workspace. Since the parent workspace Cargo.toml is NOT
    // included in the sdist, these must be inlined to their resolved values.
    // `readme` is removed (the file is copied separately and the path rewritten),
    // and `edition` is inlined to "2021".

src/source_distribution/utils.rs:76

• If common_path_prefix(from, to) returns None (e.g., Windows paths on different drive letters), common becomes empty, and to_rest can remain absolute. In that case result.push(to_rest) will discard the accumulated .. components and produce an absolute path, which is not a valid relative path. Handle the None case explicitly (e.g., return to.to_path_buf() or otherwise signal “not computable”), and add a test that covers the “no common prefix” scenario.

pub(super) fn relative_path(from: &Path, to: &Path) -> PathBuf {
    let common = common_path_prefix(from, to).unwrap_or_default();
    let from_rest = from.strip_prefix(&common).unwrap_or(Path::new(""));
    let to_rest = to.strip_prefix(&common).unwrap_or(to);
    let mut result = PathBuf::new();
    for _ in from_rest.components() {
        result.push("..");
    }
    result.push(to_rest);
    result
}

src/source_distribution/path_deps.rs:127

• resolved_package is cloned and stored for every discovered path dependency, but it’s only used later when the dependency’s workspace manifest falls outside the sdist root. Consider avoiding the unconditional clone by storing a lightweight reference key (e.g., PackageId) and looking up the package in cargo_metadata.packages when/if inlining is required, or only cloning when it’s definitively needed.

                // The root cargo metadata already resolves all package fields
                // (including workspace-inherited ones) for every package in the
                // dependency graph, regardless of which workspace they belong to.
                path_deps.insert(
                    dep_name.clone(),
                    PathDependency {
                        manifest_path: dep_manifest_path.into_std_path_buf(),
                        workspace_root: dep_workspace_root,
                        readme: dep_pkg
                            .readme
                            .as_ref()
                            .map(|r| r.clone().into_std_path_buf()),
                        license_file: dep_pkg
                            .license_file
                            .as_ref()
                            .map(|l| l.clone().into_std_path_buf()),
                        resolved_package: Some((*dep_pkg).clone()),
                    },
                );

[Copilot]

Pull request overview

Copilot reviewed 16 out of 18 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (4)

tests/run.rs:896

• The PR description notes that path deps using workspace inheritance will still fail to build from the sdist, but this test (and the new inlining logic) asserts that *.workspace = true fields are successfully inlined when the parent workspace manifest is outside the sdist root. Please align the PR description with the implemented behavior (or clarify the remaining unsupported cases).

/// Also verifies that workspace-inherited fields (`edition.workspace = true`,
/// `readme.workspace = true`) are correctly inlined in the path dependency's
/// Cargo.toml when the parent workspace manifest is outside the sdist root.

src/source_distribution/mod.rs:9

• ModuleWriter is imported but never used in this module, which will trigger an unused-import warning (and likely fail CI if warnings are denied). Remove the import here (and keep the usage via crate::module_writer::... where needed).

use crate::{BuildContext, ModuleWriter, PyProjectToml, SDistWriter, VirtualWriter};
use anyhow::{Context, Result, bail};

src/source_distribution/pyproject.rs:3

• ModuleWriter is imported but never used in this file, which will cause an unused-import warning. Remove it from the use crate::{...} list.

use crate::pyproject_toml::Format;
use crate::{ModuleWriter, PyProjectToml, SDistWriter, VirtualWriter};
use anyhow::{Context, Result};

src/source_distribution/utils.rs:71

• The doc comment for relative_path says both inputs “should be absolute”, but the function is also used/tests cover relative inputs (e.g. "foo"). Update the documentation to reflect the actual supported inputs/behavior (or enforce absoluteness in code/tests).

/// Compute a relative path from `from` directory to `to` path.
///
/// Both paths should be absolute. Returns a relative path that, when joined
/// with `from`, resolves to `to`.  If the paths share no common prefix
/// (e.g. different Windows drive letters), the absolute `to` path is returned
/// as-is since no relative traversal is possible.
pub(super) fn relative_path(from: &Path, to: &Path) -> PathBuf {
    let Some(common) = common_path_prefix(from, to) else {
        // No common prefix — fall back to the absolute target path.
        return to.to_path_buf();

[Copilot]

Pull request overview

Copilot reviewed 17 out of 19 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 18 out of 20 changed files in this pull request and generated 4 comments.