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

[gn] Simplify Support::CHIPMem build. #1592

Closed
wants to merge 1 commit into from

Conversation

turon
Copy link
Contributor

@turon turon commented Jul 14, 2020

Problem

When a source file is wrapped internally to only activate when the proper configuration macros are passed, then we can simplify the build to just include all files.

Summary of Changes

Perform such a simplification for support/CHIPMem

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

Copy link
Contributor

@andy31415 andy31415 left a comment

Choose a reason for hiding this comment

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

At some point we should pull out the #ifdefs from the alloc cpp files (after autotools is changed).

Copy link
Contributor

@andy31415 andy31415 left a comment

Choose a reason for hiding this comment

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

I actually read the patch in reverse: why do we want to compile the files and rely on #ifdef macros to include or not functions inside the alloc. This looks backwards to me - we should fix autotools instead.

@andy31415 andy31415 requested a review from mspang July 14, 2020 12:58
@andy31415
Copy link
Contributor

andy31415 commented Jul 14, 2020

I actually read the patch in reverse: why do we want to compile the files and rely on #ifdef macros to include or not functions inside the alloc. This looks backwards to me - we should fix autotools instead.

I will leave it to @mspang about best practices, but this seems backwards to me - if I see 'foo.cpp' being compiled, I assume it has useful code. if foo.cpp contains a '#ifdef FOO_ENABLED...' and then I compile and get nothing, that is very confusing.

@mspang
Copy link
Contributor

mspang commented Jul 14, 2020

It causes a warning on Mac if we build a static library containing a file with no symbols.

@mspang
Copy link
Contributor

mspang commented Jul 14, 2020

It causes a warning on Mac if we build a static library containing a file with no symbols.

Chrome has a wrapper for Mac's libtool to suppress these warnings, so another solution is to adopt such a wrapper to suppress the warning.

Copy link
Contributor

@mspang mspang left a comment

Choose a reason for hiding this comment

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

Marking request changes. We should have some way to avoid the warning.

Copy link
Contributor

@andy31415 andy31415 left a comment

Choose a reason for hiding this comment

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

Created #1595 to figure out some way to to standardize things or get a decision if the #ifdef pattern is a pattern we want to stick with.

Generally the pattern is alien to me (I expect that I only compile what I need and if I compile something, that something is generating output), however if this is somehow standard across many libraries I can live with it. Marking approve on this one for now.

@turon
Copy link
Contributor Author

turon commented Jul 14, 2020

I actually read the patch in reverse: why do we want to compile the files and rely on #ifdef macros to include or not functions inside the alloc. This looks backwards to me - we should fix autotools instead.

@andy31415 Thanks for opening #1595. @mspang I agree we need a workaround for empty file warnings on Mac -- any ideas on how to do that?

I'm not super opinionated on small adjustments like this, but I can say that large systems such as CHIP tend to have many ways to pass configuration:

  1. through the build system (thus building hard dependencies and maintenance requirements on those build systems that are supported)
  2. through C defines and config.h headers
  3. both

In order to better decouple the codebase from any particular build system, OpenThread as a project had agreed upon and moved to a model where most configuration was passed via (2). This PR is exploiting the fact that (2) is in place already for CHIPMem, and therefor decoupling that config from GN. For consistency, the PR should probably make the parallel updates to autotools as well.

@turon turon closed this Jul 14, 2020
@turon turon reopened this Jul 14, 2020
@github-actions
Copy link

Size increase report for "nrf-example-build"

File Section File VM
chip-nrf52840-lock-example.out .text 193692 193692
chip-nrf52840-lock-example.out .data -32 -32
chip-nrf52840-lock-example.out .bss 0 -4572
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.text,193692,193692
.debug_loc,0,88741
.strtab,0,76213
.debug_info,0,45293
.symtab,0,15888
[Unmapped],0,2916
.data,-32,-32
.debug_macro,0,-272
.bss,-4572,0
.debug_abbrev,0,-8251
.debug_aranges,0,-24224
.debug_ranges,0,-29944
.debug_line,0,-48999
.debug_frame,0,-62024
.debug_str,0,-191229


@github-actions
Copy link

Size increase report for "linux-example-build"

File Section File VM
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-standalone-demo.out and ./pull_artifact/chip-standalone-demo.out:

sections,vmsize,filesize


@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


@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


@mspang
Copy link
Contributor

mspang commented Jul 14, 2020

I actually read the patch in reverse: why do we want to compile the files and rely on #ifdef macros to include or not functions inside the alloc. This looks backwards to me - we should fix autotools instead.

@andy31415 Thanks for opening #1595. @mspang I agree we need a workaround for empty file warnings on Mac -- any ideas on how to do that?

