Force include lib/ files with full path #9387

kghost · 2021-09-01T04:02:04Z

Problem

There are two ways to include files under src/lib, #include <lib/core/xxx.h> or #include <core/xxx.h>. It turns out that mixing these two include may confuse #pragma once and causing redefinition error.

Change overview

Disallow using #include <core/xxx.h>. force use the full path with 'lib/'.

Testing

Verified using unit-tests

kghost · 2021-09-01T13:56:08Z

I use following command to convert includes:

for i in support core asn1 mdns shell; do git sed "#include \"$i/\\(.*\\)\"\$" "#include <lib/$i/\\1>" ; done
for i in support core asn1 mdns shell; do git sed "#include <$i/\\(.*\\)>\$" "#include <lib/$i/\\1>" ; done

woody-apple · 2021-09-01T14:01:05Z

Note, @kghost - this has real build failures.

examples/platform/nrfconnect/util/DFUTrigger.cpp

src/tools/chip-cert/chip-cert.h

src/lib/shell/MainLoopZephyr.cpp

src/lib/shell/streamer_zephyr.cpp

bzbarsky-apple · 2021-09-01T14:53:40Z

I use following command to convert includes:

Please:

Document in the commit message that these are the commands that were run.
Document in the commit message that the git-extras package is needed to run them.
Document in the commit message what manual fixups were then performed after running the commands.

bzbarsky-apple

So comparing this to the output of running the commands claimed and doing the fixups that are needed to fix the build:

Why the blank line additions to src/app/zap-templates/templates/app/af-structs.zapt?
Why the removal of the ASN1OID.h include from src/credentials/tests/CHIPCert_test_vectors.h?
Why the removal of the ASN1OID.h include from src/lib/asn1/tests/TestASN1.cpp?
Why the removal of the ASN1OID.h include from src/tools/chip-cert/chip-cert.h?

kghost · 2021-09-01T15:02:27Z

Document in the commit message what manual fixups were then performed after running the commands.

I'm already unable to trace manual fixes in the PR. It is around ten or twenty manual fixes.

I can make sure that only includes are changes, and their order due to restyle.

kghost · 2021-09-01T15:05:15Z

So comparing this to the output of running the commands claimed and doing the fixups that are needed to fix the build:

Why the blank line additions to src/app/zap-templates/templates/app/af-structs.zapt?

Prevent restyle from changing their order. Although the generate file is not processed by restyle, but we should follow the code style.

Why the removal of the ASN1OID.h include from src/credentials/tests/CHIPCert_test_vectors.h?

Why the removal of the ASN1OID.h include from src/lib/asn1/tests/TestASN1.cpp?

Why the removal of the ASN1OID.h include from src/tools/chip-cert/chip-cert.h?

ASN1OID.h is included by src/lib/asn1/ASN1.h inside chip::ASN1 namespace

namespace chip {
namespace ASN1 {

#ifndef DOXYGEN_SHOULD_SKIP_THIS
#include <asn1/ASN1OID.h>
#endif

Above includes doesn't put the file in the namespace, so causing compile error, it is probably a bug hidden behind the include order.

bzbarsky-apple · 2021-09-01T15:07:13Z

I'm already unable to trace manual fixes in the PR. It is around ten or twenty manual fixes.

There are manual fixes in 30 files or so, most of them in just a few categories. For future, to make this sort of thing reviewable, I would recomment one changeset with the script-generated mass-change, documented with what the scripts involved were (so reviewers can easily check that nothing else is in that changeset; in this case that scripted mass-change would be the sed scripts, restyled-io, and ZAP codegen) and one changeset with the manual changes that actually need to be manually reviewed....

I can make sure that only includes are changes, and their order due to restyle.

Right, given the scripts I manually verified things; that's where #9387 (review) comes from. :)

bzbarsky-apple · 2021-09-01T15:08:25Z

ASN1OID.h is included by src/lib/asn1/ASN1.h inside chip::ASN1 namespace

Oh, wow. That is ... not sane. Please file a followup issue to fix that and just put the right namespacing info in the generated file instead of doing that?

bzbarsky-apple

Approved on condition that the commit message is updated as I requested:

The scripts used
The git-extras package needed
General summary of manual changes (undo the changes for the SDK shell stuff, modify gn files to actually do the forcing of full path, stop including the IOD header outside of the namespace bits, that seems like about it?)

bzbarsky-apple · 2021-09-01T15:11:27Z

And thank you for doing this!

kghost · 2021-09-01T15:11:42Z

There are manual fixes in 30 files or so, most of them in just a few categories. For future, to make this sort of thing reviewable, I would recomment one changeset with the script-generated mass-change, documented with what the scripts involved were (so reviewers can easily check that nothing else is in that changeset; in this case that scripted mass-change would be the sed scripts, restyled-io, and ZAP codegen) and one changeset with the manual changes that actually need to be manually reviewed....

I'll split them into 2 patches, but the script generated patch will not compile, so it is unable to separate them into 2 PRs

Oh, wow. That is ... not sane. Please file a followup issue to fix that and just put the right namespacing info in the generated file instead of doing that?

Not problem

mspang · 2021-09-01T22:02:52Z

The reason this didn't happen in the last round of cleanups is that lib/ itself is inconsistent. What criteria defines whether something goes in lib/ or not? I can't think of one.

So there are two options

Add lib to includes. This still seems inconsistent.
Move the contents of lib/ up one level. In this case we don't need to change the vast majority of #include lines.

mspang · 2021-09-01T22:05:43Z

Move the contents of lib/ up one level. In this case we don't need to change the vast majority of #include lines.

Or option 3) move more code into lib/ in some principled way

But ideally our includes would actually look like this

#include <matter/...>

for which we might consider

A) Rename src/ to matter/
B) Leave things as-is

