Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1803423002:
[iOS] Define bundle_data target for //net:net_unittests. (Closed)
Description[iOS] Define bundle_data target for //net:net_unittests.
This ensure that the files required by net_unittests application
will be copied into the application bundle on iOS.
Synchronise the list of data files required for net_unittests
target in gyp and gn.
BUG=297668
Committed: https://crrev.com/b0a316459b6513356a05f1198daeceed6d196494
Cr-Commit-Position: refs/heads/master@{#381932}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Address comments #Patch Set 3 : Rebase on origin/master #Messages
Total messages: 17 (7 generated)
sdefresne@chromium.org changed reviewers: + dpranke@chromium.org, mattm@chromium.org
dpranke: Please take a look. mattm: Could you review as OWNERS?
https://codereview.chromium.org/1803423002/diff/1/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/1803423002/diff/1/net/BUILD.gn#newcode703 net/BUILD.gn:703: # net/test/cert_test_util.cc depends on the test data, so this needs Any idea why this wasn't a problem in gyp? I guess in gyp the other tests that needed it copied it themselves?: https://code.google.com/p/chromium/codesearch#chromium/src/ios/web/ios_web_un... https://code.google.com/p/chromium/codesearch#chromium/src/components/compone... Should we modify gyp to match this or vice versa? https://codereview.chromium.org/1803423002/diff/1/net/BUILD.gn#newcode1383 net/BUILD.gn:1383: "data", I think we should maintain symmetry between gyp and gn in whether to list individual directories or the whole data dir. (Re: https://code.google.com/p/chromium/codesearch#chromium/src/net/net.gyp&l=371)
lgtm w/ comments addressed. https://codereview.chromium.org/1803423002/diff/1/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/1803423002/diff/1/net/BUILD.gn#newcode1383 net/BUILD.gn:1383: "data", On 2016/03/16 20:28:33, mattm wrote: > I think we should maintain symmetry between gyp and gn in whether to list > individual directories or the whole data dir. (Re: > https://code.google.com/p/chromium/codesearch#chromium/src/net/net.gyp&l=371) Agreed we should probably match lists to avoid confusion. Also, directories should end in a trailing "/".
Description was changed from ========== [iOS] Define bundle_data target for //net:net_unittests. This ensure that the files required by net_unittests application will be copied into the application bundle on iOS. BUG=297668 ========== to ========== [iOS] Define bundle_data target for //net:net_unittests. This ensure that the files required by net_unittests application will be copied into the application bundle on iOS. Synchronise the list of data files required for net_unittests target in gyp and all gn platforms. BUG=297668 ==========
Patchset #2 (id:20001) has been deleted
Description was changed from ========== [iOS] Define bundle_data target for //net:net_unittests. This ensure that the files required by net_unittests application will be copied into the application bundle on iOS. Synchronise the list of data files required for net_unittests target in gyp and all gn platforms. BUG=297668 ========== to ========== [iOS] Define bundle_data target for //net:net_unittests. This ensure that the files required by net_unittests application will be copied into the application bundle on iOS. Synchronise the list of data files required for net_unittests target in gyp and gn. BUG=297668 ==========
Please take another look. https://codereview.chromium.org/1803423002/diff/1/net/BUILD.gn File net/BUILD.gn (right): https://codereview.chromium.org/1803423002/diff/1/net/BUILD.gn#newcode703 net/BUILD.gn:703: # net/test/cert_test_util.cc depends on the test data, so this needs On 2016/03/16 at 20:28:33, mattm wrote: > Any idea why this wasn't a problem in gyp? > > I guess in gyp the other tests that needed it copied it themselves?: > https://code.google.com/p/chromium/codesearch#chromium/src/ios/web/ios_web_un... > https://code.google.com/p/chromium/codesearch#chromium/src/components/compone... > > Should we modify gyp to match this or vice versa? With gyp, due to how the files are moved into the test application bundle, there was not way to have a single target cause the correct file to be copied to the correct location in the application bundle (this is only true for tests as they use build/copy_test_data_ios.gypi template that requires knowing the name of the final application). This is not true with gn (since I did take this into consideration when designing the support for bundle in gn). So, sadly, no there is no way to have them match. I can however split net_unittests_bundle_data in two, one that contains only net/data/ssl/certificates (for test_support) and the other that contains the data required for net_unittests only. WDYT? https://codereview.chromium.org/1803423002/diff/1/net/BUILD.gn#newcode1383 net/BUILD.gn:1383: "data", On 2016/03/16 at 22:02:20, Dirk Pranke wrote: > On 2016/03/16 20:28:33, mattm wrote: > > I think we should maintain symmetry between gyp and gn in whether to list > > individual directories or the whole data dir. (Re: > > https://code.google.com/p/chromium/codesearch#chromium/src/net/net.gyp&l=371) > > Agreed we should probably match lists to avoid confusion. Also, directories should end in a trailing "/". Agreed for keeping the list in sync. As mentioned in https://codereview.chromium.org/1806033002/diff/1/ui/gfx/BUILD.gn using a trailing "/" is currently not possible. I'll see how I can fix this in gn, but if possible, I'd like to land the CL as is and come back later to convert to a trailing "/".
On 2016/03/17 16:02:22, sdefresne wrote: > As mentioned in > https://codereview.chromium.org/1806033002/diff/1/ui/gfx/BUILD.gn using a > trailing "/" is currently not possible. I'll see how I can fix this in gn, but > if possible, I'd like to land the CL as is and come back later to convert to a > trailing "/". Landing as-is is fine, but I would like to fix this in GN if possible so that we can be consistent ...
On 2016/03/17 at 17:22:59, dpranke wrote: > On 2016/03/17 16:02:22, sdefresne wrote: > > As mentioned in > > https://codereview.chromium.org/1806033002/diff/1/ui/gfx/BUILD.gn using a > > trailing "/" is currently not possible. I'll see how I can fix this in gn, but > > if possible, I'd like to land the CL as is and come back later to convert to a > > trailing "/". > > Landing as-is is fine, but I would like to fix this in GN if possible so that > we can be consistent ... Thanks, created https://bugs.chromium.org/p/chromium/issues/detail?id=595769 to track this.
lgtm
The CQ bit was checked by sdefresne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, mattm@chromium.org Link to the patchset: https://codereview.chromium.org/1803423002/#ps60001 (title: "Rebase on origin/master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1803423002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1803423002/60001
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [iOS] Define bundle_data target for //net:net_unittests. This ensure that the files required by net_unittests application will be copied into the application bundle on iOS. Synchronise the list of data files required for net_unittests target in gyp and gn. BUG=297668 ========== to ========== [iOS] Define bundle_data target for //net:net_unittests. This ensure that the files required by net_unittests application will be copied into the application bundle on iOS. Synchronise the list of data files required for net_unittests target in gyp and gn. BUG=297668 Committed: https://crrev.com/b0a316459b6513356a05f1198daeceed6d196494 Cr-Commit-Position: refs/heads/master@{#381932} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b0a316459b6513356a05f1198daeceed6d196494 Cr-Commit-Position: refs/heads/master@{#381932} |
