Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1879493002:
[iOS/GN] Convert //ios/third_party/ochamcrest to build a framework. (Closed)
Description[iOS/GN] Convert //ios/third_party/ochamcrest to build a framework.
Add support for copying public headers to the ios_framework_bundle
template and use this to implement the //ios/third_party/ochamcrest
target.
Add a set_defaults for ios_framework_bundle and mac_framework_bundle
to build/config/BUILDCONFIG.gn so that overriding "configs" works as
expected.
BUG=599322
Committed: https://crrev.com/8fab21c694073ca4db23495b715770385c597e1a
Cr-Commit-Position: refs/heads/master@{#386957}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Use set_defaults and address comments/nits #
Total comments: 4
Patch Set 3 : Use set_sources_assignment_filter #
Total comments: 1
Patch Set 4 : Add a comment to explain the "set_sources_assignment_filter" in "ios_framework_bundle" template #
Depends on Patchset: Messages
Total messages: 22 (9 generated)
sdefresne@chromium.org changed reviewers: + rsesek@chromium.org
Please take a look. dpranke,brettw: FYI
dpranke@chromium.org changed reviewers: + dpranke@chromium.org
lgtm https://codereview.chromium.org/1879493002/diff/1/build/config/mac/rules.gni File build/config/mac/rules.gni (right): https://codereview.chromium.org/1879493002/diff/1/build/config/mac/rules.gni#... build/config/mac/rules.gni:52: if (defined(invoker.configs)) { nit: can you add a comment here referencing the bugs as well to explain this weirdness?
https://codereview.chromium.org/1879493002/diff/1/build/config/ios/rules.gni File build/config/ios/rules.gni (right): https://codereview.chromium.org/1879493002/diff/1/build/config/ios/rules.gni#... build/config/ios/rules.gni:258: _target_name = target_name optional: pull _target_name and _output_name out of the if, and use _target_name in framework_bundle() below. https://codereview.chromium.org/1879493002/diff/1/build/config/ios/rules.gni#... build/config/ios/rules.gni:265: _framework_name = _output_name + ".framework" I wish we didn't have to duplicate this, but I don't see another way. https://codereview.chromium.org/1879493002/diff/1/build/config/ios/rules.gni#... build/config/ios/rules.gni:271: action(_compile_headers_map_target) { Should each of these sub-targets have visibility set to limit their access? https://codereview.chromium.org/1879493002/diff/1/build/config/mac/rules.gni File build/config/mac/rules.gni (right): https://codereview.chromium.org/1879493002/diff/1/build/config/mac/rules.gni#... build/config/mac/rules.gni:52: if (defined(invoker.configs)) { On 2016/04/11 16:38:03, Dirk Pranke wrote: > nit: can you add a comment here referencing the bugs as well to explain this > weirdness? Also document this up top as an argument?
Oh and lgtm % nits
Description was changed from ========== [iOS/GN] Convert //ios/third_party/ochamcrest to build a framework. Add support for copying public headers to the ios_framework_bundle template and use this to implement the //ios/third_party/ochamcrest target. Fix framework_bundle to allow caller to override the "configs" and add a workaround to allow removal of some "configs" (needed by the ochamcrest target to remove -fsymbol-visibility=hidden). BUG=602217,602209,599322 ========== to ========== [iOS/GN] Convert //ios/third_party/ochamcrest to build a framework. Add support for copying public headers to the ios_framework_bundle template and use this to implement the //ios/third_party/ochamcrest target. Add a set_defaults for ios_framework_bundle and mac_framework_bundle to build/config/BUILDCONFIG.gn so that overriding "configs" works as expected. BUG=599322 ==========
sdefresne@chromium.org changed reviewers: + brettw@chromium.org
brettw: Please take a look at build/config/BUILDCONFIG.gn.
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/1879493002/diff/1/build/config/ios/rules.gni File build/config/ios/rules.gni (right): https://codereview.chromium.org/1879493002/diff/1/build/config/ios/rules.gni#... build/config/ios/rules.gni:258: _target_name = target_name On 2016/04/11 at 18:32:28, Robert Sesek wrote: > optional: pull _target_name and _output_name out of the if, and use _target_name in framework_bundle() below. I tried to pull _output_name outside of the "if" but this resulted in errors about unused variables. The only variable I could move out is "_target_name" and I'm not sure it is worth it, so not done. https://codereview.chromium.org/1879493002/diff/1/build/config/ios/rules.gni#... build/config/ios/rules.gni:265: _framework_name = _output_name + ".framework" On 2016/04/11 at 18:32:28, Robert Sesek wrote: > I wish we didn't have to duplicate this, but I don't see another way. Acknowledge. One option would be to change framework_bundle to accept a framework_name argument that would override the value used in the template, but then it would be confusing (i.e. what if I define both output_name and framework_name). https://codereview.chromium.org/1879493002/diff/1/build/config/ios/rules.gni#... build/config/ios/rules.gni:271: action(_compile_headers_map_target) { On 2016/04/11 at 18:32:29, Robert Sesek wrote: > Should each of these sub-targets have visibility set to limit their access? Done, except for "_create_module_map_target" as I do not know how to limit visibility to all targets generated by "framework_bundle" template. https://codereview.chromium.org/1879493002/diff/1/build/config/mac/rules.gni File build/config/mac/rules.gni (right): https://codereview.chromium.org/1879493002/diff/1/build/config/mac/rules.gni#... build/config/mac/rules.gni:52: if (defined(invoker.configs)) { On 2016/04/11 at 18:32:29, Robert Sesek wrote: > On 2016/04/11 16:38:03, Dirk Pranke wrote: > > nit: can you add a comment here referencing the bugs as well to explain this > > weirdness? > > Also document this up top as an argument? Removed the need for the workaround by using set_defaults, so not done.
https://codereview.chromium.org/1879493002/diff/40001/build/config/ios/rules.gni File build/config/ios/rules.gni (right): https://codereview.chromium.org/1879493002/diff/40001/build/config/ios/rules.... build/config/ios/rules.gni:280: foreach(_source_file, invoker.sources) { brettw: is there a way to perform this filtering using native functions instead of using "foreach" loop and "get_path_info"?
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/1879493002/diff/60001/build/config/ios/rules.gni File build/config/ios/rules.gni (right): https://codereview.chromium.org/1879493002/diff/60001/build/config/ios/rules.... build/config/ios/rules.gni:248: # Seems like you need "sources" here. https://codereview.chromium.org/1879493002/diff/60001/build/config/ios/rules.... build/config/ios/rules.gni:280: foreach(_source_file, invoker.sources) { Yuck. There is a bunch of Python code in GYP to hard-code this case. This gets back to what the sources are defined to be. It looks like you never use the invoker.sources variable except here. Can this just be a "headers" variable instead? This structure would make sense if this template is going to be used by another template that uses the sources for something else in addition to passing them in here. How many files do we expect this to happen to? Is this a handful of files per framework, or does this end up being most sources? If this is the former, I would say we should change gyp-mac-tool to ignore source files in the input and pass the whole sources list. If we're going to have a whole lot of files (like thousands) we probably don't want to add those extra build edges. You can do this internally using set_sources_assignment_filter and removing non-header file source types: set_sources_assignment_filter(["*.c", "*.cc", "*.cpp", "*.m", "*.mm"]) sources = invoker.sources set_sources_assignment_filter([]) I don't want to really encourage using the source assignment filter unless we really need to. I want to delete it long-term. But if we do need this functionality, I could envision us replacing the sources assignment filter with a function that explicitly filters based on some pattern.
PTAL https://codereview.chromium.org/1879493002/diff/60001/build/config/ios/rules.gni File build/config/ios/rules.gni (right): https://codereview.chromium.org/1879493002/diff/60001/build/config/ios/rules.... build/config/ios/rules.gni:248: # On 2016/04/12 at 17:43:12, brettw wrote: > Seems like you need "sources" here. Done. https://codereview.chromium.org/1879493002/diff/60001/build/config/ios/rules.... build/config/ios/rules.gni:280: foreach(_source_file, invoker.sources) { On 2016/04/12 at 17:43:12, brettw wrote: > Yuck. There is a bunch of Python code in GYP to hard-code this case. > > This gets back to what the sources are defined to be. It looks like you never use the invoker.sources variable except here. Can this just be a "headers" variable instead? > > This structure would make sense if this template is going to be used by another template that uses the sources for something else in addition to passing them in here. > > How many files do we expect this to happen to? Is this a handful of files per framework, or does this end up being most sources? > > If this is the former, I would say we should change gyp-mac-tool to ignore source files in the input and pass the whole sources list. > > If we're going to have a whole lot of files (like thousands) we probably don't want to add those extra build edges. You can do this internally using set_sources_assignment_filter and removing non-header file source types: > set_sources_assignment_filter(["*.c", "*.cc", "*.cpp", "*.m", "*.mm"]) > sources = invoker.sources > set_sources_assignment_filter([]) > > I don't want to really encourage using the source assignment filter unless we really need to. I want to delete it long-term. But if we do need this functionality, I could envision us replacing the sources assignment filter with a function that explicitly filters based on some pattern. I assign "sources" to "shared_library" target in "framework_bundle" bundle invocation below. This ends up being the list of files used to build the shared library. I got strong push-back when I asked the "sources" to be split in "headers" + "sources" in the corresponding gyp file, as being to error prone. Changed to use "set_sources_assignment_filter" for now.
lgtm https://codereview.chromium.org/1879493002/diff/80001/build/config/ios/rules.gni File build/config/ios/rules.gni (right): https://codereview.chromium.org/1879493002/diff/80001/build/config/ios/rules.... build/config/ios/rules.gni:286: set_sources_assignment_filter([ Can you comment the high-level thing you're doing here (i.e. you want only the .h files from the sources).
The CQ bit was checked by sdefresne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, dpranke@chromium.org, brettw@chromium.org Link to the patchset: https://codereview.chromium.org/1879493002/#ps100001 (title: "Add a comment to explain the "set_sources_assignment_filter" in "ios_framework_bundle" template")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1879493002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1879493002/100001
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [iOS/GN] Convert //ios/third_party/ochamcrest to build a framework. Add support for copying public headers to the ios_framework_bundle template and use this to implement the //ios/third_party/ochamcrest target. Add a set_defaults for ios_framework_bundle and mac_framework_bundle to build/config/BUILDCONFIG.gn so that overriding "configs" works as expected. BUG=599322 ========== to ========== [iOS/GN] Convert //ios/third_party/ochamcrest to build a framework. Add support for copying public headers to the ios_framework_bundle template and use this to implement the //ios/third_party/ochamcrest target. Add a set_defaults for ios_framework_bundle and mac_framework_bundle to build/config/BUILDCONFIG.gn so that overriding "configs" works as expected. BUG=599322 Committed: https://crrev.com/8fab21c694073ca4db23495b715770385c597e1a Cr-Commit-Position: refs/heads/master@{#386957} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8fab21c694073ca4db23495b715770385c597e1a Cr-Commit-Position: refs/heads/master@{#386957} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
