Darwin compile under gn #5403

vivien-apple · 2021-03-16T17:56:00Z

Problem

This PR adds examples/chip-tool-darwin as a build target when ones runs ./gn_build.sh.

The specific goal here is to avoid the various src/darwin/Framework breakages that happens because it does not run in CI and because we do not build the iOS app all day.

Basically this PR builds the src/darwin/Framework/CHIP files and it should help us catch regression in a faster way since a few of us are building using macOS all day long.

Summary of Changes

Add examples/chip-tool-darwin to be built with ./gn_build.sh on Mac

…ework files more often and avoid breakages

bzbarsky-apple · 2021-03-16T17:59:16Z

examples/chip-tool-darwin/main.m

+@interface CHIPToolPersistentStorageDelegate : NSObject <CHIPPersistentStorageDelegate>
+@end
+
+@implementation CHIPToolPersistentStorageDelegate


Might be worth a comment explaining why a stub implementation is ok.

I guess we can remove most of the content of main.m. I did this code mostly for checking missing headers but that's probably OK to remove all of it now.

bzbarsky-apple · 2021-03-16T18:00:06Z

src/darwin/Framework/CHIP/CHIPDevice.mm

 {
    CHIP_ERROR err = CHIP_NO_ERROR;

    chip::SetupPayload setupPayload;

+    if (duration > (uint32_t) -1) {


There's no UINT32_MAX? Same for the other places where (uint32_t)-1 happens.

bzbarsky-apple · 2021-03-16T18:07:26Z

src/darwin/Framework/CHIP/CHIPPersistentStorageDelegateBridge.mm

+                error = CHIP_ERROR_BUFFER_TOO_SMALL;
+            } else {
+                if (value != nullptr) {
+                    size = (uint16_t) strlcpy(value, [valueString UTF8String], size);


Is this right? I would think [valueString length] is the number of 16-bit UTF-16 units, which tells you very little about the length of the UTF8String representation, unless we know the string is ASCII.

I guess we already had this issue with the existing size < [valueString length] check?

Maybe we should use [valueString lengthOfBytesUsingEncoding:NSUTF8StringEncoding]?

mspang · 2021-03-16T22:09:14Z

examples/chip-tool-darwin/main.m

+#import <CHIP/gen/CHIPClustersObjc.h>
+#import <Foundation/Foundation.h>
+
+int main(int argc, const char * argv[]) { return EXIT_SUCCESS; }


I'm confused.. this binary doesn't do anything?

I guess the executable is to verify linking (i.e. no unresolved symbols)? Otherwise you can just build source_sets.

I guess the executable is to verify linking (i.e. no unresolved symbols)? Otherwise you can just build source_sets.

Yes. 2 of the last breakages were due to missing symbols.

mspang · 2021-03-16T22:30:41Z

What about actually building the iOS example from GN?

mspang · 2021-03-16T22:33:58Z

examples/chip-tool-darwin/BUILD.gn

+
+executable("chip-tool-darwin") {
+  sources = [
+    "${chip_root}/src/app/clusters/media-playback-client/media-playback-client.cpp",


We ought to dedup explicitly listing all the bits in src/app/util using source_set dependencies.

And also for src/darwin/Framework/CHIP

Do you have good suggestion for that ?
My understanding is that I should normally create a BUILD.gn file into src/app/ or src/app/util with something like source_set("chip-app-util") { ...} into it, and then add public_deps += ["${chip_root}/src/app/util:chip-app-util"] to examples/*/BUILD.gn files.

But this naive approach won't work because some of the files from src/app depends on examples/*/gen/ folders :/

Ah I see. There are still some options to dedup, such as making a template and invoking it from each application build file, but no silver bullet.

mspang · 2021-03-16T23:04:32Z

If I'm understanding the goal correctly, my main question this is just whether it goes far enough. For example I'd expect this be compiled with an ios toolchain rather than a mac toolchain. Does it not matter?

vivien-apple · 2021-03-17T08:45:35Z

If I'm understanding the goal correctly, my main question this is just whether it goes far enough. For example I'd expect this be compiled with an ios toolchain rather than a mac toolchain. Does it not matter?

I agree it does not goes far enough. Ideally we would use command line Xcode to build the src/darwin/Framework/ and src/darwin/ChipTool, and then run the tests from those folders.

Short term goal is to reduce the number of breakages of src/darwin/Framework (since this is where most of the breakages seems to appears) and to reduce the latency between a breakage and a fix.

Medium term goal would be to use the right toolchain to build those folders properly.

Longer term goal would be to also run the tests, and add more tests since I believe the src/darwin tests are not runned very much.

mspang · 2021-03-17T16:42:52Z

If I'm understanding the goal correctly, my main question this is just whether it goes far enough. For example I'd expect this be compiled with an ios toolchain rather than a mac toolchain. Does it not matter?

I agree it does not goes far enough. Ideally we would use command line Xcode to build the src/darwin/Framework/ and src/darwin/ChipTool, and then run the tests from those folders.

It would be good to automatically produce test iOS applications from each chip_test_suite(). If using XCode to build, I'd wonder if it would be fast enough (versus doing a native iOS application build from GN like Chrome does).

Short term goal is to reduce the number of breakages of src/darwin/Framework (since this is where most of the breakages seems to appears) and to reduce the latency between a breakage and a fix.

Medium term goal would be to use the right toolchain to build those folders properly.

Longer term goal would be to also run the tests, and add more tests since I believe the src/darwin tests are not runned very much.

* src/darwin/Framework: error: implicit conversion loses integer precision * Add src/examples/chip-tool-darwin in order to compile src/darwin/Framework files more often and avoid breakages * Address some review comments and update some .gn files style

vivien-apple added 2 commits March 16, 2021 17:52

src/darwin/Framework: error: implicit conversion loses integer precision

1b61e11

Add src/examples/chip-tool-darwin in order to compile src/darwin/Fram…

d55a350

…ework files more often and avoid breakages

vivien-apple self-assigned this Mar 16, 2021

boring-cyborg bot added darwin examples labels Mar 16, 2021

restyled-io bot mentioned this pull request Mar 16, 2021

Restyle Darwin compile under gn #5404

Closed

bzbarsky-apple requested changes Mar 16, 2021

View reviewed changes

Address some review comments and update some .gn files style

4b5ef54

vivien-apple requested a review from bzbarsky-apple March 16, 2021 18:25

pullapprove bot requested review from andy31415, chrisdecenzo, Damian-Nordic, hawk248, jepenven-silabs, msandstedt and rwalker-apple March 16, 2021 18:27

bzbarsky-apple approved these changes Mar 16, 2021

View reviewed changes

mspang reviewed Mar 16, 2021

View reviewed changes

msandstedt approved these changes Mar 17, 2021

View reviewed changes

mspang approved these changes Mar 17, 2021

View reviewed changes

mspang merged commit 87d46d5 into project-chip:master Mar 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Darwin compile under gn #5403

Darwin compile under gn #5403

vivien-apple commented Mar 16, 2021

bzbarsky-apple Mar 16, 2021

vivien-apple Mar 16, 2021

bzbarsky-apple Mar 16, 2021

bzbarsky-apple Mar 16, 2021

bzbarsky-apple Mar 16, 2021

mspang Mar 16, 2021

mspang Mar 16, 2021

vivien-apple Mar 17, 2021

mspang commented Mar 16, 2021

mspang Mar 16, 2021

vivien-apple Mar 17, 2021

mspang Mar 17, 2021

mspang commented Mar 16, 2021

vivien-apple commented Mar 17, 2021

mspang commented Mar 17, 2021

Darwin compile under gn #5403

Darwin compile under gn #5403

Conversation

vivien-apple commented Mar 16, 2021

Problem

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mspang commented Mar 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mspang commented Mar 16, 2021

vivien-apple commented Mar 17, 2021

mspang commented Mar 17, 2021