-
Notifications
You must be signed in to change notification settings - Fork 647
kernel/syscall: Fix if enable Net/socket/iperf but not enable iNet/IPv4. #3219
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
base: master
Are you sure you want to change the base?
Conversation
cab76dc to
6503dfd
Compare
b9f0118 to
fbf3bfd
Compare
|
Is there any chance you can split this into multiple PRs? 9 commits is a lot. Also, can you please provide more detail about what the internal CI tests? There is an impact of this PR if you are modifying code. Please list what applications/tests are impacted by this change. |
Merge patch into one pr aim to save the CI resource. if split too much will cause more CI actions. Every commit is described in commit msg, maybe don't have to copy all from commit msg to github pr? About internal CI, we are not only one codebase from github. when we using these code for project, patch already included and tested. |
|
@jasonbu You can't assume that if something works in Xiaomi internal repo, it will also work with the upstream. Xiaomi repositories are not synchronized with the upstream. This has led to bugs before. Changes to Apache repositories should be tested with Apache repositories. Splitting PRs into smaller ones is safer because it's easier to do reviews, even if the cost is higher CI usage. |
Sure, we have to re check to ensure no any break when upstream, Next pr will make it more clear and more relative and more convenient for reviewer。 |
Yes, so can you please tell us what you tested, how you tested and include the log results?
No, all PRs need to be clear and convenient for the reviewers. Please update this PR so that it is more clear, as raiden suggested. I really suggest all these commits be broken into multiple PRs, regardless of CI resources, because you have many unrelated fixes for multiple apps and they will all require their own summary, impact and test description. |
linguini1
left a comment
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.
Needs proper testing, should be split into multiple PRs.
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.
Thank you @jasonbu , please follow https://github.com/apache/nuttx/blob/master/CONTRIBUTING.md as requested by @linguini1 and @raiden00pl already :-)
We may possible enable socket and no ipv4 Signed-off-by: buxiasen <buxiasen@xiaomi.com>
We possible want to use iperf to test rpmsg socket etc. Signed-off-by: buxiasen <buxiasen@xiaomi.com>
|
@raiden00pl @cederom @linguini1 splited into #3225 #3226 #3227 #3228 #3229 |
|
@cederom @linguini1 can we continue? |
|
It seems all the related PRs were merged. None of them include any test logs or good test descriptions, so I will still hold my request for testing information on this PR. |
could you review the change? this patch resovle the compiler error, so the change pass ci already indicate the modification is good. The change which fix the compiler issue, don't need runtime log to indicate wether the modification is correct or not. |
I disagree because this PR introduces compile time changes that conditionally removes logic. It would be good to see testing of the modified examples. |
Summary
Fix some case break if we enable Net & local socket & rpmsg socket, but did not enable iNet and ipv6,
iNet is not a must have feature in Net, especially Asymmetric Multi-Processing devices.
Impact
After patch, able to open iperf to test rpmsg socket etc.
Testing
CI-test, qemu-arm-v7a ostest