-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Respect -proto_dist flag when specified multiple times #8672
Conversation
CT Test Results 2 files 70 suites 1h 2m 17s ⏱️ Results for commit 4de3cc9. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts// Erlang/OTP Github Action Bot |
2bd9c31
to
c015d1c
Compare
Thanks for the PR! Can you log a warning when the option is specified multiple times (c.f. #2227)? |
|
||
proto_dist_argument() -> | ||
case init:get_argument(proto_dist) of | ||
{ok, [Protos | Rest]} -> |
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.
Sidenote: similar functions above do not have that warning.
A follow up question: is there a reason we process proto_dist
as a list, instead of getting the first item? In other words, is there a case where the user would pass -proto_dist [proto1, proto2]
. The docs say -proto_dist Proto
, indicating a single value is expected.
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.
Indeed, not all functions have that warning, nor do many other places elsewhere in the codebase that uses init:get_argument/1
(most seem to either ignore the flag or grab the first argument, both of them silently). It's rather inconsistent unfortunately, but there's nothing wrong with fixing it as we go.
As for processing as a list the intent seems to be supporting multiple protocols at the same time, but as you noted we haven't documented that anywhere so it's not supported.
[] -> | ||
ok; | ||
_ -> | ||
?LOG_WARNING("Multiple -proto_dist given to erl, using the first, ~p", [Protos]) |
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.
Note: the warning shows up twice, since we read the value in two places.
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.
I think that's fine. :-)
Hi again, the tests ran fine and I'm ready to merge this as soon as the commits are squashed :-) |
a642a60
to
4de3cc9
Compare
@jhogberg rebased and squashed! |
Merged, thanks again for the PR! |
Currently specifying multiple instances of
-proto_dist
ignores the flag altogether.