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

build: Move __NuttX__ definition to tools/Config.mk #2192

Merged
merged 1 commit into from
Nov 2, 2020

Conversation

xiaoxiang781216
Copy link
Contributor

Summary

ensure this critical macro get defined in all projects

Impact

No functional change

Testing

All build pass

tools/Config.mk Show resolved Hide resolved
ensure this critical macro get defined in all projects

Signed-off-by: Xiang Xiao <xiaoxiang@xiaomi.com>
Copy link
Contributor

@yamt yamt left a comment

Choose a reason for hiding this comment

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

lgtm

@patacongo
Copy link
Contributor

patacongo commented Nov 2, 2020 via email

@xiaoxiang781216
Copy link
Contributor Author

xiaoxiang781216 commented Nov 2, 2020

Yes, but most developer use the official toolchain, not NuttX build root toolchain. We have to consider this usecase too. A common location is needed here, otherwise the duplication will spread out all board's Make.defs or the compiler will fail in some external library which explicitly check __NuttX__. And the change isn't harmful byself because it's compatible with both toolchain.

@jerpelea
Copy link
Contributor

jerpelea commented Nov 2, 2020

LGTM

@protobits protobits merged commit 9208176 into apache:master Nov 2, 2020
@xiaoxiang781216 xiaoxiang781216 deleted the make branch November 2, 2020 18:23
@masayuki2009
Copy link
Contributor

@xiaoxiang781216
I noticed that this PR affected build time.

For example, spresense:wifi_smp with a parallel build

Before:
real	0m23.740s
user	1m16.792s
sys	0m18.236s

After:
real	0m28.574s
user	1m23.348s
sys	0m19.613s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants