Add entrypoint for iox2 cli #106
Conversation
|
@elBoberido @elfenpiff I am not quite finished, but could I get a preliminary review of the code to check if it is following rust conventions and idioms ? |
iceoryx2-cli/iox2/src/cli.rs
Outdated
| pub external_command: Vec<String>, | ||
| } | ||
|
|
||
| fn help_template() -> &'static str { |
There was a problem hiding this comment.
@orecham I thought clap is generating the whole help automatically. Was it not compatible with commands?
There was a problem hiding this comment.
@elfenpiff The problem is that clap cannot dynamically generate the help, which is required if the available commands are to be determined by the binaries that are present.
orecham
force-pushed
the
iox2-98-entrypoint-for-iox2-cli
branch
from
7e72ce3
to
38ebaf2
Compare
This would require commands to be included in the
This is probably because the |
orecham
force-pushed
the
iox2-98-entrypoint-for-iox2-cli
branch
3 times, most recently
from
ef76b75
to
1ac970b
Compare
orecham
force-pushed
the
iox2-98-entrypoint-for-iox2-cli
branch
from
1ac970b
to
5e04e86
Compare
orecham
force-pushed
the
iox2-98-entrypoint-for-iox2-cli
branch
from
5e04e86
to
f21d72b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #106 +/- ##
==========================================
- Coverage 80.02% 79.16% -0.87%
==========================================
Files 183 192 +9
Lines 20423 20648 +225
==========================================
+ Hits 16343 16345 +2
- Misses 4080 4303 +223
|
|
@elBoberido @elfenpiff Getting a puzzling error in the mac os CI job: It's puzzling because the version I specify in the Have you seen this before ? |
It's a dependency from Btw, this is the relevant output of |
@elBoberido Is it possible to increase the version of |
orecham
force-pushed
the
iox2-98-entrypoint-for-iox2-cli
branch
from
9bfebdd
to
e158d01
Compare
Yes, this is not a problem! I could create a separate PR and increase it to rust version 1.74 - but we can also go higher. |
@elfenpiff Any version 1.74 or above will make things easier for this PR. You can choose what you like. Or just let me know how to do it and I can do the PR. Note that |
Not necessarily. The Commands:
introspec
processes
services
... See all commands with --listNot for this PR but maybe something for a follow up if you like the idea.
If I call |
| if is_valid_command_binary(&path) { | ||
| path.file_name() | ||
| .and_then(|n| n.to_str()) | ||
| .map(|command_name| CommandInfo { | ||
| name: command_name.to_string(), | ||
| path: path.clone(), | ||
| is_development: false, | ||
| }) | ||
| } else { | ||
| None | ||
| } |
There was a problem hiding this comment.
This duplication leads to errors. The one from the development binaries strips the iox2- prefix but this does not. Can the two implementations be combined?
| let development_commands = find_development_command_binaries(); | ||
| let installed_commands = find_installed_command_binaries(); | ||
|
|
||
| let mut all_commands = development_commands; | ||
| all_commands.extend(installed_commands.iter().cloned()); | ||
| all_commands |
There was a problem hiding this comment.
The question is what to prioritize, the development versions or the installed versions
Personally, I would prefer this
Installed Commands:
introspect
introspect (dev)
processes (dev)
pub
pub (dev)
rpc (dev)
services (dev)
sub (dev)
over this
introspect (dev)
introspect
processes (dev)
pub (dev)
pub
rpc (dev)
services (dev)
sub (dev)
But not so much to have a lengthy discussion about this 😅
The main reason would be to use the installed foo with iox2 foo. Currently it does not work when foo and foo (dev) is found. When foo (dev) is listed as second entry, one could assume that iox2 foo would be called and iox2 --dev foo would call foo (dev). What do you think?
| pub fn list() { | ||
| println!("{}", "Installed Commands:".bright_green().bold()); | ||
| let mut installed_commands = find(); | ||
| installed_commands.sort_by_key(|command| command.name.clone()); |
There was a problem hiding this comment.
| installed_commands.sort_by_key(|command| command.name.clone()); | |
| installed_commands.sort_by_cached_key(|command| command.name.clone()); |
This is probably be faster
| " {} {}", | ||
| command.name.bold(), | ||
| if command.is_development { | ||
| "(dev) ".italic() |
There was a problem hiding this comment.
| "(dev) ".italic() | |
| "(dev)".italic() |
| // SPDX-License-Identifier: Apache-2.0 OR MIT | ||
|
|
||
| fn main() { | ||
| println!("Not implemented. Stay tuned !"); |
There was a problem hiding this comment.
| println!("Not implemented. Stay tuned !"); | |
| println!("Not implemented. Stay tuned!"); |
Same in the other files
| struct CommandInfo { | ||
| name: String, | ||
| path: PathBuf, | ||
| is_development: bool, |
There was a problem hiding this comment.
Food for thought. What do you think of using something like this instead of is_development
#[derive(Clone, Debug, PartialEq)]
enum BinaryType {
Installed,
Development,
}
impl BinaryType {
fn suffix(&self) -> &'static str {
match self {
Self::Installed => "",
Self::Development => "(dev)",
}
}
}Depending on how much code you want to share for finding the iox2 commands, this might make it more readable than passing true and false.
| path.file_name() | ||
| .and_then(|n| n.to_str()) | ||
| .map(|command_name| { | ||
| let stripped = command_name.strip_prefix("iox2-").unwrap_or(command_name); |
There was a problem hiding this comment.
| let stripped = command_name.strip_prefix("iox2-").unwrap_or(command_name); | |
| let stripped = command_name.strip_prefix("iox2-").expect("The filter already ensured to be an iox2 command binary"); |
| .map(|entry| entry.path()) | ||
| .filter(|path_buf: &PathBuf| is_valid_command_binary(path_buf.as_path())) | ||
| .filter_map(|path| { | ||
| path.file_name() | ||
| .and_then(|n| n.to_str()) | ||
| .map(|command_name| { | ||
| let stripped = command_name.strip_prefix("iox2-").unwrap_or(command_name); | ||
| CommandInfo { | ||
| name: stripped.to_string(), | ||
| path: path.clone(), | ||
| is_development: true, | ||
| } | ||
| }) | ||
| }) | ||
| .collect() |
There was a problem hiding this comment.
In case you want to reduce the indentation even more, then this would be an option
.map(|entry| entry.path())
.filter(|path_buf: &PathBuf| is_valid_command_binary(path_buf.as_path()))
.filter_map(|path| {
path.as_path()
.file_name()
.and_then(|n| n.to_os_string().into_string().ok())
.map(|name| (path, name.strip_prefix("iox2-").expect("").to_string()))
})
.map(|(path, name)| CommandInfo {
name,
path,
is_development: true,
})
.collect()| fn is_valid_command_binary(path: &Path) -> bool { | ||
| path.is_file() | ||
| && path | ||
| .file_stem() | ||
| .unwrap() | ||
| .to_str() | ||
| .unwrap() | ||
| .starts_with("iox2-") | ||
| && path.extension().is_none() // Exclude files with extensions (e.g. '.d') | ||
| } |
There was a problem hiding this comment.
Please use either expect instead of unwrap or rewrite this to return a Option<PathBuf> and then you can just use ? or .ok()? in case the function returns a Result.
Returning an Option<PathBuf> might even simplify the filter_map above.
| if cli.list { | ||
| commands::list(); | ||
| } else if cli.paths { | ||
| commands::paths(); | ||
| } else if !cli.external_command.is_empty() { |
There was a problem hiding this comment.
Can there be a situation where none of this three branches will be taken? If yes, could you add an else branch and print something useful for the user?
Notes for Reviewer
iox2iox2-in the$PATHor in the cargo build directories--list- the same as howcargohandles external binariesPATHcan be executed as if they were a defined sub command--devset$PATHand in the build directoriesExample Usage:
Pre-Review Checklist for the PR Author
SPDX-License-Identifier: Apache-2.0 OR MITiox2-123-introduce-posix-ipc-example)[#123] Add posix ipc example)task-list-completed)Checklist for the PR Reviewer
Post-review Checklist for the PR Author
References
Use either 'Closes #123' or 'Relates to #123' to reference the corresponding issue.
Closes #ISSUE-NUMBER