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

Cannot build vendored git dependency if it inherits info from a workspace #11192

Closed
hunts opened this issue Oct 7, 2022 · 12 comments · Fixed by #11414
Closed

Cannot build vendored git dependency if it inherits info from a workspace #11192

hunts opened this issue Oct 7, 2022 · 12 comments · Fixed by #11414
Labels
A-workspace-inheritance Area: workspace inheritance RFC 2906 A-workspaces Area: workspaces C-bug Category: bug Command-vendor

Comments

@hunts
Copy link

hunts commented Oct 7, 2022

Problem

If we cargo vendor our dependencies, and one of the crates in the vendor folder is inheriting package info from its Workspace, cargo build --offline would fail with errors like:

Caused by:
  error inheriting `version` from workspace root manifest's `workspace.package.version`
Caused by:
  failed to find a workspace root

Steps

  1. Add a crate that is from a project using Workspace inheritance
$ tail Cargo.toml
...
[dependencies]
lib-in-workspace = { git = "ssh://git@internal-git-service/rust-project-using-workspace.git", tag = "v0.176.0" }
...
  1. Vendor dependencies:
$ cargo vendor >> .cargo/config.toml

$ tail .cargo/config.toml
...
[source."ssh://git@internal-git-service/rust-project-using-workspace.git"]
git = "ssh://git@internal-git-service/rust-project-using-workspace.git"
tag = "v0.176.0"
replace-with = "vendored-sources"

[source.vendored-sources]
directory = "vendor"

$ cat Cargo.lock
...
[[package]]
name = "lib-in-workspace"
version = "0.176.0"
source = "git+ssh://git@internal-git-service/rust-project-using-workspace.git?tag=v0.176.0#7efcf95d618e9af79431e036414903fe3b4954b8"
dependencies = [
 "aho-corasick",
 "bstr",
 "cc",
 ...
]
...
  1. Build app offline
$ cargo build --offline --release

error: failed to get `aho-corasick` as a dependency of package `app v0.1.0 (/home/hunts/code/app)`

Caused by:
  failed to load source for dependency `aho-corasick`

Caused by:
  Unable to update registry `crates-io`

Caused by:
  failed to update replaced source registry `crates-io`

Caused by:
  failed to parse manifest at `/home/hunts/code/app/vendor/lib-in-workspace/Cargo.toml`

Caused by:
  error inheriting `version` from workspace root manifest's `workspace.package.version`

Caused by:
  failed to find a workspace root
  1. Check what's in the crate's Cargo.toml
$ head vendor/lib-in-workspace/Cargo.toml

[package]
name = "lib-in-workspace"
version.workspace = true
authors.workspace = true
description.workspace = true
edition.workspace = true
publish.workspace = true

Possible Solution(s)

I'm no familiar with the workspace design, but it feels like we need to replace the workspace variables with real values when vendor a crate, or will need to provide metadata of the workspace the crate is sourced from.

Notes

I've anonymized the package/lib names related to our internal systems, hope that won't cause inconvenience in understanding the problem.

Version

cargo 1.64.0 (387270bc7 2022-09-16)
release: 1.64.0
commit-hash: 387270bc7f446d17869c7f208207c73231d6a252
commit-date: 2022-09-16
host: x86_64-unknown-linux-gnu
libgit2: 1.4.2 (sys:0.14.2 vendored)
libcurl: 7.83.1-DEV (sys:0.4.55+curl-7.83.1 vendored ssl:OpenSSL/1.1.1q)
os: Ubuntu 22.04 (jammy) [64-bit]
@hunts hunts added the C-bug Category: bug label Oct 7, 2022
@weihanglo weihanglo added A-workspaces Area: workspaces A-workspace-inheritance Area: workspace inheritance RFC 2906 Command-vendor labels Oct 7, 2022
@hunts hunts changed the title Workplace inheritance does not work in offline mode Workspace inheritance does not work in offline mode Oct 7, 2022
@weihanglo
Copy link
Member

Thank you for reporting this bug! @Muscraft and I had a talk, and we feel that it might take longer before a better design for this come out. So, in the meanwhile, we are seeking help for polishing the error message. Something explicitly tells user the cause is great, such like "vendoring a git dependency with inherited fields from a workspace is not supported yet".

@ipetkov
Copy link

ipetkov commented Oct 9, 2022

@weihanglo just curious for my own understanding, is the limitation here that cargo vendor doesn't (yet) know how to vendor such crates, or is it a broader issue that cargo itself cannot support offline building of such crates?

I'm interested in supporting dependencies with workspace inheritance in Crane (tl;dr an alternative implementation of cargo vendor to work with Nix) and wanted to understand where/what the limitations actually are!

@weihanglo weihanglo changed the title Workspace inheritance does not work in offline mode Cannot build vendored git dependency if it inherits info from a workspace Oct 9, 2022
@weihanglo
Copy link
Member

@ipetkov Haven't dived deep yet but I don't think this affects general cases.

So far it seems to happen only when building a vendored git dependency which inherits info from its parent workspace. I guess it's cp_sources copying the crate sources without copying the whole workspace, and then cargo realizes workspace information is not there.

This is a reproducible case in the form of cargo's own test suite if anyone is interested.