(this is orthogonal to whether lib/ is kept or not)

kghost · 2021-09-02T09:19:18Z

This goal of this PR is to disambiguate the include path of lib/core/xxx.h and core/xxx.h. It is not going to do any cleanups.

It is fine to re-arrange files, and do cleanups, in the following up PRs, but they have nothing to do with this PR.

andy31415

Approve assuming CI pases and changes are only include changes. I am unable to properly review because Github UI uses 100% CPU and fails to easily show me the deltas.

andy31415 · 2021-09-02T14:16:54Z

@kghost - the mbedos failure may be real.

This is auto generated by using following commands: for i in support core asn1 mdns shell; do git sed "#include \"$i/\$.*\$\"\$" "#include <lib/$i/\\1>" ; done for i in support core asn1 mdns shell; do git sed "#include <$i/\$.*\$>\$" "#include <lib/$i/\\1>" ; done Note: git-sed is in the git-extras package

github-actions · 2021-09-02T16:23:31Z

Size increase report for "esp32-example-build" from a8f02e2

File	Section	File	VM
chip-shell.elf	.flash.text	48	48
chip-ipv6only-app.elf	.flash.text	-172	-172
chip-bridge-app.elf	.flash.text	-48	-48

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.flash.text,48,48
.debug_str,0,-1
.debug_loc,0,-12
[Unmapped],0,-48
.debug_abbrev,0,-68
.debug_line,0,-215

Comparing ./master_artifact/chip-temperature-measurement-app.elf and ./pull_artifact/chip-temperature-measurement-app.elf:

sections,vmsize,filesize
.debug_info,0,11
.debug_loc,0,-27
.strtab,0,-48
.debug_abbrev,0,-136
.debug_line,0,-3800

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,8
.riscv.attributes,0,-1
.debug_abbrev,0,-2
.strtab,0,-48
.debug_loc,0,-86
.debug_line,0,-6687

Comparing ./master_artifact/chip-lock-app.elf and ./pull_artifact/chip-lock-app.elf:

sections,vmsize,filesize
.debug_info,0,7
.debug_str,0,1
.debug_abbrev,0,-2
.strtab,0,-48
.debug_loc,0,-189
.debug_line,0,-4149

Comparing ./master_artifact/chip-persistent-storage.elf and ./pull_artifact/chip-persistent-storage.elf:

sections,vmsize,filesize
.debug_info,0,3
.debug_loc,0,3
.debug_abbrev,0,-22

Comparing ./master_artifact/chip-ipv6only-app.elf and ./pull_artifact/chip-ipv6only-app.elf:

sections,vmsize,filesize
.flash.text,-172,-172
[Unmapped],0,-3924

Comparing ./master_artifact/chip-bridge-app.elf and ./pull_artifact/chip-bridge-app.elf:

sections,vmsize,filesize
[Unmapped],0,48
.debug_loc,0,13
.debug_info,0,11
.flash.text,-48,-48
.strtab,0,-48
.debug_abbrev,0,-136
.debug_line,0,-4260

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize

mspang · 2021-09-02T21:12:29Z

Approve assuming CI pases and changes are only include changes. I am unable to properly review because Github UI uses 100% CPU and fails to easily show me the deltas.

You can download the patch..

bzbarsky-apple · 2021-09-04T08:22:14Z

Indeed. And given that @kghost split this up into multiple changesets, can verify that the script-generated changeset matches the script output and review the small manual bits.... That's what I ended up doing.

boring-cyborg bot added the examples label Sep 1, 2021

restyled-io bot mentioned this pull request Sep 1, 2021

Restyle Force include lib/ files with full path #9388

Closed

kghost force-pushed the include branch from 78e2270 to 2782f5c Compare September 1, 2021 04:04

pullapprove bot requested review from chrisdecenzo, jelderton, jmartinez-silabs, LuDuda, mspang, saurabhst and woody-apple September 1, 2021 04:05

pullapprove bot added the review - pending label Sep 1, 2021

kghost force-pushed the include branch from 2782f5c to 0af4f8c Compare September 1, 2021 08:31

woody-apple added the SDK Approved label Sep 1, 2021

kghost force-pushed the include branch from 0af4f8c to ed37c9b Compare September 1, 2021 14:36

bzbarsky-apple reviewed Sep 1, 2021

View reviewed changes

examples/platform/nrfconnect/util/DFUTrigger.cpp Outdated Show resolved Hide resolved

src/tools/chip-cert/chip-cert.h Show resolved Hide resolved

src/lib/shell/MainLoopZephyr.cpp Outdated Show resolved Hide resolved

src/lib/shell/streamer_zephyr.cpp Outdated Show resolved Hide resolved

kghost force-pushed the include branch from ed37c9b to cb6a884 Compare September 1, 2021 14:52

bzbarsky-apple reviewed Sep 1, 2021

View reviewed changes

bzbarsky-apple approved these changes Sep 1, 2021

View reviewed changes

kghost force-pushed the include branch 3 times, most recently from b2aec2f to 1957463 Compare September 1, 2021 15:24

kghost mentioned this pull request Sep 2, 2021

Define namespace chip inside asn1/ASN1OID.h #9402

Merged

kghost force-pushed the include branch from 1957463 to 9ee99cf Compare September 2, 2021 09:25

andy31415 approved these changes Sep 2, 2021

View reviewed changes

kghost force-pushed the include branch from 9ee99cf to 781b760 Compare September 2, 2021 14:19

kghost added 2 commits September 2, 2021 22:24

Restyle

f76de53

kghost force-pushed the include branch 2 times, most recently from 8e77155 to da0805f Compare September 2, 2021 14:54

Manual fixes

70923ac

kghost force-pushed the include branch from da0805f to 70923ac Compare September 2, 2021 15:07

yufengwangca approved these changes Sep 2, 2021

View reviewed changes

saurabhst approved these changes Sep 2, 2021

View reviewed changes

pullapprove bot added review - approved and removed review - pending labels Sep 2, 2021

jmartinez-silabs approved these changes Sep 2, 2021

View reviewed changes

andy31415 merged commit 53dd583 into project-chip:master Sep 2, 2021

kghost deleted the include branch September 4, 2021 08:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Force include lib/ files with full path #9387

Force include lib/ files with full path #9387

kghost commented Sep 1, 2021

kghost commented Sep 1, 2021

woody-apple commented Sep 1, 2021

bzbarsky-apple commented Sep 1, 2021

bzbarsky-apple left a comment

kghost commented Sep 1, 2021

kghost commented Sep 1, 2021 •

edited

Loading

bzbarsky-apple commented Sep 1, 2021

bzbarsky-apple commented Sep 1, 2021

bzbarsky-apple left a comment

bzbarsky-apple commented Sep 1, 2021

kghost commented Sep 1, 2021

mspang commented Sep 1, 2021 •

edited

Loading

mspang commented Sep 1, 2021 •

edited

Loading

kghost commented Sep 2, 2021 •

edited

Loading

andy31415 left a comment

andy31415 commented Sep 2, 2021

github-actions bot commented Sep 2, 2021

mspang commented Sep 2, 2021

bzbarsky-apple commented Sep 4, 2021

Force include lib/ files with full path #9387

Force include lib/ files with full path #9387

Conversation

kghost commented Sep 1, 2021

Problem

Change overview

Testing

kghost commented Sep 1, 2021

woody-apple commented Sep 1, 2021

bzbarsky-apple commented Sep 1, 2021

bzbarsky-apple left a comment

Choose a reason for hiding this comment

kghost commented Sep 1, 2021

kghost commented Sep 1, 2021 • edited Loading

bzbarsky-apple commented Sep 1, 2021

bzbarsky-apple commented Sep 1, 2021

bzbarsky-apple left a comment

Choose a reason for hiding this comment

bzbarsky-apple commented Sep 1, 2021

kghost commented Sep 1, 2021

mspang commented Sep 1, 2021 • edited Loading

mspang commented Sep 1, 2021 • edited Loading

kghost commented Sep 2, 2021 • edited Loading

andy31415 left a comment

Choose a reason for hiding this comment

andy31415 commented Sep 2, 2021

github-actions bot commented Sep 2, 2021

mspang commented Sep 2, 2021

bzbarsky-apple commented Sep 4, 2021

kghost commented Sep 1, 2021 •

edited

Loading

mspang commented Sep 1, 2021 •

edited

Loading

mspang commented Sep 1, 2021 •

edited

Loading

kghost commented Sep 2, 2021 •

edited

Loading