[#432] Implement CLI iox2-config #468
Conversation
brosier01
deleted the
iox2-432-CLI-displaying-full-system-configuration
branch
brosier01
restored the
iox2-432-CLI-displaying-full-system-configuration
branch
|
@brosier01 don't close it - it looked good! |
| // old | ||
| let fuu = hello().is_it_me_you_re_looking_for() | ||
| ```bash | ||
| cargo run --bin iox2-config show |
There was a problem hiding this comment.
| cargo run --bin iox2-config show | |
| iox2 config show |
| } | ||
|
|
||
| pub fn show() -> Result<()> { | ||
| print_system_configuration(); |
There was a problem hiding this comment.
Does this print the default in-built config if there is no config file being loaded from file?
There was a problem hiding this comment.
print_system_configuration() displays the whole system configuration with all limits, features and details to the console. It does not print the built-in config but the system configuration. print_system_configuration() function is independent of the config file.
There was a problem hiding this comment.
@elfenpiff Should the defaults be added to the output of iox2 config show, under a different sub-heading ? I would think yes, but what did you have in mind?
There was a problem hiding this comment.
@elfenpiff Will need your feedback on this. What was your intention for this command?
There was a problem hiding this comment.
@elfenpiff I implemented this to provide me with some debug output of the configuration of the POSIX system, meaning the POSIX configuration that states like how many semaphores can be created or what the maximum value of a semaphore could be.
Windows, Mac Os, FreeBSD, and Linux have different values here, and in the early days of iceoryx2, it was very helpful when analyzing bugs. Now, everything is abstracted away with the iceoryx2_pal and iceoryx2_cal layer but this still makes sense for mission-critical environments where we define the minimum requirements we have for the system (somewhere) and then this would print out the systems requirement and would also highlight the things which would violate our requirements.
A practical example is the iceoryx2_bb_threadsafe::trigger_queue. It uses two semaphores to trigger the producer/consumer when changes happen. But when the semaphores max value is 1024 it means the trigger queue can have a capacity of at most 1024 elements, otherwise the underlying semaphore would overflow and break the queue.
Another example is a gateway. When this subscribes to thousands of services it must be able to memory map thousands of shared memories and also hold thousands of fd's. By default the max number of fd's is 1024 so here it may makes sense to print a warning on gateway startup when the config does not fit.
There was a problem hiding this comment.
@elfenpiff Apologies, what is your advice here ?
There was a problem hiding this comment.
@orecham My apologies!
Should the defaults be added to the output of iox2 config show, under a different sub-heading ?
Yes, this would make sense.
There was a problem hiding this comment.
@elfenpiff My apologies, did you say I had to set up a new subtitle? Like “iox2 config show default” which shows the default configuration? Am I right ?
There was a problem hiding this comment.
@brosier01 Yeah, if you could additionally print the contents of the config file under another subtitle, this would be perfect.
Let us know if you hit problems or it turns out more difficult than it seems.
orecham
changed the title
brosier01
force-pushed
the
iox2-432-CLI-displaying-full-system-configuration
branch
from
ff8654f
to
973c574
Compare
| } | ||
|
|
||
| pub fn generate() -> Result<()> { | ||
| let config_dir = dirs::config_dir().unwrap().join("iceoryx2/"); |
There was a problem hiding this comment.
This results in a directory C:\Users\orecham\AppData\Roaming\iceoryx2/config.toml on Windows (the last slash should be a backslash.) I think you can just remove the / here and rely on .join() adding the proper separators.
There was a problem hiding this comment.
On a side note. Windows also supports the forward slash as path separator and if you need to pass a path as string I would recommend to use C:/Windows/foo or you might end up with something like C:\\\\Windows\\\\foo, depending on where the string is defined and how many layers it needs to pass until it reaches the actual Windows call.
|
|
||
| /// Prints the whole system configuration with all limits, features and details to the console. | ||
| pub fn print_system_configuration() { | ||
| const HEADER_COLOR: &str = "\x1b[4;92m"; |
There was a problem hiding this comment.
In PowerShell on Windows, these codes don't seem to be parsed correctly:
PS C:\Users\orecham> iox2 config show
←[4;92mposix system configuration←[0m
←[4;92msystem info←[0m
←[0;37mPosixVersion ←[0m ←[0;94m200809←[0m
←[0;37mPageSize ←[0m ←[0;94m4096←[0m
←[0;37mNumberOfClockTicksPerSecond ←[0m ←[0;94m0←[0m
←[0;37mNumberOfCpuCores ←[0m ←[0;94m16←[0m
←[4;92mlimits←[0m
←[0;37mMaxLengthOfHostname ←[0m ←[0;94m[ unlimited ]←[0m
←[0;37mMaxLengthOfLoginName ←[0m ←[0;94m[ unlimited ]←[0m
←[0;37mMaxNumberOfSupplementaryGroupIds ←[0m ←[0;94m[ unlimited ]←[0m
←[0;37mMaxNumberOfOpenFiles ←[0m ←[0;94m[ unlimited ]←[0m
←[0;37mMaxNumberOfSimultaneousProcessesPerUser ←[0m ←[0;94m[ unlimited ]←[0m
←[0;37mMaxLengthOfArguments ←[0m ←[0;94m[ unlimited ]←[0m
←[0;37mMaxNumberOfOpenStreams ←[0m ←[0;94m[ unlimited ]←[0m
←[0;37mMaxNumberOfSymbolicLinks ←[0m ←[0;94m[ unlimited ]←[0m
←[0;37mMaxLengthOfTerminalDeviceName ←[0m ←[0;94m[ unlimited ]←[0m
←[0;37mMaxNumberOfBytesInATimezone ←[0m ←[0;94m[ unlimited ]←[0m
←[0;37mMaxNumberOfSemaphores ←[0m ←[0;94m[ unlimited ]←[0m
←[0;37mMaxSemaphoreValue ←[0m ←[0;94m2147483646←[0m
←[0;37mMaxNumberOfOpenMessageQueues ←[0m ←[0;94m[ unlimited ]←[0m
←[0;37mMaxMessageQueuePriority ←[0m ←[0;94m[ unlimited ]←[0m
←[0;37mMaxNumberOfThreads ←[0m ←[0;94m1024←[0m
←[0;37mMaxSizeOfPasswordBuffer ←[0m ←[0;94m[ unlimited ]←[0m
←[0;37mMaxSizeOfGroupBuffer ←[0m ←[0;94m[ unlimited ]←[0m
←[0;37mMaxPathLength ←[0m ←[0;94m260←[0m
←[0;37mMaxFileNameLength ←[0m ←[0;94m[ unlimited ]←[0m
←[0;37mMaxUnixDomainSocketNameLength ←[0m ←[0;94m108←[0m
←[0;37mMinStackSizeOfThread ←[0m ←[0;94m1048576←[0m
←[4;92moptions←[0m
←[0;90mAdvisoryInfo ←[0m ←[0;90mfalse←[0m
←[0;90mCpuTime ←[0m ←[0;90mfalse←[0m
←[0;90mFsync ←[0m ←[0;90mfalse←[0m
←[0;90mIpv6 ←[0m ←[0;90mfalse←[0m
←[0;90mMemLock ←[0m ←[0;90mfalse←[0m
←[0;90mMemLockRange ←[0m ←[0;90mfalse←[0m
←[0;90mMessagePassing ←[0m ←[0;90mfalse←[0m
←[0;90mPrioritizedIo ←[0m ←[0;90mfalse←[0m
←[0;90mPriorityScheduling ←[0m ←[0;90mfalse←[0m
←[0;90mRegularExpressions ←[0m ←[0;90mfalse←[0m
←[0;90mSpawn ←[0m ←[0;90mfalse←[0m
←[0;90mProcessSporadicServer ←[0m ←[0;90mfalse←[0m
←[0;90mSynchronizedIo ←[0m ←[0;90mfalse←[0m
←[0;90mThreadStackAddress ←[0m ←[0;90mfalse←[0m
←[0;90mThreadStackSize ←[0m ←[0;90mfalse←[0m
←[0;90mThreadCpuTimeClock ←[0m ←[0;90mfalse←[0m
←[0;90mThreadExecutionScheduling ←[0m ←[0;90mfalse←[0m
←[0;90mThreadProcessSharedSynchronization ←[0m ←[0;90mfalse←[0m
←[0;90mMutexPriorityInheritance ←[0m ←[0;90mfalse←[0m
←[0;90mMutexPriorityProtection ←[0m ←[0;90mfalse←[0m
←[0;90mRobustMutexPriorityInhertiance ←[0m ←[0;90mfalse←[0m
←[0;90mRobustMutexPriorityProtection ←[0m ←[0;90mfalse←[0m
←[0;90mThreadSporadicServer ←[0m ←[0;90mfalse←[0m
←[0;90mTrace ←[0m ←[0;90mfalse←[0m
←[0;90mTraceEventFilter ←[0m ←[0;90mfalse←[0m
←[0;90mTraceInherit ←[0m ←[0;90mfalse←[0m
←[0;90mTraceLog ←[0m ←[0;90mfalse←[0m
←[0;90mTypedMemoryObjects ←[0m ←[0;90mfalse←[0m
←[4;92mfeatures←[0m
←[0;37mBarriers ←[0m ←[0;94mtrue←[0m
←[0;90mAsynchronousIo ←[0m ←[0;90mfalse←[0m
←[0;90mClockSelection ←[0m ←[0;90mfalse←[0m
←[0;90mJobControl ←[0m ←[0;90mfalse←[0m
←[0;37mMappedFiles ←[0m ←[0;94mtrue←[0m
←[0;90mMemoryProtection ←[0m ←[0;90mfalse←[0m
←[0;90mMonotonicClock ←[0m ←[0;90mfalse←[0m
←[0;90mRawSockets ←[0m ←[0;90mfalse←[0m
←[0;37mReaderWriterLocks ←[0m ←[0;94mtrue←[0m
←[0;90mRealtimeSignals ←[0m ←[0;90mfalse←[0m
←[0;90mSavedUserAndGroupIds ←[0m ←[0;90mfalse←[0m
←[0;37mSemaphores ←[0m ←[0;94mtrue←[0m
←[0;37mSharedMemoryObjects ←[0m ←[0;94mtrue←[0m
←[0;90mShell ←[0m ←[0;90mfalse←[0m
←[0;37mSpinLocks ←[0m ←[0;94mtrue←[0m
←[0;37mThreadSafeFunctions ←[0m ←[0;94mtrue←[0m
←[0;90mThreads ←[0m ←[0;90mfalse←[0m
←[0;37mTimeouts ←[0m ←[0;94mtrue←[0m
←[0;37mTimers ←[0m ←[0;94mtrue←[0m
←[0;90mOpenRealtimeOptionGroup ←[0m ←[0;90mfalse←[0m
←[0;90mOpenRealtimeThreadsOptionGroup ←[0m ←[0;90mfalse←[0m
←[4;92mprocess resource limits←[0m
←[0;37mMaxCoreFileSize ←[0m soft: ←[0;94m0 ←[0m hard: ←[0;94m0←[0m
←[0;37mMaxConsumableCpuTime ←[0m soft: ←[0;94m0 ←[0m hard: ←[0;94m0←[0m
←[0;37mMaxDataSegmentSize ←[0m soft: ←[0;94m0 ←[0m hard: ←[0;94m0←[0m
←[0;37mMaxFileSize ←[0m soft: ←[0;94m0 ←[0m hard: ←[0;94m0←[0m
←[0;37mMaxNumberOfOpenFileDescriptors ←[0m soft: ←[0;94m0 ←[0m hard: ←[0;94m0←[0m
←[0;37mMaxStackSize ←[0m soft: ←[0;94m0 ←[0m hard: ←[0;94m0←[0m
←[0;37mMaxSizeOfTotalMemory ←[0m soft: ←[0;94m0 ←[0m hard: ←[0;94m0←[0m
A crate like colored, like used in the rest of the CLI, should be used for cross-platform coloring.
I know this is not your code, but this would need to be updated for UX purposes on Windows.
| use std::io::Write; | ||
|
|
||
| /// Prints the whole system configuration with all limits, features and details to the console. | ||
| pub fn print_system_configuration() { |
There was a problem hiding this comment.
This function has an error on macOS:
0 [F] "ProcessResourceLimit::soft_limit"
| This should never happen! Unable to acquire soft limit for MaxSizeOfTotalMemory due to an unknown error (errno { name = "EINVAL", value = 22, details = "Invalid argument" }).
We should make it possible for the CI to catch stuff like this. Could you add a test that executes this function?
As for the reason, it's not immediately obvious to me. For this PR, one workaround could be to update the code to catch errors from the calls it uses to get the information it prints. The function could then continue even if a call has an error, and print a message to the user explaining which information was not retrievable.
cc @elfenpiff
There was a problem hiding this comment.
FYI the bug is probably due to: #473
Not for this PR though - handling the error as described above would be sufficient.
iceoryx2/src/config.rs
Outdated
| &FilePath::new_unchecked(DEFAULT_CONFIG_FILE) | ||
| &FilePath::new_unchecked( | ||
| dirs::config_dir() | ||
| .unwrap() |
There was a problem hiding this comment.
Won't this panic if the directory doesn't exist. The None case should be handled explicitly.
iceoryx2/src/config.rs
Outdated
| @@ -69,22 +69,20 @@ | |||
| //! # } | |||
| //! ``` | |||
|
|
|||
| use dirs; | |||
There was a problem hiding this comment.
Rather than using this directly, an abstraction should be implemented.
This allows for swapping out the implementation and will make it easier to isolate for safety certification purposes. I haven't looked at the implementation of dirs closely, but it might not be suitable for certification e.g. if it uses heap.
The cleanest way would probably be to create a "concept" in the concept abstraction layer (iceoryx2-cal) for configuration storage, then add one implementation that uses dirs.
This will allow for:
- Swapping out an implementation to a safety-certifiable version
- Implementation of the concept for arbitrary environments that don't have a file system
| } | ||
|
|
||
| pub fn show() -> Result<()> { | ||
| print_system_configuration(); |
There was a problem hiding this comment.
@elfenpiff Will need your feedback on this. What was your intention for this command?
iceoryx2/src/config.rs
Outdated
| use iceoryx2_bb_container::semantic_string::SemanticString; | ||
| use iceoryx2_bb_elementary::lazy_singleton::*; | ||
| use iceoryx2_bb_posix::{file::FileBuilder, shared_memory::AccessMode}; | ||
| use iceoryx2_bb_system_types::file_name::FileName; | ||
| use iceoryx2_bb_system_types::file_path::FilePath; | ||
| use iceoryx2_bb_system_types::path::Path; | ||
| use serde::{Deserialize, Serialize}; | ||
| use std::time::Duration; | ||
| use std::{os::unix::ffi::OsStrExt, time::Duration}; |
There was a problem hiding this comment.
This will not work on Windows (unix-only).
There was a problem hiding this comment.
Hi @orecham I fixed my code by adding a new crate abastraction layer to get the default directory of the configuration file. And I've added a new sub-command: now I can display the system configuration and show the current iceoryx2 configuration.
brosier01
force-pushed
the
iox2-432-CLI-displaying-full-system-configuration
branch
from
e7798a3
to
037781e
Compare
|
Hi @orecham @elfenpiff, is there any way to run CI locally? |
brosier01
force-pushed
the
iox2-432-CLI-displaying-full-system-configuration
branch
from
037781e
to
82f5387
Compare
|
@brosier01 Yes there is. You can use the When you entered it you can run the usual cargo commands. Under |
brosier01
force-pushed
the
iox2-432-CLI-displaying-full-system-configuration
branch
from
82f5387
to
08a4135
Compare
Co-authored-by: orecham <[email protected]>
Co-authored-by: orecham <[email protected]>
…rm coloring + implementation of user confirmation required before deleting current configuration file
…file with the standard configuration path
…the configuration file + add a subcommand to display the contents of the configuration file
brosier01
force-pushed
the
iox2-432-CLI-displaying-full-system-configuration
branch
from
08a4135
to
41d20b8
Compare
|
Hi @orecham, I think I've implemented all the elements described in the original problem. Do you see any other features I need to work on ? |
@brosier01 I will be able to review sometime today or tomorrow, apologies for the delay. |
Notes for Reviewer
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)Tests follow the best practice for testingtask-list-completed)Checklist for the PR Reviewer
Unit tests have been written for new behaviorPost-review Checklist for the PR Author
References
Relates to #432