Skip to content

testing/cmocka: stdio.h Header file adjustment related #2852

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

testing/cmocka: stdio.h Header file adjustment related #2852

wants to merge 1 commit into from

Conversation

anjiahao1
Copy link
Contributor

@anjiahao1 anjiahao1 commented Nov 14, 2024 •
edited by cederom
Loading

Summary

Impact

  • testing/cmocka: all used stdio (?)

Testing

sim:citest build

@nuttxpr
Copy link

nuttxpr commented Nov 14, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a link to the PR and mentions stdio.h and sim:citest, it lacks crucial details.

Here's what's missing:

  • Summary: It needs a clear explanation of why the change to stdio.h is necessary (bug fix, improvement, new feature?). What exactly was changed in the header file, and how does it work? It should also include a related NuttX issue number if one exists.
  • Impact: The PR states "all used stdio" which is too vague. It needs specific YES/NO answers to all the impact questions (user, build, hardware, documentation, security, compatibility). If any answer is YES, a detailed description is required. For example, if the change affects the user, explain how.
  • Testing: While it mentions sim:citest, it lacks specific logs from before and after the change. Simply stating "build" is insufficient. It should show the relevant output demonstrating the problem before the change and the successful result after the change. It also needs details about the build host (OS, CPU, compiler) used for testing. If other targets besides sim:citest were tested, those should be listed as well.

The provided link to the PR is helpful, but the PR description itself needs to be self-contained and meet all the listed requirements. Relying on reviewers to click through to another page to gather essential information is not good practice.

@anjiahao1
remove <nuttx/xxx.h> in stdio need add depend header file
9c2a17a 
Error: cmocka/src/cmocka.c:2568:9: error: implicit declaration of function 'ftruncate'; did you mean 'strncat'? [-Werror=implicit-function-declaration]
     2568 |         ftruncate(fileno(fp), ftell(fp));
          |         ^~~~~~~~~
          |         strncat

Signed-off-by: anjiahao <anjiahao@xiaomi.com>
xiaoxiang781216
xiaoxiang781216 reviewed Nov 14, 2024
View reviewed changes
testing/cmocka/CMakeLists.txt
@@ -43,6 +43,8 @@ if(CONFIG_TESTING_CMOCKA)
${CMAKE_CURRENT_LIST_DIR}/0005-cmocka-cmocka_private-fix-warning-in-cmocka_private.patch
&& patch -p0 -d ${CMAKE_CURRENT_LIST_DIR}/cmocka <
${CMAKE_CURRENT_LIST_DIR}/0006-fix-linux-risc-v-compile-error-list_initialize.patch
&& patch -p0 -d ${CMAKE_CURRENT_LIST_DIR}/cmocka <
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but no compiler error happen in cmocka:
https://github.com/apache/nuttx/actions/runs/11834392207/job/32975068050?pr=14697

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i test in linux, ftruncate is need include <unistd.h>. ftruncate is not use in cmocka mainline

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's update to cmocka new version instead

@cederom cederom changed the title stdio.h Header file adjustment related testing/cmocka: stdio.h Header file adjustment related Jan 30, 2025
cederom
cederom requested changes Jan 30, 2025
View reviewed changes
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 @anjiahao1 :-)

  • Please update git commit topic and message.
  • Should this PR go in pair with apache/nuttx#14697 or is it independent update?

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@xiaoxiang781216 xiaoxiang781216 xiaoxiang781216 left review comments

@cederom cederom cederom requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Area: Testing Size: S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@anjiahao1 @nuttxpr @cederom @xiaoxiang781216