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/mkconfig: add support of set custom command options to config.h #15594

Merged
merged 1 commit into from
Jan 18, 2025

Conversation

anchao
Copy link
Contributor

@anchao anchao commented Jan 17, 2025

Summary

cmake/mkconfig: add support of set custom command options to config.h

add support of set custom command options to config.h
Use the -D parameter to pass the config to header file.
NOTE which must begin with the CONFIG_ prefix.
eg:

  cmake -DCONFIG_AAA=1 -DCONFIG_BBB=1 -B build -DBOARD_CONFIG=sim/nsh -GNinja

source file:

#include <nuttx/config.h>

int main(int argc, FAR char *argv[])
{
#ifdef CONFIG_AAA
  printf("Hello, World!! AAA: %d\n", CONFIG_AAA);
#endif
#ifdef CONFIG_BBB
  printf("Hello, World!! BBB: %d\n", CONFIG_BBB);
#endif
  return 0;
}

output:

$ ./build/nuttx 

NuttShell (NSH) NuttX-10.4.0
nsh> hello
Hello, World!! AAA: 1
Hello, World!! BBB: 1
nsh> 

Signed-off-by: chao an [email protected]

Impact

N/A

Testing

ci-check

@github-actions github-actions bot added Area: Build system Size: S The size of the change in this PR is small labels Jan 17, 2025
add support of set custom command options to config.h
Use the -D parameter to pass the config to header file.
NOTE which must begin with the CONFIG_ prefix.
eg:
  cmake -DCONFIG_AAA=1 -DCONFIG_BBB=1 -B build -DBOARD_CONFIG=sim/nsh -GNinja

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

nuttxpr commented Jan 17, 2025

[Experimental Bot, please feedback here]

This PR does not fully meet the NuttX requirements. While it provides a summary of the change and testing results, it lacks crucial information in the Impact section. Here's a breakdown:

Missing Information:

  • Impact - Is new feature added? Is existing feature changed?: This is clearly a new feature and should be explicitly stated as "YES - New feature added."
  • Impact on user (will user need to adapt to change?): While seemingly minor, this does impact users. Users now have a new way to define configurations. This needs to be explained. Something like "YES - Users can now define custom configuration options using -DCONFIG_XXX=value during the CMake configuration stage."
  • Impact on build (will build process change?): This does impact the build process. "YES - The CMake configuration process now recognizes and handles -DCONFIG_XXX options, adding them to the generated config.h file."
  • Impact on documentation (is update required / provided?): Documentation almost certainly needs updating to explain this new feature. If documentation is provided in the PR itself, state that. Otherwise, state "YES - Documentation updates are required to explain the usage of -DCONFIG_XXX options."

Insufficient Information:

  • Impact (all other sections): Simply stating "N/A" is not acceptable. Even if there's no impact, explicitly state "NO - No impact on ...". This shows that the impact was considered.

Recommendations for Improvement:

  1. Expand the Impact section significantly. Address all points mentioned above. Be explicit. Don't assume reviewers will understand the implications of the change.
  2. Clarify the benefit of this change. Why is this new feature useful? What problems does it solve? This should be in the Summary.
  3. Provide more comprehensive testing logs. The "ci-check" statement is insufficient. Show relevant portions of the build logs demonstrating the change works as intended. Include examples with different data types (string, integer, etc.) if applicable.
  4. Consider potential downsides. Are there any security implications of allowing arbitrary CONFIG_ defines? Could this lead to naming conflicts?

By addressing these points, the PR will be much more likely to be accepted. Clear and thorough documentation of changes is crucial for open-source projects.

@acassis
Copy link
Contributor

acassis commented Jan 17, 2025

@anchao does it also work for standard makefiles ?

@anchao
Copy link
Contributor Author

anchao commented Jan 18, 2025

@anchao does it also work for standard makefiles ?

@acassis Makefiles already have this feature, but due to different backend implementations, makefile use EXTRAFLAGS to pass special definitions:

make -j12 EXTRAFLAGS="-DCONFIG_AAA=1 -DCONFIG_BBB=1"

(I remember you had a related wiki introducing this feature before :)

@acassis
Copy link
Contributor

acassis commented Jan 18, 2025

@anchao does it also work for standard makefiles ?

@acassis Makefiles already have this feature, but due to different backend implementations, makefile use EXTRAFLAGS to pass special definitions:

make -j12 EXTRAFLAGS="-DCONFIG_AAA=1 -DCONFIG_BBB=1"

(I remember you had a related wiki introducing this feature before :)

I don't remember, but I remember using EXTRALIB flag sometime ago, hehehe

@acassis acassis merged commit 36b2b90 into apache:master Jan 18, 2025
39 checks passed
@anchao
Copy link
Contributor Author

anchao commented Jan 18, 2025

I don't remember, but I remember using EXTRALIB flag sometime ago, hehehe

@acassis here it is
https://acassis.wordpress.com/2022/10/18/how-to-transform-warning-in-error-during-nuttx-compilation/

On second thought, would it be better to use EXTRAFLAGS in cmake?

@anchao
Copy link
Contributor Author

anchao commented Jan 18, 2025

@acassis please review again: #15596

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Build system 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.

5 participants