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

Unknown extra binary listed for data-encoding-bin #9222

Closed
ia0 opened this issue Aug 5, 2024 · 9 comments
Closed

Unknown extra binary listed for data-encoding-bin #9222

ia0 opened this issue Aug 5, 2024 · 9 comments
Labels
A-backend ⚙️ C-bug 🐞 Category: unintended, undesired behavior

Comments

@ia0
Copy link

ia0 commented Aug 5, 2024

Current Behavior

I've read about #5882 in the news and wanted to check it on my binary crates:

  • For wasefire-cli it's all good. The text displays Running the above command will globally install the wasefire binary. which is correct.
  • However for data-encoding-bin it's wrong. The text displays Running the above command will globally install the data-encoding and data-encoding-bin binaries. while to my understanding there should only be a single build-target, named data-encoding.

Expected Behavior

For data-encoding-bin, the text displays Running the above command will globally install the data-encoding binary..

Steps To Reproduce

  1. Go to https://crates.io/crates/data-encoding-bin
  2. Look at the install section on the right
  3. cargo install data-encoding-bin
  4. ls ~/.cargo/bin/data-encoding* lists only data-encoding binary

Environment

I hope it's irrelevant, but Chrome on Linux.

Anything else?

No response

@Turbo87
Copy link
Member

Turbo87 commented Aug 5, 2024

hmm, from what I can see in the Cargo.toml file that indeed appears to be incorrect. I'll take a look once I'm back from vacation :)

@Turbo87 Turbo87 added C-bug 🐞 Category: unintended, undesired behavior A-backend ⚙️ labels Aug 5, 2024
@ia0
Copy link
Author

ia0 commented Aug 5, 2024

Thanks for the quick feedback! No rush :-) enjoy your vacation!

@eth3lbert
Copy link
Contributor

I think the culprit is the following:

https://github.com/LukeMathWalker/cargo-manifest/blob/00bef139e1a40b468bfb4ef5965d4eb488a8da1e/src/lib.rs#L338-L347

            let has_main_rs = src.contains("main.rs");
            if has_main_rs {
                let target = DiscoveredTarget {
                    name: package.name.clone(),
                    path: "src/main.rs".to_string(),
                };
                discovered_targets.push(target);
            }

            process_discovered_targets(&mut self.bin, discovered_targets, autobins)?;

It adds a target with the package name (data-ecoding-bin in this case), but there's already a [[bin]] with the name data-encoding declared in the manifest1 that also has the same path src/main.rs. I think it should only add new DiscoveredTarget if there are no declared bin with the same path.

Footnotes

  1. manifest https://docs.rs/crate/data-encoding-bin/latest/source/Cargo.toml

@ia0
Copy link
Author

ia0 commented Aug 5, 2024

Maybe that's enough to fix the issue, but it looks to me that the situation is more complicated than that. To the best of my understanding, the wasefire-cli scenario and the data-encoding-bin scenario are very similar. They only expose a single build-target with a different name than the crate name and with src/main.rs as their path. However, one is working correctly (a single build-target shown in crates.io) and one is not (2 build-targets showing up). The only difference I can think of (given I've read there was some form of a backfill), is that data-encoding-bin was published 3 months ago while wasefire-cli was only published 13 days ago. Thus data-encoding-bin might have been through the "backfill" path, while wasefire-cli went through the "new" path. Is the code snippet you showed for both paths or only one (logically the "backfill" one)?

@eth3lbert
Copy link
Contributor

Maybe that's enough to fix the issue, but it looks to me that the situation is more complicated than that. To the best of my understanding, the wasefire-cli scenario and the data-encoding-bin scenario are very similar. They only expose a single build-target with a different name than the crate name and with src/main.rs as their path. However, one is working correctly (a single build-target shown in crates.io) and one is not (2 build-targets showing up). The only difference I can think of (given I've read there was some form of a backfill), is that data-encoding-bin was published 3 months ago while wasefire-cli was only published 13 days ago. Thus data-encoding-bin might have been through the "backfill" path, while wasefire-cli went through the "new" path. Is the code snippet you showed for both paths or only one (logically the "backfill" one)?

Oh, there's autobins = false in the manifest of wasefire-cli, which is different from data-encoding-bin. And if autobins is false, according to https://github.com/LukeMathWalker/cargo-manifest/blob/00bef139e1a40b468bfb4ef5965d4eb488a8da1e/src/lib.rs#L455, wasefire-cli won't be added.

@ia0
Copy link
Author

ia0 commented Aug 5, 2024

Thanks for the debugging! I think it makes sense now. For wasefire-cli I used a version of cargo that had rust-lang/cargo#13713, while I couldn't have for data-encoding-bin since that PR was merged after the publication of that crate.

@Turbo87
Copy link
Member

Turbo87 commented Aug 19, 2024

LukeMathWalker/cargo-manifest#51 should hopefully fix the issue

@Turbo87
Copy link
Member

Turbo87 commented Aug 22, 2024

fix is deployed and I've rerun the backfilling script for crate versions with more than one bin (about 47k versions). I think we're done here :)

let me know if I missed anything!

@Turbo87 Turbo87 closed this as completed Aug 22, 2024
@ia0
Copy link
Author

ia0 commented Aug 22, 2024

Looks good to me, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-bug 🐞 Category: unintended, undesired behavior
Projects
None yet
Development

No branches or pull requests

3 participants