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

libxx: All cxx libraries move down one level #14668

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

Conversation

W-M-R
Copy link
Contributor

@W-M-R W-M-R commented Nov 6, 2024

Note: Please adhere to Contributing Guidelines.

Summary

Since there are some patches that need to be loaded manually, the original method is rather confusing. Now these patches that need to be loaded manually are classified into various cxx library folders, and each dynamically downloaded cxx library is moved down one directory to make the directory structure clearer.

Impact

No

Testing

No

@github-actions github-actions bot added Area: OS Components OS Components issues Size: L The size of the change in this PR is large labels Nov 6, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 6, 2024

[Experimental Bot, please feedback here]

No. This PR does not adequately meet the NuttX requirements. While it provides a brief summary, it lacks crucial details. Here's why:

  • Insufficient Summary: "All cxx libraries move down one level" is vague. Why was this move necessary? What problem does it solve? What specific libraries are affected? What does "down one level" mean in terms of directory structure?
  • Missing Issue References: Are there related NuttX or NuttX Apps issues that prompted this change? These should be linked.
  • Impact Underexplored: Simply stating "No" for all impact categories is insufficient. Even seemingly simple changes can have unforeseen consequences. Consider build processes, dependencies, and potential compatibility issues. Justify why there's no impact in each area. For example, if there's no impact on the build, explain why moving the libraries doesn't affect build scripts or dependencies.
  • Inadequate Testing: Stating "depends on another PR" isn't enough. While dependencies exist, this PR needs its own verification. Provide specific build host and target details, along with "before" and "after" logs demonstrating the change's effectiveness and the lack of unintended side effects. What happens if the other PR isn't merged? How is this PR tested independently?

In short, the PR needs to be significantly more detailed and self-contained to meet the NuttX requirements. It should clearly explain the motivation, implementation details, potential impact, and provide independent verification of its functionality.

@W-M-R W-M-R force-pushed the dev-libxx branch 5 times, most recently from 4bb2673 to 235d793 Compare November 6, 2024 12:54
Copy link
Contributor

@acassis acassis left a comment

Choose a reason for hiding this comment

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

Approved, but please include an explanatory message in the commit log as to why this was necessary?

@W-M-R
Copy link
Contributor Author

W-M-R commented Nov 6, 2024

Approved, but please include an explanatory message in the commit log as to why this was necessary?

OKey

libs/libxx/uClibc++/CMakeLists.txt Outdated Show resolved Hide resolved
libs/libxx/uClibc++/Make.defs Show resolved Hide resolved
libs/libxx/libcxxabi/CMakeLists.txt Outdated Show resolved Hide resolved
libs/libxx/libcxx/CMakeLists.txt Outdated Show resolved Hide resolved
@lupyuen
Copy link
Member

lupyuen commented Nov 7, 2024

Sorry I have to cancel the CI Check, there's a problem with CI Test:

@W-M-R
Copy link
Contributor Author

W-M-R commented Nov 7, 2024

Sorry I have to cancel the CI Check, there's a problem with CI Test:

Do we need to restart ci after the fix is ​​merged?

@lupyuen
Copy link
Member

lupyuen commented Nov 7, 2024

@W-M-R Hopefully not? Let us track down the root cause and figure out the impact. Thanks!

@W-M-R W-M-R force-pushed the dev-libxx branch 2 times, most recently from 610bd67 to 2c79bda Compare November 7, 2024 08:31
Since there are some patches that need to be loaded manually, the original method is rather confusing. Now these patches that need to be loaded manually are classified into various cxx library folders, and each dynamically downloaded cxx library is moved down one directory to make the directory structure clearer.

Signed-off-by: wangmingrong1 <[email protected]>
@lupyuen
Copy link
Member

lupyuen commented Nov 7, 2024

Sorry I cancelled the CI Check again, since there's a problem with CI Test:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: OS Components OS Components issues Size: L The size of the change in this PR is large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants