Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[#432] Implement CLI iox2-config #468
base: main
Are you sure you want to change the base?
[#432] Implement CLI iox2-config #468
Changes from 2 commits
2bcc183
0935fe2
a087102
ac89a38
973c574
1ba0dcc
1ec5fed
e7798a3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 function has an error on macOS:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI the bug is probably due to: #473
Not for this PR though - handling the error as described above would be sufficient.
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.
In PowerShell on Windows, these codes don't seem to be parsed correctly:
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elfenpiff Will need your feedback on this. What was your intention for this command?
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.
@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
andiceoryx2_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elfenpiff Apologies, what is your advice here ?
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.
@orecham My apologies!
Yes, this would make sense.
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.