-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Darwin compile under gn #5403
Darwin compile under gn #5403
Conversation
…ework files more often and avoid breakages
examples/chip-tool-darwin/main.m
Outdated
@interface CHIPToolPersistentStorageDelegate : NSObject <CHIPPersistentStorageDelegate> | ||
@end | ||
|
||
@implementation CHIPToolPersistentStorageDelegate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be worth a comment explaining why a stub implementation is ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
{ | ||
CHIP_ERROR err = CHIP_NO_ERROR; | ||
|
||
chip::SetupPayload setupPayload; | ||
|
||
if (duration > (uint32_t) -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no UINT32_MAX? Same for the other places where (uint32_t)-1 happens.
error = CHIP_ERROR_BUFFER_TOO_SMALL; | ||
} else { | ||
if (value != nullptr) { | ||
size = (uint16_t) strlcpy(value, [valueString UTF8String], size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should use [valueString lengthOfBytesUsingEncoding:NSUTF8StringEncoding]
?
#import <CHIP/gen/CHIPClustersObjc.h> | ||
#import <Foundation/Foundation.h> | ||
|
||
int main(int argc, const char * argv[]) { return EXIT_SUCCESS; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused.. this binary doesn't do anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the executable is to verify linking (i.e. no unresolved symbols)? Otherwise you can just build source_sets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
What about actually building the iOS example from GN? |
|
||
executable("chip-tool-darwin") { | ||
sources = [ | ||
"${chip_root}/src/app/clusters/media-playback-client/media-playback-client.cpp", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We ought to dedup explicitly listing all the bits in src/app/util using source_set dependencies.
And also for src/darwin/Framework/CHIP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 Short term goal is to reduce the number of breakages of 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 |
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).
|
* 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
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
examples/chip-tool-darwin
to be built with./gn_build.sh
on Mac