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

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

Open
ckoehler opened this issue Apr 8, 2024 · 8 comments
Open

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

ckoehler opened this issue Apr 8, 2024 · 8 comments
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. A-manifest Area: Cargo.toml issues C-bug Category: bug regression-from-stable-to-stable Regression in stable that worked in a previous stable release. S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.

Comments

@ckoehler
Copy link

ckoehler commented Apr 8, 2024

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]
@ckoehler ckoehler added C-bug Category: bug S-triage Status: This issue is waiting on initial triage. labels Apr 8, 2024
@Muscraft
Copy link
Member

Muscraft commented Apr 8, 2024

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

@ckoehler
Copy link
Author

ckoehler commented Apr 8, 2024

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
Copy link
Contributor

ehuss commented Apr 8, 2024

@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
Copy link
Author

ckoehler commented Apr 8, 2024

@Muscraft Here is a test demonstrating it:

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

@epage
Copy link
Contributor

epage commented Apr 9, 2024

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 epage added A-diagnostics Area: Error and warning messages generated by Cargo itself. A-manifest Area: Cargo.toml issues regression-from-stable-to-stable Regression in stable that worked in a previous stable release. labels Apr 9, 2024
@epage
Copy link
Contributor

epage commented Apr 10, 2024

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:

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 epage changed the title build error in example code since 0d62ae2 non-blocking build error reported in example code since 0d62ae2 Apr 10, 2024
@epage
Copy link
Contributor

epage commented Apr 10, 2024

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...

bors added a commit that referenced this issue Jul 15, 2024
fix(source): Don't warn about unreferenced duplicate packages

### What does this PR try to resolve?

This also improves the message, consolidating multiple duplicates and saying which was loaded instead, as it naturally fell out of the design

Fixes #10752

### How should we test and review this PR?

### Additional information

We're still subject to #13724 and fully load every manifest, even if we don't use it.  I'm exploring that topic at https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/Redundant.20code.20in.20.60GitSouce.60.3F/near/450783427

This change builds on
- #13993
- #14169
- #14231
- #14234
@epage
Copy link
Contributor

epage commented Oct 7, 2024

#14395 is tracking the performance optimization that would reduce the chance of seeing an error.

@weihanglo weihanglo added S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted. and removed S-triage Status: This issue is waiting on initial triage. labels Nov 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Error and warning messages generated by Cargo itself. A-manifest Area: Cargo.toml issues C-bug Category: bug regression-from-stable-to-stable Regression in stable that worked in a previous stable release. S-needs-design Status: Needs someone to work further on the design for the feature or fix. NOT YET accepted.
Projects
None yet
Development

No branches or pull requests

5 participants