Skip to content
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

cmake(bugfix):reduce static library propagation behavior #2844

Merged
merged 1 commit into from
Nov 13, 2024

Conversation

xuxin930
Copy link
Contributor

Summary

[bugfix]

target_link_library will cause duplicate definitions during the link process

Impact

bugfix

Testing

cmake -B build -DBOARD_CONFIG=nrf52832-dk:mcuboot_app -GNinja

cmake --build build -j12

test build pass

This will cause duplicate definitions during the link  process

Signed-off-by: xuxin19 <[email protected]>
@nuttxpr
Copy link

nuttxpr commented Nov 13, 2024

[Experimental Bot, please feedback here]

No, this PR description does not adequately meet the NuttX requirements. Here's why and how to fix it:

  • Insufficient Summary: While the summary identifies this as a bugfix and mentions target_link_library causing duplicate definitions, it lacks crucial details:

    • Why was this change necessary? What specific problem did the duplicate definitions cause? (e.g., build failures, runtime crashes, unexpected behavior).
    • What functional part of the code is being changed? Be precise. Which files/functions/modules were modified? (e.g., CMakeLists.txt in the boards/arm/nordic/nrf52832-dk directory)
    • How does the change work? Don't just state the problem; explain the solution. What was done to prevent the duplicate definitions? (e.g., removed redundant library linking, corrected library order, etc.).
    • Issue References: Are there any related NuttX or NuttX Apps issues? If so, provide links. Even if not, stating "N/A" is helpful for clarity.
  • Incomplete Impact Assessment: Simply stating "bugfix" isn't enough. While several impacts are likely "NO," you must explicitly state this for each item. For example:

    • Impact on user: NO
    • Impact on build: YES (build now succeeds where it previously failed) <-- Explain the specific build impact.
    • Impact on hardware: NO
    • Impact on documentation: NO (or YES if documentation was updated to reflect the fix)
    • Impact on security: NO (or YES if the bug had security implications)
    • Impact on compatibility: NO (or YES if the fix affects compatibility)
  • Insufficient Testing Details:

    • Build Host: Provide details about your build host OS, CPU architecture, and compiler version. (e.g., Linux x86_64, GCC 12.2.1).
    • Targets: Be more specific about the target. You mention nrf52832-dk:mcuboot_app, but provide the full details (e.g., nRF52832-DK board, mcuboot bootloader configuration).
    • Missing "Before" Logs: You must provide logs demonstrating the problem before your change. Show the build errors caused by the duplicate definitions.
    • "After" Logs Should Show Success: The "after" logs should show a clean, successful build. Consider also including relevant runtime logs demonstrating that the fix works correctly.

Example of an Improved PR Description:

Summary

This PR fixes a build issue where using target_link_libraries in the boards/arm/nordic/nrf52832-dk/CMakeLists.txt file resulted in duplicate symbol definitions during the linking stage when building the mcuboot_app configuration. This caused build failures preventing the application from being linked. The fix removes the redundant linking of the libxyz library, which was already being included through a dependency.

Related NuttX Issue: N/A
Related NuttX Apps Issue/PR: N/A

Impact

  • Is new feature added? NO
  • Is existing feature changed? NO
  • Impact on user: NO
  • Impact on build: YES (The build now completes successfully for the nrf52832-dk:mcuboot_app configuration).
  • Impact on hardware: NO
  • Impact on documentation: NO
  • Impact on security: NO
  • Impact on compatibility: NO

Testing

I confirm that changes are verified on local setup and works as intended:

  • Build Host(s): Linux Ubuntu 22.04, x86_64, GCC 12.1.0
  • Target(s): nRF52832-DK, mcuboot_app configuration

Testing logs before change:

[100%] Linking CXX executable nuttx
/usr/bin/ld: nuttx.elf: in function `some_function':
path/to/libxyz.a(some_file.o):(.text+0x123): multiple definition of `some_function';
nuttx.elf:(.text+0x456): first defined here
collect2: error: ld returned 1 exit status
make[2]: *** [nuttx] Error 1

Testing logs after change:

[100%] Linking CXX executable nuttx
[100%] Built target nuttx

By providing specific details, you make it much easier for reviewers to understand and evaluate your changes, significantly increasing the chances of your PR being accepted. Remember to adapt this example to your specific situation.

@xiaoxiang781216 xiaoxiang781216 merged commit 37bab55 into apache:master Nov 13, 2024
25 checks passed
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.

3 participants