-
Notifications
You must be signed in to change notification settings - Fork 6k
switched to using the ocmock build file in //build #17707
Conversation
|
LGTM |
shell/platform/darwin/ios/BUILD.gn
Outdated
| deps = [ | ||
| ":flutter_framework_source", | ||
| ":ocmock", | ||
| "//build/secondary/third_party/ocmock:ocmock", |
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.
build/secondary is redundant and is skipped elsewhere.
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'll do this in a sec after build lands so i can test it.
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.
done
|
|
||
| #include "flutter/shell/platform/darwin/common/framework/Headers/FlutterMacros.h" | ||
| #import "flutter/shell/platform/darwin/ios/framework/Source/SemanticsObject.h" | ||
| #import "third_party/ocmock/Source/OCMock/OCMock.h" |
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.
You could the OCMock target export a public config that adds //third_party/ocmock/Source to the inlcude dirs. That would make these source changes no longer necessary.
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 made this change on purpose because I think double quote includes relative to the project root are preferred to <> includes. It's a bit easier for maintainers and is less complicated.
Technically they could still use <> includes because i needed to enable them for OCMock to work (they use them in their own headers). That's why there is ocmock_config.
chinmaygarde
left a comment
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.
LGTM other than the nit about removing the redundant //build/secondary and of course the buildroot roll.
related issue: flutter/flutter#54503
related //build PR: flutter/buildroot#364 (DEPS should be updated to reflect this commit)