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

libbuiltin: Simplify makefile writing #14661

Merged
merged 1 commit into from
Nov 6, 2024
Merged

Conversation

W-M-R
Copy link
Contributor

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

Create a separate bin folder to place the generated .o files

Note: Please adhere to Contributing Guidelines.

Summary

Create a separate bin folder to place the generated .o files

Impact

None

Testing

None

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

nuttxpr commented Nov 6, 2024

[Experimental Bot, please feedback here]

No, this PR description does not meet the NuttX requirements. It's far too brief and lacks crucial information. Here's why and how it can be improved:

Missing Information in Summary:

  • Why is this change necessary? What problem does it solve? Is it for organizational purposes, build speed improvements, or something else? This is the most critical missing piece.
  • What functional part of the code is being changed? Is this a change to the build system (Makefiles, CMakeLists.txt)? Be specific.
  • How does the change exactly work? Describe the mechanism. Does it modify build rules? Introduce new variables? Walk through the technical details.

Missing Information in Impact:

  • While stating "None" is concise, it's likely inaccurate. Consider these:
    • Impact on build: This almost certainly changes the build process. Where is the bin folder created? How does this affect dependency tracking? Clean targets? This needs detailed explanation.
    • Impact on compatibility: Does this break existing user configurations or scripts that rely on the current output directory structure?
  • Even if there is truly no impact in other areas, explicitly stating "NO" for each category demonstrates that you've considered those aspects.

Missing Information in Testing:

  • "None" is unacceptable. You must test your changes and provide evidence.
  • Specify your build host details (OS, compiler, etc.).
  • Specify your target platform (architecture, board, configuration).
  • Show actual logs or output demonstrating the behavior before and after the change. For example, demonstrate where the .o files are placed before your change, and then after your change. Directory listings would be helpful. Even a simple "make" output showing the changed compilation commands would be better than nothing.

Example of an Improved PR Description:

Summary

This change improves build organization by creating a dedicated bin directory within each build directory to store compiled object files (.o). Currently, object files are mixed with source files, which can clutter the build directory and make it harder to navigate. This change aims to improve organization and potentially simplify cleaning up build artifacts. This addresses the lack of build organization discussed in [NuttX Issue #XYZ] (if applicable).

Impact

  • Is new feature added? Is existing feature changed? Existing feature changed (build process).
  • Impact on user: YES. Users will need to be aware that object files are now located in a bin subdirectory. Existing scripts or tools that rely on the previous location of object files may need to be updated.
  • Impact on build: YES. The build system (Makefiles) will be modified to create the bin directory and place object files within it. This is achieved by adding bin/ before the object file name in the compile rules. The clean target will also be updated to remove the bin directory.
  • Impact on hardware: NO.
  • Impact on documentation: YES. The build system documentation should be updated to reflect the new directory structure. I will update the documentation as part of this PR.
  • Impact on security: NO.
  • Impact on compatibility: YES. Backward compatibility may be affected if users have scripts relying on the old object file location. This will be documented.
  • Anything else to consider: None.

Testing

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

  • Build Host(s): Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
  • Target(s): sim:nsh

Testing logs before change:

ls build/
Makefile  nsh.c  nsh.o  ... other files

Testing logs after change:

ls build/
Makefile  bin/  nsh.c ... other files
ls build/bin
nsh.o  ... other object files

By providing this level of detail, reviewers can understand the purpose, implementation, and impact of your changes, leading to a smoother and faster review process. Remember to replace the placeholder information with your actual details.

Create a separate bin folder to place the generated .o files

Signed-off-by: wangmingrong1 <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit 4ba941e into apache:master Nov 6, 2024
27 checks passed
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: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants