Skip to content

Conversation

@jasonbu
Copy link
Contributor

@jasonbu jasonbu commented Nov 25, 2025

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

@jasonbu jasonbu force-pushed the tests branch 2 times, most recently from b9f0118 to fbf3bfd Compare November 26, 2025 03:31
@linguini1
Copy link
Contributor

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.

@jasonbu
Copy link
Contributor Author

jasonbu commented Nov 27, 2025

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.
If we create pr for specific topic, I agree with we should only include relative patchs.

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.

@raiden00pl
Copy link
Member

@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.

@jasonbu
Copy link
Contributor Author

jasonbu commented Nov 27, 2025

@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,
as these change a all small and in apps, merged commits into one pr here.

Next pr will make it more clear and more relative and more convenient for reviewer。

@linguini1
Copy link
Contributor

linguini1 commented Nov 27, 2025

Sure, we have to re check to ensure no any break when upstream, as these change a all small and in apps, merged commits into one pr here.

Yes, so can you please tell us what you tested, how you tested and include the log results?

Next pr will make it more clear and more relative and more convenient for reviewer。

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.

Copy link
Contributor

@linguini1 linguini1 left a 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.

Copy link
Contributor

@cederom cederom left a 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>
@jasonbu jasonbu changed the title Fix samples/tests abnormal scenes Fix if enable Net/socket/iperf but not enable iNet/IPv4. Nov 30, 2025
@jasonbu jasonbu requested review from cederom and linguini1 November 30, 2025 08:13
@jasonbu
Copy link
Contributor Author

jasonbu commented Nov 30, 2025

@raiden00pl @cederom @linguini1 splited into #3225 #3226 #3227 #3228 #3229
hopes will make it more easy to understand, and be happy

@jerpelea jerpelea changed the title Fix if enable Net/socket/iperf but not enable iNet/IPv4. kernel/syscall: Fix if enable Net/socket/iperf but not enable iNet/IPv4. Dec 1, 2025
@xiaoxiang781216
Copy link
Contributor

@cederom @linguini1 can we continue?

@linguini1
Copy link
Contributor

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.

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Dec 5, 2025

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.

@linguini1
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants