Add command to audit dependencies #236
Conversation
|
I tested it on Cheshire, it works for me. Only slight gripe is that conflicts are reported if a remote exists with and without |
mrogenmoserTT
force-pushed
the
audit
branch
2 times, most recently
from
6140185 to
c4bed3d
Compare
fischeti
left a comment
fischeti
left a comment
There was a problem hiding this comment.
Very nice feature! I tried it out in picobello, and it seems to work as intended apart from few examples (see below):
I think the actual output could be a bit improved since it is not immediately clear how to interpret the output and what action to take. To give some examples:
Conflict: tech_cells_generic -> check parents
Up-to-date: tech_cells_generic @ 0.2.13
here it says there is a conflict (which sounds bad), but then on the next line says it's up to date.
Conflict: hwpe-ctrl -> check parents
Auto-update: hwpe-ctrl 2.0.0 -> 2.1.0
Conflict: riscv-dbg -> check parents
Update: riscv-dbg 0.8.1 -> 0.9.0
Those are somehow conflicting, no?
Auto-update: cluster_icache 0.2.0 -> 0.3.1
Shouldn't this be a breaking version?
I think the improve we could improve the command by writting out the cause and action e.g.:
Audit: audited `axi_stream` -> @ 0.1.1 is up-to-date
Audit: audited `clic` -> consider *upgrading* 2.0.0 to 3.0.0
Audit: idma *failed* -> run `bender cargo parents idma` to check versions
- cause: `idma` uses hash instead of version -> consider switching to versioned release
Audit: audited cluster_icache -> *auto-upgrade* 0.2.0 -> 0.3.1 with `bender update cluster_icache`
This is more of a nice to have. We can also merge it like this, and then maybe later I can lay my hands on it🤓
I also have some minor code suggestions below
| io: &SessionIo, | ||
| dep: &str, | ||
| ) -> Result<IndexMap<String, Vec<String>>> { | ||
| let mut map = IndexMap::<String, Vec<String>>::new(); |
There was a problem hiding this comment.
Nitpick: You could add (String, String) instead of Vec<String> as values into the IndexMap since it does not need to be a dynamic data type (at least if I understand it correctly, it always inserts <version>, and <url_or_path>)
There was a problem hiding this comment.
This is to align with #235, where the parents command can return all targets passed to it by specific parents. In an internal branch where the two features are merged, this function provides both functionalities, requiring a vector as a return value.
src/cmd/audit.rs
Outdated
| let current_version_unwrapped = if current_version.is_some() { | ||
| format!("{}", current_version.clone().unwrap()) | ||
| } else { | ||
| "".to_string() | ||
| }; |
There was a problem hiding this comment.
| let current_version_unwrapped = if current_version.is_some() { | |
| format!("{}", current_version.clone().unwrap()) | |
| } else { | |
| "".to_string() | |
| }; | |
| let current_version_unwrapped = current_version.map(|v| v.to_string()).unwrap_or_default(); |
src/cmd/audit.rs
Outdated
| let current_revision_unwrapped = if current_revision.is_some() { | ||
| current_revision.clone().unwrap().to_string() | ||
| } else { | ||
| "".to_string() | ||
| }; |
There was a problem hiding this comment.
| let current_revision_unwrapped = if current_revision.is_some() { | |
| current_revision.clone().unwrap().to_string() | |
| } else { | |
| "".to_string() | |
| }; | |
| let current_revision_unwrapped = current_revision.unwrap_or_default(); |
src/cmd/audit.rs
Outdated
| let default_version = parent_array | ||
| .values() | ||
| .next() | ||
| .unwrap_or(&vec!["".to_string()])[0] | ||
| .clone(); | ||
| let url = parent_array | ||
| .values() | ||
| .next() | ||
| .unwrap_or(&vec!["".to_string(), "".to_string()])[1] | ||
| .clone(); |
There was a problem hiding this comment.
(nitpick)
| let default_version = parent_array | |
| .values() | |
| .next() | |
| .unwrap_or(&vec!["".to_string()])[0] | |
| .clone(); | |
| let url = parent_array | |
| .values() | |
| .next() | |
| .unwrap_or(&vec!["".to_string(), "".to_string()])[1] | |
| .clone(); | |
| let (default_version, url) = parent_array | |
| .values() | |
| .next() | |
| .map(|v| (v[0].clone(), v[1].clone())) | |
| .unwrap_or_else(|| ("".to_string(), "".to_string())); |
|
@paulsc96 I added a flag to ignore URL conflicts when running audit, but I think checking these conflicts and parsing for |
3 participants
No description provided.