Closed orecham closed 6 days ago
@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 ?
@elBoberido
* cargo also lists some of the commands with `--help`
This would require commands to be included in the iox2
binary i.e. not be external. If implemented, these commands would be listed under Commands:
before ... See all installed commands with --list
* it seems `iox2 services --help` prints the `iox2` help instead of the one from `iox2-services`
This is probably because the services
binary does not include a CLI that parses the arguments. When these commands are implemented to parse the CLI, it should print the help of the specific command.
Attention: Patch coverage is 0%
with 237 lines
in your changes missing coverage. Please review.
Project coverage is 78.49%. Comparing base (
ad6dcd0
) to head (ccbd452
).
@elBoberido @elfenpiff Getting a puzzling error in the mac os CI job:
error: package `clap_builder v4.5.2` cannot be built because it requires rustc 1.74 or newer, while the currently active rustc version is 1.73.0
Either upgrade to rustc 1.74 or newer, or use
cargo update -p clap_builder@4.5.2 --precise ver
where `ver` is the latest version of `clap_builder` supporting rustc 1.73.0
Error: Process completed with exit code 101.
It's puzzling because the version I specify in the Cargo.toml
is not v4.5.2
:
clap = { version = "4.4.18", features = ["derive"] }
Have you seen this before ?
@elBoberido @elfenpiff Getting a puzzling error in the mac os CI job:
error: package `clap_builder v4.5.2` cannot be built because it requires rustc 1.74 or newer, while the currently active rustc version is 1.73.0 // snip Have you seen this before ?
It's a dependency from human-panic
. You can either try an older version of human-panic
or you can add default-features = false
to the human-panic
dependency. This will turn off the colors but also removes the dependency on clap
. You could also try to convince the maintainer of anstyle
and anstream
to add a flag to not use clap
and then ask the maintainer of human-panic
to use that flag. Then continue until none of the dependencies is using clap
by default :sweat_smile:
Btw, this is the relevant output of cargo tree
├── clap v4.5.4
│ ├── clap_builder v4.5.2
│ │ ├── anstream v0.6.14
│ │ │ ├── anstyle v1.0.7
│ │ │ ├── anstyle-parse v0.2.4
│ │ │ │ └── utf8parse v0.2.1
│ │ │ ├── anstyle-query v1.0.3
│ │ │ ├── colorchoice v1.0.1
│ │ │ ├── is_terminal_polyfill v1.70.0
│ │ │ └── utf8parse v0.2.1
│ │ ├── anstyle v1.0.7
│ │ ├── clap_lex v0.7.0
│ │ └── strsim v0.11.1
│ └── clap_derive v4.5.4 (proc-macro)
│ ├── heck v0.5.0
│ ├── proc-macro2 v1.0.76
│ │ └── unicode-ident v1.0.12
│ ├── quote v1.0.35
│ │ └── proc-macro2 v1.0.76 (*)
│ └── syn v2.0.48
│ ├── proc-macro2 v1.0.76 (*)
│ ├── quote v1.0.35 (*)
│ └── unicode-ident v1.0.12
It's a dependency from
human-panic
. You can either try an older version ofhuman-panic
or you can adddefault-features = false
to thehuman-panic
dependency. This will turn off the colors but also removes the dependency onclap
. You could also try to convince the maintainer ofanstyle
andanstream
to add a flag to not useclap
and then ask the maintainer ofhuman-panic
to use that flag. Then continue until none of the dependencies is usingclap
by default 😅
@elBoberido Is it possible to increase the version of rustc
in the CI Image ?
It's a dependency from
human-panic
. You can either try an older version ofhuman-panic
or you can adddefault-features = false
to thehuman-panic
dependency. This will turn off the colors but also removes the dependency onclap
. You could also try to convince the maintainer ofanstyle
andanstream
to add a flag to not useclap
and then ask the maintainer ofhuman-panic
to use that flag. Then continue until none of the dependencies is usingclap
by default 😅@elBoberido Is it possible to increase the version of
rustc
in the CI Image ?
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.
It's a dependency from
human-panic
. You can either try an older version ofhuman-panic
or you can adddefault-features = false
to thehuman-panic
dependency. This will turn off the colors but also removes the dependency onclap
. You could also try to convince the maintainer ofanstyle
andanstream
to add a flag to not useclap
and then ask the maintainer ofhuman-panic
to use that flag. Then continue until none of the dependencies is usingclap
by default 😅@elBoberido Is it possible to increase the version of
rustc
in the CI Image ?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 ubuntu_22_04_aarch64_min_version_debug
has the same problem.
@orecham I updated all dependencies to the newest version and upgraded the minimum rust version to 1.75. PR is merged: https://github.com/eclipse-iceoryx/iceoryx2/pull/222
So you could continue from here.
@elBoberido
* cargo also lists some of the commands with `--help`
This would require commands to be included in the
iox2
binary i.e. not be external. If implemented, these commands would be listed underCommands:
before... See all installed commands with --list
Not necessarily. The --help
call could do a --list
behind the scene and either list all commands or do it similar to cargo --list
and only show the most important ones like
Commands:
introspec
processes
services
... See all commands with --list
Not for this PR but maybe something for a follow up if you like the idea.
* it seems `iox2 services --help` prints the `iox2` help instead of the one from `iox2-services`
This is probably because the
services
binary does not include a CLI that parses the arguments. When these commands are implemented to parse the CLI, it should print the help of the specific command.
If I call iox2-services --help
it prints Not implemented. Stay tuned !
. I guess the argument is not forwarded to the command.
Notes for Reviewer
iox2
iox2-
in the$PATH
or in the cargo build directories--list
- the same as howcargo
handles external binariesPATH
can be executed as if they were a defined sub command--dev
set$PATH
and in the build directoriesExample Usage:
Pre-Review Checklist for the PR Author
SPDX-License-Identifier: Apache-2.0 OR MIT
iox2-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.
Relates to #98