Click to see test code
#[cargo_test]
fn vendor_git_source_from_workspace_with_inheritance() {
    let git = git::new("gitworkspace", |p| {
        p.file(
            "Cargo.toml",
            r#"
                [workspace]
                member = ["foo"]

                [workspace.package]
                version = "0.1.0"
            "#,
        )
        .file(
            "bar/Cargo.toml",
            r#"
                [package]
                name = "bar"
                version.workspace = true
            "#,
        )
        .file("bar/src/lib.rs", "")
    });

    let p = project()
        .file(
            "Cargo.toml",
            &format!(
                r#"
                    [package]
                    name = "foo"
                    version = "0.1.0"

                    [dependencies]
                    bar = {{ git = '{}' }}
                "#,
                git.url(),
            ),
        )
        .file("src/lib.rs", "")
        .build();

    p.cargo("vendor --respect-source-config")
        .run();

    p.change_file(
        ".cargo/config.toml",
        &format!(r#"
            [source."{url}"]
            git = "{url}"
            replace-with = 'vendor'

            [source.vendor]
            directory = 'vendor'
        "#,
            url = git.url(),
        ),
    );
    p.cargo("build").with_stderr("").run();
}
Click to see error output
error: failed to get `bar` as a dependency of package `foo v0.1.0 (/projects/cargo/target/tmp/cit/t0/foo)`

Caused by:
  failed to load source for dependency `bar`

Caused by:
  Unable to update file:///projects/cargo/target/tmp/cit/t0/gitworkspace#02b3fb58

Caused by:
  failed to update replaced source file:///projects/cargo/target/tmp/cit/t0/gitworkspace#02b3fb58

Caused by:
  failed to parse manifest at `/projects/cargo/target/tmp/cit/t0/foo/vendor/bar/Cargo.toml`

Caused by:
  error inheriting `version` from workspace root manifest's `workspace.package.version`

Caused by:
  failed to find a workspace root
', tests/testsuite/vendor.rs:990:38

@hunts I edited the title a bit to reflect the issue more precisely. At this moment I can't see any case other than git dependency having this issue. If we later find it's broader than that, we can always update then.

@ipetkov
Copy link

ipetkov commented Oct 9, 2022

@weihanglo can an entire workspace be copied over intact into the vendor directory? I had assumed there was a reason why cargo vendor puts each crate into its own directory (even ones coming from the same workspace)

@hunts
Copy link
Author

hunts commented Oct 10, 2022

Thanks, the updated title sounds good to me.

@weihanglo
Copy link
Member

weihanglo commented Oct 10, 2022

@ipetkov

Off the top of my head, there are at least two approach to solve this:

  1. Fill in all field having workspace = true with the actual data when copying git dependencies
  2. As you proposed, copy the whole workspace into some place in vendor directory for git dependencies to refer to.

I would say it has technical difficulties in pulling workspace into vendor directory. Firstly, a directory source at this moment is in a flat one-level layout listing all crates on the same level. By copying workspaces into vendor directory, we need to craft a special algorithm to identify the relationship between crates and workspaces. We also need to mind any name conflicts on crate names and our special folders (which I mentioned a while back here #11178 (comment)). Besides, pulling the whole workspace does contribute to size bloat. If we want to tackle it, we might need another way to slim down unnecessary members, which is also hard and might break things.

For me, I don't like the first approach, as it modifies the original Cargo.toml. However, before we figure out a better way to circumvent this, it stills sounds feasible than the latter.

@weihanglo weihanglo added the I-nominated-to-discuss To be discussed during issue triage on the next Cargo team meeting label Oct 10, 2022
@ipetkov
Copy link

ipetkov commented Oct 10, 2022

For me, I don't like the first approach, as it modifies the original Cargo.toml. However, before we figure out a better way to circumvent this, it stills sounds feasible than the latter.

Definitely agree with your sentiment! It looks like cargo will normalize the Cargo.toml by copying the workspace-inherited definitions before publishing a crate. it might be that vendoring will need to take this approach as well :/

@weihanglo weihanglo removed the I-nominated-to-discuss To be discussed during issue triage on the next Cargo team meeting label Oct 25, 2022
@piaoger
Copy link

piaoger commented Oct 26, 2022

Just clone main branch of boa ( a rust version of JavaScript) by using it as a local dependency directly and found it's not easy without tweaking. :)

Edit:

@dzmitry-lahoda
Copy link
Contributor

Cannot produce reproducible repeatable builds until fixed.

@jimblandy
Copy link

This bug is also making Firefox work difficult: https://bugzilla.mozilla.org/show_bug.cgi?id=1804530

@bors bors closed this as completed in 50eb688 Jan 19, 2023
@weihanglo
Copy link
Member

#11414 is a fix for the issue. Kudos to @Muscraft!

Reiterate the potential behavior change after that pull request.

⚠️ Potential behavior change ⚠️

There was an overlook regarding workspace inheritance interacting with git dependency when doing cargo vendor. After this PR, Cargo starts generating a normalized Cargo.toml for git dependencies when vendoring. The normalized Cargo.toml won't be identical to the original hand written one.

Relevant discussion on Zulip t-cargo

There are rooms for improvements, such as only normalizing git deps containing workspace deps, or moreover, using toml_edit to resolve workspace deps inline. However, these approaches could introduce too much complexity. During the discussion, we decide to go with the current fix, it is a reasonable tradeoff between consistency and complexity, and @Muscraft has already helped minimize the churn to only affecting vendored git dependencies.

@hunts
Copy link
Author

hunts commented Jan 24, 2023

Thank you the fix, great work!

LeSpocky pushed a commit to LeSpocky/ptxdist that referenced this issue Mar 15, 2024
To handle all downloads manually, PTXdist basically fakes vendoring all
dependencies. This can cause problems with workspaces where packages
inherit from the workspace. 'cargo vendor' used to have the same
problem[1] and fixed it by rewriting the Cargo.toml.

Do the same thing here to fix this.

[1] rust-lang/cargo#11192

Signed-off-by: Michael Olbrich <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-workspace-inheritance Area: workspace inheritance RFC 2906 A-workspaces Area: workspaces C-bug Category: bug Command-vendor
Projects
None yet
6 participants