Skip to content

Conversation

@guohao15
Copy link
Contributor

Summary

1 make the libuv run use epoll as backend
2 make the inotify test case runable in nuttx

Impact

select epoll as backend may spend more stack memory

Testing

uv_run_tests fs_event_watch_dir
uv_run_tests fs_event_close_in_callback
uv_run_tests fs_event_watch_file
uv_run_tests fs_event_watch_file_current_dir
uv_run_tests fs_event_ref
uv_run_tests fs_event_immediate_close
uv_run_tests fs_event_watch_file_exact_path
uv_run_tests fs_event_no_callback_on_close
uv_run_tests fs_event_no_callback_after_close
uv_run_tests fs_event_close_with_pending_event
uv_run_tests fs_event_close_with_pending_delete_event
uv_run_tests fs_event_error_reporting
uv_run_tests fs_event_start_and_close
uv_run_tests fs_event_watch_file_twice
uv_run_tests fs_event_getpath
uv_run_tests fs_event_watch_invalid_path
uv_run_tests fs_event_stop_in_cb

@nuttxpr
Copy link

nuttxpr commented Oct 12, 2024

[Experimental Bot, please feedback here]

The PR summary is lacking some details. Here's a breakdown and suggestions:

What's Missing in the Summary

  • Why: The summary states WHAT is being changed ("make libuv use epoll", "make inotify test runnable"), but not WHY these changes are necessary.
    • Is there a bug being fixed?
    • Is this adding a new capability to NuttX?
    • Is it improving performance?
  • How (more details): The summary is very high-level. It would be helpful to elaborate on HOW these changes are implemented:
    • libuv epoll: What parts of the libuv code were modified to support epoll? Were any new files added?
    • inotify test: What prevented the inotify test from running previously? What changes were needed to make it functional?
  • Issues/PRs: The summary should explicitly link to any related NuttX issues or pull requests.

Impact Section: Needs More Detail

  • Stack Memory: Quantify the impact. How much more stack memory is used? Is this a significant concern for resource-constrained devices?
  • Other Impacts: The PR focuses solely on stack memory. Consider these questions:
    • Build: Will any build system changes be needed (new configuration options, dependencies)?
    • Hardware: Does epoll have any specific hardware requirements or dependencies?
    • Documentation: Will documentation need to be updated to reflect these changes?
    • Compatibility: Could these changes potentially break existing applications or have compatibility implications?

Testing Section: Missing Context

  • Testing Environment: Provide details about your testing setup:
    • Which NuttX version are you using?
    • What is your target hardware (or simulator)?
    • Which compiler and toolchain were used?
  • Log Snippets: The current logs are just test names. Include relevant snippets of the logs that demonstrate the changes in behavior before and after your modifications. Focus on:
    • libuv epoll: Show logs that confirm epoll is being used correctly.
    • inotify test: Show that the test, which previously failed, now passes.

In conclusion: The PR needs more detail and context to meet NuttX requirements. Focus on explaining the "why" behind the changes, providing specifics about the implementation, quantifying the impact, and including relevant testing details.

@cederom
Copy link
Contributor

cederom commented Oct 13, 2024

Looks like build errors to be fixed? Will restart failed CI tasks :-)

(...)
x luv-1.44.2-1/src/fs_poll.c
x luv-1.44.2-1/src/thread.c
x luv-1.44.2-1/src/lhandle.c
x luv-1.44.2-1/README.md

100  2058    0  2058    0     0   2955      0 --:--:-- --:--:-- --:--:--  2955
100 1646k    0 1646k    0     0  1454k      0 --:--:--  0:00:01 --:--:-- 3779k
Error: libuv/src/threadpool.c:201:5: error: use of undeclared identifier 'CONFIG_LIBUV_THREADPOOL_PRIORITY'
    DEF_THREADPOOL_PRIORITY
    ^
<command line>:14:33: note: expanded from macro 'DEF_THREADPOOL_PRIORITY'
#define DEF_THREADPOOL_PRIORITY CONFIG_LIBUV_THREADPOOL_PRIORITY
                                ^
1 error generated.
make[2]: *** [threadpool.c.Users.runner.work.nuttx-apps.nuttx-apps.sources.apps.system.libuv.o] Error 1
make[2]: Target `all' not remade because of errors.
make[1]: *** [/Users/runner/work/nuttx-apps/nuttx-apps/sources/apps/system/libuv_all] Error 2
make[1]: Target `all' not remade because of errors.
make: *** [/Users/runner/work/nuttx-apps/nuttx-apps/sources/apps/libapps.a] Error 2
make: Target `all' not remade because of errors.
/Users/runner/work/nuttx-apps/nuttx-apps/sources/nuttx/tools/testbuild.sh: line 385: /Users/runner/work/nuttx-apps/nuttx-apps/sources/nuttx/../nuttx/nuttx.manifest: No such file or directory
  Normalize sim/lua

Donny9 and others added 5 commits October 14, 2024 14:22
Signed-off-by: dongjiuzhu1 <dongjiuzhu1@xiaomi.com>
Signed-off-by: guohao15 <guohao15@xiaomi.com>
Signed-off-by: guohao15 <guohao15@xiaomi.com>
Signed-off-by: guohao15 <guohao15@xiaomi.com>
Signed-off-by: guohao15 <guohao15@xiaomi.com>
Signed-off-by: yinshengkai <yinshengkai@xiaomi.com>
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 @guohao15 :-)

@GUIDINGLI GUIDINGLI merged commit 9c51919 into apache:master Oct 15, 2024
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.

8 participants