non-blocking build error reported in example code since 0d62ae2

[ckoehler]

Problem

We're seeing the following error during a cargo build:

error: invalid character `{` in package name: `{{package_name}}`, the first character must be a Unicode XID start character (most letters or `_`)
 --> ../../.cargo/git/checkouts/my-crate-e3ad1ac7415c84bc/3314343/tools/generator/thing/thing/{{thing_name}}/Cargo.toml:2:8
  |
2 | name = "{{package_name}}"
  |        ^^^^^^^^^^^^^^

This started with Rust 1.77, and I bisected it to commit 0d62ae2. That one looks to really just show errors differently, so I don't know if it caused our error or just surfaced it. The build is not affected, because it's just the cargo-generator code for some of our stuff. That path not in any of our workspace manifests, I think cargo just finds it by walking the source files of our crate and finds the templated Cargo.toml.

Steps

No response

Possible Solution(s)

We can move the templated stuff out of our repo and somewhere else, but since it's a change in behavior, I wanted to flag it.

Notes

Happy to consider any other workarounds to make cargo ignore that part of our source. Thanks!

Version

cargo 1.77.1 (e52e36006 2024-03-26)
release: 1.77.1
commit-hash: e52e360061cacbbeac79f7f1215a7a90b6f08442
commit-date: 2024-03-26
host: aarch64-apple-darwin
libgit2: 1.7.2 (sys:0.18.2 vendored)
libcurl: 8.4.0 (sys:0.4.70+curl-8.5.0 system ssl:(SecureTransport) LibreSSL/3.3.6)
ssl: OpenSSL 1.1.1w  11 Sep 2023
os: Mac OS 14.2.1 [64-bit]

[Muscraft]

Can you share the repo where this happened or create an example? It would help in debugging what is going on.

[ckoehler]

Sorry, unfortunately it's a private corp repo :/ I may be able to do an example, tho I am not quite sure what all causes this...
Give me a little bit and I'll see what I can do.

[ehuss]

@Muscraft Here is a test demonstrating it:

#[cargo_test]
fn invalid_name_in_git_repo() {
    // Checks to ignore invalid package name in git repository.
    let git_project = git::new("bar", |p| {
        p.file("Cargo.toml", &basic_manifest("bar", "1.0.0"))
            .file("src/lib.rs", "")
            .file(
                "dont_look_at_me/Cargo.toml",
                r#"
                    [package]
                    name = "{{project_name}}"
                    version = "1.0.0"
                    edition = "2021"
                "#,
            )
            .file("dont_look_at_me/src/lib.rs", "")
    });
    let p = project()
        .file(
            "Cargo.toml",
            &format!(
                r#"
                    [package]
                    name = "foo"
                    version = "1.0.0"
                    edition = "2021"

                    [dependencies]
                    bar = {{ git = "{}" }}
                "#,
                git_project.url()
            ),
        )
        .file("src/lib.rs", "")
        .build();
    p.cargo("fetch")
        .with_stderr(
            "\
[UPDATING] git repository `[..]`
error: invalid character `{` in package name: `{{project_name}}`, \
the first character must be a Unicode XID start character (most letters or `_`)
 --> [..]/dont_look_at_me/Cargo.toml:3:28
  |
3 |                     name = \"{{project_name}}\"
  |                            ^^^^^^^^^^^^^^^^^^
  |
[LOCKING] 2 packages
",
        )
        .run();
}

I would generally expect it to ignore packages it cannot parse, unless it is unable to find the requested package. Errors are supposed to be deferred here and not shown unless it fails. Perhaps the new diagnostics aren't getting accumulated?

[ckoehler]

@Muscraft Here is a test demonstrating it:

Thanks so much, that's better and faster than what I would've done!

[epage]

So from what I understand of this situation is that this is superficial though misleading in a very negative way.

As for the change. We switched from reporting the error exclusively through Result to printing the error immediately and then using a special error to say that we already reported it. This means that when we try to capture and ignore the error, we don't fail but we still print.

In terms of fixes.

• We could switch things so our new error report is done through Result, like before.
  • We might run into conflicts between annotate-snippets and cargo both wanting to report error:
  • This will likely make error recovery (ie reporting multiple errors, not just the first) more difficult
• We could use cargo-util-schema or just toml to pre-parse the data to see if its a manifest we care about
  • I'm assuming this would require other changes as I asume we gather everything and then figure out what we care about
  • Likely missing some cases where this would cause us problems since its been a bit since I've delved into this machinery
• We could pass in a trait object for reporting diagnostics and change out the implementation in this case
• Write to buffers and replay the result

[epage]

We could use cargo-util-schema or just toml to pre-parse the data to see if its a manifest we care about

Looking a bit more into this. Doing this would likely also fix:

• [diagnostics] Errors for failure to parse a manifest in a git dependency are lost #6211 (for certain classes of errors)
• warning: skipping duplicate package case found at ~\.cargo\git\checkouts\cargo-.. #10752 which blocks turning that warning into an error (see also Next step for deprecations in Cargo #13629)

The idea would be that we walk the filesystem, finding Cargo.tomls and indexing them by package.name. When querying a git source, we would take the name from the Dependency, look up the item (warning on duplicate) and then do a full load, caching it.

The biggest problem is with cargo install --git. In that case, select_pkg loads all packages to discover bins, much like we do with workspaces (#4830 would expand this further).

What if we changed read_packages to stop walking on a Cargo.toml and instead do the normal workspace loading logic?

[epage]

One way to reduce the sympton is to reduce how much we walk in git sources.

In #10752 (comment) we talked about a breadcrumb to switch our walking from recursive directory walking to workspace loading. This would reduce the chance of seeing duplicate package name warnings and reduce the parse errors reported because most of those are likely coming from test or example cases underneath a workspace.

I wonder if we could switch to this model without requiring a breadcrumb...