-
Notifications
You must be signed in to change notification settings - Fork 424
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
Add public interface to lockfile interospection #2515
Add public interface to lockfile interospection #2515
Conversation
@Calsign with this patch, I believe gazelle_rust should work with an unmodified rules_rust if the following patch is applied: diff --git a/rust_parser/lockfile_crates.rs b/rust_parser/lockfile_crates.rs
index d5c455c..2163bba 100644
--- a/rust_parser/lockfile_crates.rs
+++ b/rust_parser/lockfile_crates.rs
@@ -7,20 +7,9 @@ use std::path::PathBuf;
use messages_rust_proto::Package;
pub fn get_bazel_lockfile_crates(lockfile_path: PathBuf) -> Result<Vec<Package>, Box<dyn Error>> {
- let lockfile_str = match std::fs::read_to_string(&lockfile_path) {
- Err(err) => {
- eprintln!(
- "Could not open lockfile {}: {}",
- &lockfile_path.to_str().unwrap_or("<utf-8 decode error>"),
- err,
- );
- std::process::exit(1);
- }
- read_str => read_str?,
- };
// Surprisingly, using serde_json::from_str is much faster than using serde_json::from_reader.
// See: https://github.com/serde-rs/json/issues/160
- let context: cargo_bazel::context::Context = match serde_json::from_str(&lockfile_str) {
+ let context = match cargo_bazel::lockfile::parse(&lockfile_path) {
Err(err) => {
eprintln!(
"Could not parse lockfile {}: {}",
@@ -35,7 +24,7 @@ pub fn get_bazel_lockfile_crates(lockfile_path: PathBuf) -> Result<Vec<Package>,
let mut crates = Vec::new();
let mut add_crate = |id: &_, is_proc_macro| {
- let crate_ = context.crates.get(id).expect("missing crate");
+ let crate_ = context.crate_info(id).expect("missing crate");
if let Some(library_target_name) = &crate_.library_target_name {
let mut package = Package::default();
@@ -47,25 +36,24 @@ pub fn get_bazel_lockfile_crates(lockfile_path: PathBuf) -> Result<Vec<Package>,
}
};
- for workspace_member in context.workspace_members.keys() {
+ for workspace_member in context.workspace_members() {
let workspace_crate = context
- .crates
- .get(workspace_member)
+ .crate_info(&workspace_member)
.expect("missing workspace member");
- for dep in workspace_crate.common_attrs.deps.values() {
+ for dep in workspace_crate.normal_deps().values() {
add_crate(&dep.id, false);
}
- for dep in workspace_crate.common_attrs.deps_dev.values() {
+ for dep in workspace_crate.dev_deps().values() {
add_crate(&dep.id, false);
}
- for proc_macro_dep in workspace_crate.common_attrs.proc_macro_deps.values() {
+ for proc_macro_dep in workspace_crate.proc_macro_deps().values() {
add_crate(&proc_macro_dep.id, true);
}
- for proc_macro_dep in workspace_crate.common_attrs.proc_macro_deps_dev.values() {
+ for proc_macro_dep in workspace_crate.proc_macro_dev_deps().values() {
add_crate(&proc_macro_dep.id, true);
}
}
@@ -117,7 +105,7 @@ pub fn get_cargo_lockfile_crates(lockfile_path: PathBuf) -> Result<Vec<Package>,
for dep in deps {
let mut package = Package::default();
package.name = dep.name.as_str().to_string();
- package.crate_name = cargo_bazel::utils::sanitize_module_name(&package.name);
+ package.crate_name = package.name.replace('-', "_");
package.proc_macro = is_proc_macro[dep.name.as_str()];
crates.push(package); |
3b563f5
to
9e6fbd8
Compare
Thanks! This looks great. I haven't gotten around to doing this yet so thanks for picking up the slack. Just for reference, there are additional changes which could make this situation even better:
For 1, I'll do this at some point in the future if I'm sufficiently motivated. For now it's not that big of a deal. For 2, that would need some kind of guarantee from rules_rust which the project is understandably not ready to provide. Improving the status quo to not require a patch is great and definitely good enough for now. But at some point in the future it would be nice to have a stronger guarantee. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine to me!
Had a few nits/questions but nothing urgent or worth blocking merge 😄
f9aa6be
to
4c7bade
Compare
Now that we actually have some pub items, this will help us to avoid accidentally leaking more (e.g. in bazelbuild#2515 we are leaking `CrateId`, `Select`, and `CrateDependency` to be public because they're marked `pub` not `pub(crate)`. I'm not sure I like this, but wanted send as an RFC to discuss.
https://github.com/Calsign/gazelle_rust currently relies on a patch to rules_rust which makes the `Context` struct public. Instead, supply the information they need via a public API. All names (and whether we want to hide these structs behind traits and just return `impl Trait`s) are open to discussion.
6faaa73
to
1ff8008
Compare
Now that we actually have some pub items, this will help us to avoid accidentally leaking more (e.g. in bazelbuild#2515 we are leaking `CrateId`, `Select`, and `CrateDependency` to be public because they're marked `pub` not `pub(crate)`. I'm not sure I like this, but wanted send as an RFC to discuss.
Now that we actually have some pub items, this will help us to avoid accidentally leaking more (e.g. in #2515 we are leaking `CrateId`, `Select`, and `CrateDependency` to be public because they're marked `pub` not `pub(crate)`. I'm not sure I like this, but wanted send as an RFC to discuss.
https://github.com/Calsign/gazelle_rust currently relies on a patch to rules_rust which makes the `Context` struct public. Instead, supply the information they need via a public API. All names (and whether we want to hide these structs behind traits and just return `impl Trait`s) are open to discussion. Fixes bazelbuild#1725 cc @Calsign
Now that we actually have some pub items, this will help us to avoid accidentally leaking more (e.g. in bazelbuild#2515 we are leaking `CrateId`, `Select`, and `CrateDependency` to be public because they're marked `pub` not `pub(crate)`. I'm not sure I like this, but wanted send as an RFC to discuss.
https://github.com/Calsign/gazelle_rust currently relies on a patch to rules_rust which makes the
Context
struct public.Instead, supply the information they need via a public API.
All names (and whether we want to hide these structs behind traits and just return
impl Trait
s) are open to discussion.Fixes #1725
cc @Calsign