Three ideas

  1. Wrap libtool with a script that filters out uninteresting warnings. Chrome does this.
  2. Do nothing? No warning if we conditionally compile the file.
  3. (hacky) Add some unused symbol to the file.

I'm not super opinionated on small adjustments like this, but I can say that large systems such as CHIP tend to have many ways to pass configuration:

  1. through the build system (thus building hard dependencies and maintenance requirements on those build systems that are supported)
  2. through C defines and config.h headers
  3. both

In order to better decouple the codebase from any particular build system, OpenThread as a project had agreed upon and moved to a model where most configuration was passed via (2). This PR is exploiting the fact that (2) is in place already for CHIPMem, and therefor decoupling that config from GN. For consistency, the PR should probably make the parallel updates to autotools as well.

@woody-apple
Copy link
Contributor

@andy31415 @mspang next steps?

@mspang
Copy link
Contributor

mspang commented Jul 22, 2020

@andy31415 @mspang next steps?

I would like to hold the line on avoiding uninteresting build output as this can drown out useful information. Here is the script Chrome uses to squelch uninteresting libtool warnings on Mac:

https://chromium.googlesource.com/chromium/src.git/+/master/build/toolchain/mac/filter_libtool.py

@woody-apple
Copy link
Contributor

@turon Can we address @mspang 's feedback here?

@turon
Copy link
Contributor Author

turon commented Jul 31, 2020

Closing until more general mechanisms for handling empty file warnings are in place.

@turon turon closed this Jul 31, 2020
jmartinez-silabs pushed a commit to SiliconLabs/matter that referenced this pull request Mar 18, 2024
…pick/device-argument to RC_2.3.0-1.3-alpha.3

Auto-Merge: Pull request project-chip#1628: [AUTO] Cherry-Pick Pull request project-chip#1592: Provision: Added 'device' argument.

Merge in WMN_TOOLS/matter from cherry-pick/device-argument to RC_2.3.0-1.3-alpha.3

Squashed commit of the following:

commit e65b44b0bdc456b7db4554580bdf7aebedab0b3d
Author: Ricardo Casallas <[email protected]>
Date:   Wed Feb 28 19:17:42 2024 +0000

    Pull request project-chip#1592: Provision: Added 'device' argument.

    Merge in WMN_TOOLS/matter from feature/provision_device_argument to RC_2.3.0-1.3

    Squashed commit of the following:

    commit 9f713da2c653c0400954ec14a40b45631a64cf3b
    Author: Ricardo Casallas <[email protected]>
    Date:   Tue Feb 27 11:56:04 2024 -0500

        Provision: Added 'device' argument.
jmartinez-silabs pushed a commit to SiliconLabs/matter that referenced this pull request May 8, 2024
Merge in WMN_TOOLS/matter from feature/provision_device_argument to RC_2.3.0-1.3

Squashed commit of the following:

commit 9f713da2c653c0400954ec14a40b45631a64cf3b
Author: Ricardo Casallas <[email protected]>
Date:   Tue Feb 27 11:56:04 2024 -0500

    Provision: Added 'device' argument.
rcasallas-silabs pushed a commit to rcasallas-silabs/connectedhomeip that referenced this pull request Jun 20, 2024
…pick/device-argument to RC_2.3.0-1.3-alpha.3

Auto-Merge: Pull request project-chip#1628: [AUTO] Cherry-Pick Pull request project-chip#1592: Provision: Added 'device' argument.

Merge in WMN_TOOLS/matter from cherry-pick/device-argument to RC_2.3.0-1.3-alpha.3

Squashed commit of the following:

commit e65b44b0bdc456b7db4554580bdf7aebedab0b3d
Author: Ricardo Casallas <[email protected]>
Date:   Wed Feb 28 19:17:42 2024 +0000

    Pull request project-chip#1592: Provision: Added 'device' argument.

    Merge in WMN_TOOLS/matter from feature/provision_device_argument to RC_2.3.0-1.3

    Squashed commit of the following:

    commit 9f713da2c653c0400954ec14a40b45631a64cf3b
    Author: Ricardo Casallas <[email protected]>
    Date:   Tue Feb 27 11:56:04 2024 -0500

        Provision: Added 'device' argument.
rcasallas-silabs added a commit to rcasallas-silabs/connectedhomeip that referenced this pull request Jun 20, 2024
Merge in WMN_TOOLS/matter from feature/provision_device_argument to RC_2.3.0-1.3

Squashed commit of the following:

commit 9f713da2c653c0400954ec14a40b45631a64cf3b
Author: Ricardo Casallas <[email protected]>
Date:   Tue Feb 27 11:56:04 2024 -0500

    Provision: Added 'device' argument.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants