Skip to content

Conversation

@xuxin930
Copy link
Contributor

@xuxin930 xuxin930 commented Oct 9, 2024

Summary

sync CMake missing sources

There are differences between the current CMake base and Makefile.
This patch synchronizes the differences between modules
so that the CMake compilation content should be consistent with the Makefile.

Impact

[arm ach, fs, sched, mm , libc, drivers]

Testing

all CI build

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Area: File System File System issues Area: Memory Management Memory Management issues Area: OS Components OS Components issues Size: M The size of the change in this PR is medium labels Oct 9, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 9, 2024

[Experimental Bot, please feedback here]

Yes, but with minor improvements:

Meets Requirements:

  • Summary: Clearly states the "why" (CMake/Makefile discrepancy), "what" (synchronizing modules), and "how" (patch for consistency).
  • Impact: Lists affected areas, which is helpful.
  • Testing: States "all CI builds" pass, indicating thoroughness.

Suggested Improvements:

  • Impact:
    • Be more specific. Instead of just listing areas, briefly describe the impact within each. E.g., "fs: Adds missing source files for X feature."
    • Address ALL impact points. Even if it's "NO", explicitly state it for clarity (especially security, compatibility, etc.).
  • Testing:
    • While "all CI builds" is good, a few specific examples of before/after log snippets would add strong evidence. Choose ones demonstrating the problem being solved.

Overall: Good start! Adding the specifics mentioned above will make this PR review much smoother.

@xuxin930
Copy link
Contributor Author

xuxin930 commented Oct 9, 2024

image
hi @simbit18 It seems that there is a bash command line limitation issue under msys2
Could you help take a look?

@simbit18
Copy link
Contributor

simbit18 commented Oct 9, 2024

Hi @xuxin930 Sure now I try.

@simbit18
Copy link
Contributor

simbit18 commented Oct 9, 2024

I tried this PR locally

Cmake
The problem does not occur when building NuttX only with Cmake.

[100%] Linking C static library libapps.a
[100%] Built target apps
[100%] Building C object apps/system/nsh/CMakeFiles/apps_nsh.dir/nsh_main.c.obj
[100%] Linking C static library libapps_nsh.a
[100%] Built target apps_nsh
[100%] Building C object apps/system/nsh/CMakeFiles/apps_sh.dir/sh_main.c.obj
[100%] Linking C static library libapps_sh.a
[100%] Built target apps_sh
[100%] Building C object apps/examples/hello/CMakeFiles/apps_hello.dir/hello_main.c.obj
[100%] Linking C static library libapps_hello.a
[100%] Built target apps_hello
[100%] Building C object CMakeFiles/nuttx.dir/empty.c.obj
[100%] Linking C executable nuttx
[100%] Built target nuttx
[100%] Generating nuttx.hex
[100%] Built target nuttx-hex
[100%] Generating nuttx.bin
[100%] Built target nuttx-bin

Cmake+Ninja
The problem occurs when building NuttX with Cmake+Ninja.

/bin/sh: line 1: /home/bit/nuttxnew/tools/gcc-arm-none-eabi/bin/arm-none-eabi-ar: Argument list too long
[813/1026] Building C object mm/CMakeFiles/mm.dir/umm_heap/umm_malloc_size.c.obj
ninja: build stopped: subcommand failed.

It is certainly related to the fact that Windows has a limit of 8192 characters for command line arguments.

This does not seem an easy error to solve!!!

Should we disable CMAKE + Ninja for Msys2 while waiting for a solution?

@xuxin930
Copy link
Contributor Author

xuxin930 commented Oct 9, 2024

Should we disable CMAKE + Ninja for Msys2 while waiting for a solution?

@simbit18 nice catch!
We split the ar step here, but it seems that it only works for the default generator, not the ninja generator.
I agree that disable the ninja generator in the msys2 environment.

set(CMAKE_ARCHIVE_COMMAND "<CMAKE_AR> rcs <TARGET> <LINK_FLAGS> <OBJECTS>")
set(CMAKE_RANLIB_COMMAND "<CMAKE_RANLIB> <TARGET>")
set(CMAKE_C_ARCHIVE_CREATE ${CMAKE_ARCHIVE_COMMAND})
set(CMAKE_CXX_ARCHIVE_CREATE ${CMAKE_ARCHIVE_COMMAND})
set(CMAKE_ASM_ARCHIVE_CREATE ${CMAKE_ARCHIVE_COMMAND})
set(CMAKE_C_ARCHIVE_APPEND ${CMAKE_ARCHIVE_COMMAND})
set(CMAKE_CXX_ARCHIVE_APPEND ${CMAKE_ARCHIVE_COMMAND})
set(CMAKE_ASM_ARCHIVE_APPEND ${CMAKE_ARCHIVE_COMMAND})
set(CMAKE_C_ARCHIVE_FINISH ${CMAKE_RANLIB_COMMAND})
set(CMAKE_CXX_ARCHIVE_FINISH ${CMAKE_RANLIB_COMMAND})
set(CMAKE_ASM_ARCHIVE_FINISH ${CMAKE_RANLIB_COMMAND})

Signed-off-by: xuxin19 <xuxin19@xiaomi.com>
Signed-off-by: xuxin19 <xuxin19@xiaomi.com>
@simbit18
Copy link
Contributor

simbit18 commented Oct 9, 2024

@xuxin930 I will be away for a few days so if you decide to disable Cmake + Ninja for Msys2 just remove -N

on build.yml for Nuttx

./cibuild.sh -g -i -A -C -N -R testlist/${{matrix.boards}}.dat

and Apps
https://github.com/apache/nuttx-apps/blob/faea166566d1620d5ca4f6b5ea0419b148b042f4/.github/workflows/build.yml#L322

@xuxin930
Copy link
Contributor Author

xuxin930 commented Oct 9, 2024

Signed-off-by: xuxin19 <xuxin19@xiaomi.com>
@xiaoxiang781216 xiaoxiang781216 merged commit 7def098 into apache:master Oct 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arch: arm Issues related to ARM (32-bit) architecture Area: File System File System issues Area: Memory Management Memory Management issues Area: OS Components OS Components issues Size: M The size of the change in this PR is medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants