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: Map all output files to the bin directory #14657

Closed
wants to merge 1 commit into from

Conversation

W-M-R
Copy link
Contributor

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

Summary

libxx: Map all output files to the bin directory

  1. There are files with the same name in libcxx, so the generated files must be separated by files

Impact

No

Testing

Mo

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

nuttxpr commented Nov 5, 2024

[Experimental Bot, please feedback here]

No, this PR does not meet the NuttX requirements. While it provides a summary, it lacks crucial information.

Here's why:

  • Insufficient Summary: While it states what is being changed (mapping output files), it fails to explain why this change is necessary, how it works, and if it relates to any existing issues.
  • Missing Impact Assessment: Simply stating "No" is insufficient. Each impact category needs to be addressed individually, even if the answer is no. A justification is required. For example, "Impact on user: NO - This is a purely internal change to the build system and will not affect user experience."
  • Inadequate Testing: "Mo" is not acceptable. The testing section needs to provide details about the build host and target environment(s). It also requires logs demonstrating the behavior before and after the change to prove the fix works as intended. Without proper logs, there's no evidence the changes have been tested effectively.

To make this PR acceptable, the author needs to:

  1. Expand the Summary: Explain the rationale behind moving output files. What problem does this solve? How does the mapping mechanism work? Are there any related issues?

  2. Address Each Impact Category: Even if the impact is "NO", briefly explain why. For instance:

    • Impact on build: YES - The build system now places all output files in the bin directory.
    • Impact on user: NO - This change only affects the build process and should not impact user functionality.
    • ...and so on for all categories.
  3. Provide Thorough Testing Information: Specify the build host OS, CPU architecture, compiler, and version. Detail the target architecture, board, and configuration. Include relevant logs from before and after the change to demonstrate the issue and the fix.

Without these improvements, the PR provides insufficient information for proper review and cannot be considered complete.

1. There are files with the same name in libcxx, so the generated files must be separated by files

Signed-off-by: wangmingrong1 <[email protected]>
@@ -37,6 +37,11 @@ libcxx: libcxx-$(LIBCXX_VERSION).src.tar.xz
$(Q) patch -p0 < 0001-libcxx-fix-ld-errors.patch
$(Q) patch -p0 < 0001-Fix-build-error-about-__GLIBC__.patch
$(Q) touch $@
$(Q) mkdir $(BINDIR)/libcxx \
Copy link
Contributor

Choose a reason for hiding this comment

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

why need manually create the subdir?

@W-M-R
Copy link
Contributor Author

W-M-R commented Nov 6, 2024

Through discussion, since the bin directory is mainly for separate compilation of kernel mode and user mode, libxx does not need this process. Therefore, the creation of the bin directory is meaningless and this patch will be closed.

@W-M-R W-M-R closed this Nov 6, 2024
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: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants