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

Generate BuildConfig headers in GN #2270

Merged

Conversation

mspang
Copy link
Contributor

@mspang mspang commented Aug 20, 2020

Generate config headers to save the configuration options to be installed
or used by another build system.

Unlike autoconf but like the main config headers (e.g. CHIPConfig.h)
this generates separate configs per component (e.g. CHIPBuildConfig.h).
This will allow scoping of options to particular config headers, which
didn't quite work properly with automake (options set by autoconf became
available everywhere).

The header generator template is derived from Chrome's, but with some
extra wrapping of define usage with BUILDFLAG() removed. That's a safety
feature that we may want to bring back - it triggers an error when macros
resolve to zero after forgetting an #include. Alternatively, we can enable
-Wundef, although all integrations may not be able to do that.

This is a prerequisite for closing #1513.

This is needed for integrating multiple build systems without
significant micromanagement of ABI affecting #defines. Most of CHIP's
configuration affects ABI (unlike e.g. OpenThread).

Unlike autoconf but like the main config headers (e.g. CHIPConfig.h)
this generates separate configs per component (e.g. CHIPBuildConfig.h).
This will allow scoping of options to particular config headers, which
didn't quite work properly with automake (all autoconf generated options
were available everywhere).

The header generator is derived from Chrome's, but without needing to
wrap all config usages with BUILDFLAG(). That's a safety feature that
we may want to bring back - it avoids configs resolving to zero when
an #include is missing. Alternatively, we should enable -Wundef,
although probably not all integrations will be able to do that.
Copy link
Contributor

@rwalker-apple rwalker-apple left a comment

Choose a reason for hiding this comment

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

lgtm.

This would have saved me a lot of work in my recent "gn in chip_xcode_connector.sh" work, but I'm a little concerned longer term CHIP's use of config headers to govern behavior, as there are alternatives that are less fragile (if possibly fatter).

@mspang
Copy link
Contributor Author

mspang commented Aug 21, 2020

lgtm.

This would have saved me a lot of work in my recent "gn in chip_xcode_connector.sh" work, but I'm a little concerned longer term CHIP's use of config headers to govern behavior, as there are alternatives that are less fragile (if possibly fatter).

I share the concern. I like openthread's approach - C API that doesn't change based on config. Is that what you're thinking or something else?

@rwalker-apple
Copy link
Contributor

lgtm.
This would have saved me a lot of work in my recent "gn in chip_xcode_connector.sh" work, but I'm a little concerned longer term CHIP's use of config headers to govern behavior, as there are alternatives that are less fragile (if possibly fatter).

I share the concern. I like openthread's approach - C API that doesn't change based on config. Is that what you're thinking or something else?

Yes: an API that doesn't change at a minimum

Copy link
Contributor

@e-rk e-rk left a comment

Choose a reason for hiding this comment

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

Works great. Thanks for this. 👍

@github-actions
Copy link

Size increase report for "nrfconnect-example-build"

File Section File VM
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-nrf52840-lock-example.elf and ./pull_artifact/chip-nrf52840-lock-example.elf:

sections,vmsize,filesize
.debug_info,0,13303
.debug_line,0,2495
.debug_abbrev,0,837
.debug_str,0,182
[Unmapped],0,7


@github-actions
Copy link

Size increase report for "esp32-example-build"

File Section File VM
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-wifi-echo.elf and ./pull_artifact/chip-wifi-echo.elf:

sections,vmsize,filesize
.debug_info,0,48


@github-actions
Copy link

Size increase report for "gn_nrf-example-build"

File Section File VM
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv


@github-actions
Copy link

Size increase report for "gn_linux-example-build"

File Section File VM
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv


@lgtm-com
Copy link

lgtm-com bot commented Aug 21, 2020

This pull request introduces 1 alert when merging 259eb91 into 1cf3b1f - view on LGTM.com

new alerts:

  • 1 for Unused import

@mspang
Copy link
Contributor Author

mspang commented Aug 22, 2020

@andy31415

@BroderickCarlin BroderickCarlin merged commit 1fcb6dd into project-chip:master Aug 24, 2020
@mspang mspang deleted the for-chip/generate-buildconfigs branch September 23, 2020 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants