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.

Sign up for GitHub

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

Fixing an error when the crate's name mismatches it will panic #170

Closed
wants to merge 1 commit into from
Closed

Fixing an error when the crate's name mismatches it will panic #170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions cargo-geiger/src/mapping/geiger.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,7 @@ fn handle_source_repr(source_repr: &str) -> cargo_geiger_serde::Source {
rev: String::from(revision),
}
}
_ => {
panic!("Unrecognised source type: {}", source_type)
}
_ => panic!("Unrecognised source type: {}", source_type),
}
}

Expand Down
3 changes: 2 additions & 1 deletion cargo-geiger/src/mapping/metadata.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ impl MatchesIgnoringSource for cargo_metadata::Dependency {
self.name
== krates
.get_package_name_from_cargo_metadata_package_id(&package_id)
.unwrap()
.unwrap_or(package_id.to_string())
//.unwrap()
&& self.req.matches(
&krates
.get_package_version_from_cargo_metadata_package_id(
Comment on lines 59 to 66
Copy link
Contributor

@anderejd anderejd Jan 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yijunyu Thanks for the fix, did you find the root cause of the problem? Why is the get_package_name_from_cargo_metadata_package_id function failing to return the package name?

@jmcconnell26 Any ideas?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, @yijunyu, could you provide the package_id that fails / a link to a repository to scan that fails?
If so, I can try and write a few more test cases to catch behaviour like this.
I would like to go back at some point, and add some further error handling to this part of the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a print statement before the line,

 59         println!("package_id: {:?}", package_id.to_string());
 60         self.name
 61             == krates
 62                 .get_package_name_from_cargo_metadata_package_id(&package_id)
 63                 .unwrap()

It turns out that the crate that caused the error is wasm-bindgen.
This can be confirmed if you run cargo-geiger with this crate at https://github.com/rustwasm/wasm-bindgen.
I was trying to debug into this, the error is likely from .cargo/registry/src/github.com-1ecc6299db9ec823/krates-0.5.0/src/lib.rs, but I am not too sure about it.
Indeed if I used the unwrap_or_default instead, it will skip the wasm-bindgen crate, which is not what we wanted.
What will be the better way to fix the error?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the info @yijunyu! Taking a look at the issue, something seems strange with the dependencies being added to the graph, but only when running with wasm-bindgen as the root of the crate. If wasm-bindgen is added as a dependency in a Cargo.toml, it looks to generate the report correctly.
I'll take a deeper look at this, and try to figure out what is causing the root problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for looking into this: we will test it again later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmcconnell26 Any news? Just curious, no stress 😃

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anderejd, apologies, I've been so busy lately I haven't had a chance to do half the things I've been meaning to! I may take a look at issue #175 first, and then come back to this one if you agree?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem :) Take your time and thanks for #178!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been trying to replicate this behaviour in a new test project, and have been stumped. I can see it happening in the wasm-bindgen crate, but attempting to create a new crate with the same error fails.
@anderejd, if its OK with you, I'd like to land a few PR's cleaning up the error handling, and hopefully that will lead me to the root of the problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sound great to me, go for it!

Expand Down