Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
588963002:
Start the process of renaming ui_unittests to ui_base_unittests. (Closed)
DescriptionStart the process of renaming ui_unittests to ui_base_unittests.
BUG=331829, 373148, 299841, 103304
TEST=ninja -C out/Debug ui_unittests, then verify that
out/Debug/ui_base_unittests is there and runs. Same thing with
ui_unittests
R=sky@chromium.org
Committed: https://crrev.com/b5e7760a55f0450b2c9fb615b0aa0f030443a57e
Cr-Commit-Position: refs/heads/master@{#296182}
Patch Set 1 #Patch Set 2 : update the TODO #Patch Set 3 : fix android? #Patch Set 4 : one more try for android #Patch Set 5 : fix it? #
Total comments: 2
Patch Set 6 : don't try to copy on android - there it is different #
Total comments: 4
Patch Set 7 : refactor conditions #Patch Set 8 : more android changes :) #
Total comments: 2
Patch Set 9 : back with conditions #Patch Set 10 : try identical targets - first with android #Patch Set 11 : all or nothing - ALL IN #Patch Set 12 : fix clobber build of ui_unittests on Android - crbug.com/374490 #
Total comments: 4
Patch Set 13 : add ui_base_tests.gypi - share the body between the old and the new target #Patch Set 14 : add more TODOS #
Messages
Total messages: 32 (5 generated)
PTAL.
tfarina@chromium.org changed reviewers: + rohitrao@chromium.org
rohitrao: Please, review the iOS part. Thanks.
Copying just the executable file by itself doesn't work, does it? There are other things in the .app bundle, such as Info.plist and any test data, that almost certainly have to be copied too. Have we successfully used this method to rename an ios test in the past?
On 2014/09/21 16:51:29, rohitrao wrote: > Copying just the executable file by itself doesn't work, does it? There are > other things in the .app bundle, such as Info.plist and any test data, that > almost certainly have to be copied too. > Ah! Thanks for catching that! ui_unittests certainly has more things to be copied. We copy test data to ios and we also have this .plist too. :( Unfortunately I don't have a Mac to verify this. Could you check locally for me? > Have we successfully used this method to rename an ios test in the past? I think it is the first time. googleurl_unittests haven't had data to be copied nor any Info.plist file.
tfarina@chromium.org changed reviewers: + cjhopman@chromium.org
Chris, could you review the android changes? Thanks.
https://codereview.chromium.org/588963002/diff/80001/ui/base/ui_base_tests.gyp File ui/base/ui_base_tests.gyp (right): https://codereview.chromium.org/588963002/diff/80001/ui/base/ui_base_tests.gy... ui/base/ui_base_tests.gyp:247: 'source_file': '<(PRODUCT_DIR)/ui_base_unittests<(EXECUTABLE_SUFFIX)', It the only difference is in the variables, why not move the conditions in there? https://codereview.chromium.org/588963002/diff/100001/ui/base/ui_base_tests.gyp File ui/base/ui_base_tests.gyp (right): https://codereview.chromium.org/588963002/diff/100001/ui/base/ui_base_tests.g... ui/base/ui_base_tests.gyp:286: }, For android you probably have to copy <(PRODUCT_DIR)/ui_base_unittests/ui_base_unittests-debug.apk to <(PRODUCT_DIR)/ui_unittests/ui_unittests-debug.apk You can probably put that in the ui_unittest_apk target (or even better rename that target to ui_base_unittests_apk and add a ui_unittests_apk for the copy just like you've done here for other platforms. https://codereview.chromium.org/588963002/diff/100001/ui/base/ui_base_tests.g... ui/base/ui_base_tests.gyp:311: 'ui_unittests', remove ui_unittests
Thanks Chris! I have built ui_base_unittests_apk and ui_unittests_apk targets locally. Haven't run, because my father traveled and took the Nexus 5 with him :( https://codereview.chromium.org/588963002/diff/80001/ui/base/ui_base_tests.gyp File ui/base/ui_base_tests.gyp (right): https://codereview.chromium.org/588963002/diff/80001/ui/base/ui_base_tests.gy... ui/base/ui_base_tests.gyp:247: 'source_file': '<(PRODUCT_DIR)/ui_base_unittests<(EXECUTABLE_SUFFIX)', On 2014/09/22 00:07:40, cjhopman wrote: > It the only difference is in the variables, why not move the conditions in > there? Done. https://codereview.chromium.org/588963002/diff/100001/ui/base/ui_base_tests.gyp File ui/base/ui_base_tests.gyp (right): https://codereview.chromium.org/588963002/diff/100001/ui/base/ui_base_tests.g... ui/base/ui_base_tests.gyp:286: }, On 2014/09/22 00:07:41, cjhopman wrote: > For android you probably have to copy > <(PRODUCT_DIR)/ui_base_unittests/ui_base_unittests-debug.apk > to > <(PRODUCT_DIR)/ui_unittests/ui_unittests-debug.apk > > You can probably put that in the ui_unittest_apk target (or even better rename > that target to ui_base_unittests_apk and add a ui_unittests_apk for the copy > just like you've done here for other platforms. Done. https://codereview.chromium.org/588963002/diff/100001/ui/base/ui_base_tests.g... ui/base/ui_base_tests.gyp:311: 'ui_unittests', On 2014/09/22 00:07:41, cjhopman wrote: > remove ui_unittests Done.
> Could you check locally for me? I'm not entirely sure what you're asking me to check. I can try to find some time tomorrow, but I'll be mostly OOO this week and next week, so you'll likely need to find another iOS reviewer to take over for me =) Clobber builds on the iOS trybots should also be useful (but make sure they're clobber builds, or else the old binary will still be around and mask failures). We'll need to copy the entire ui_unittests.app folder into ui_base_unittests.app. I'm pretty sure everything in that app bundle needs to be present in order for the app to run successfully. I'm not sure if it's enough to simply copy the .app folder, or if you also need to change Info.plist to refer to the new folder name. Have you considered making two identical targets with different names, rather than trying to copy the output binary? If you can extract the bulk of the current target into a common template, then there won't be much gyp duplication. We'll end up compiling these files twice, but that won't add too much time, it'll only be temporary, and it should sidestep all of these copying problems.
android lgtm https://codereview.chromium.org/588963002/diff/140001/ui/base/ui_base_tests.gyp File ui/base/ui_base_tests.gyp (right): https://codereview.chromium.org/588963002/diff/140001/ui/base/ui_base_tests.g... ui/base/ui_base_tests.gyp:244: ['OS != "ios" or OS != "android"', s/or/and also, we don't want this action at all on android (which works just fine with the conditions outside the action like you used to have-- feel free to switch back to that if you think it'll be clearest).
https://codereview.chromium.org/588963002/diff/140001/ui/base/ui_base_tests.gyp File ui/base/ui_base_tests.gyp (right): https://codereview.chromium.org/588963002/diff/140001/ui/base/ui_base_tests.g... ui/base/ui_base_tests.gyp:244: ['OS != "ios" or OS != "android"', On 2014/09/22 01:03:14, cjhopman wrote: > s/or/and > > also, we don't want this action at all on android (which works just fine with > the conditions outside the action like you used to have-- feel free to switch > back to that if you think it'll be clearest). Done.
Rohit, I will sleep with that idea of making identical targets with just different names. That might be easier. I'm doing tries with 'git cl try -c -r HEAD', I think -c clobbers the build.
tfarina@chromium.org changed reviewers: + tony@chromium.org
Tony could you review the changes in ui/resources/ui_resources.gyp for android? Chris, might want look at that too? This will hopefully address crbug.com/374490 Thanks, https://codereview.chromium.org/588963002/diff/220001/ui/resources/ui_resourc... File ui/resources/ui_resources.gyp (right): https://codereview.chromium.org/588963002/diff/220001/ui/resources/ui_resourc... ui/resources/ui_resources.gyp:127: '<(PRODUCT_DIR)/locales/en-US.pak', I'm hoping this will address -> crbug.com/374490. +Tony for this!
Rohit, I went with your suggestion of duplicating the target. That is going to avoid a whole set of problems. Specially when talking about iOS. Now copying (CC) lliabraa. I was mistakenly copying ibra :/ PTAL.
https://codereview.chromium.org/588963002/diff/220001/ui/resources/ui_resourc... File ui/resources/ui_resources.gyp (right): https://codereview.chromium.org/588963002/diff/220001/ui/resources/ui_resourc... ui/resources/ui_resources.gyp:128: ], It seems to have fixed it: http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
https://codereview.chromium.org/588963002/diff/220001/ui/resources/ui_resourc... File ui/resources/ui_resources.gyp (right): https://codereview.chromium.org/588963002/diff/220001/ui/resources/ui_resourc... ui/resources/ui_resources.gyp:128: ], It seems to have fixed it: http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...
lliabraa@chromium.org changed reviewers: + lliabraa@chromium.org
LGTM for iOS
On 2014/09/22 02:41:59, tfarina wrote: > Rohit, I went with your suggestion of duplicating the target. Rohit's suggestion was to extract the body of the target into a gypi and include it twice, not to copy and paste 200+ lines of code, which creates a near-certainty that the two won't stay in sync for however long it takes to unwind the hack.
On 2014/09/22 13:36:56, stuartmorgan wrote: > On 2014/09/22 02:41:59, tfarina wrote: > > Rohit, I went with your suggestion of duplicating the target. > > Rohit's suggestion was to extract the body of the target into a gypi and include > it twice, not to copy and paste 200+ lines of code, Right, I will try this later tonight. I think it will be a prettier solution and much more clearer. > which creates a near-certainty that the two won't stay in sync for however long it takes to > unwind the hack. I don't think these target are going to be updated very frequently in the time-frame I'm projecting. Though, I think a reviewer could easily enforce that they stay in sync, and do not let go unsync. Regards,
https://codereview.chromium.org/588963002/diff/220001/ui/resources/ui_resourc... File ui/resources/ui_resources.gyp (right): https://codereview.chromium.org/588963002/diff/220001/ui/resources/ui_resourc... ui/resources/ui_resources.gyp:127: '<(PRODUCT_DIR)/locales/en-US.pak', On 2014/09/22 02:34:55, tfarina wrote: > I'm hoping this will address -> crbug.com/374490. +Tony for this! Won't this get clobbered by the en-US.pak that is created by chrome_resources since they both point to the same output file? Why does the test apk try to read locales/en-US.pak instead of locales/ui/en-US.pak. We should probably split this into a separate change.
https://codereview.chromium.org/588963002/diff/220001/ui/resources/ui_resourc... File ui/resources/ui_resources.gyp (right): https://codereview.chromium.org/588963002/diff/220001/ui/resources/ui_resourc... ui/resources/ui_resources.gyp:127: '<(PRODUCT_DIR)/locales/en-US.pak', On 2014/09/22 16:20:50, tony wrote: > On 2014/09/22 02:34:55, tfarina wrote: > > I'm hoping this will address -> crbug.com/374490. +Tony for this! > > Won't this get clobbered by the en-US.pak that is created by chrome_resources > since they both point to the same output file? Why does the test apk try to > read locales/en-US.pak instead of locales/ui/en-US.pak. > Tony, Chris, could this being caused by https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/ui_base_te... From my understanding of https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/ui_base_pa..., DIR_LOCALES on Android won't be out/Debug/locales/en-US.pak. > We should probably split this into a separate change. Yeah, I was thinking on that. The fix to the clobber issue on Android deserves a separate CL.
On 2014/09/22 17:17:15, tfarina wrote: > https://codereview.chromium.org/588963002/diff/220001/ui/resources/ui_resourc... > File ui/resources/ui_resources.gyp (right): > > https://codereview.chromium.org/588963002/diff/220001/ui/resources/ui_resourc... > ui/resources/ui_resources.gyp:127: '<(PRODUCT_DIR)/locales/en-US.pak', > On 2014/09/22 16:20:50, tony wrote: > > On 2014/09/22 02:34:55, tfarina wrote: > > > I'm hoping this will address -> crbug.com/374490. +Tony for this! > > > > Won't this get clobbered by the en-US.pak that is created by chrome_resources > > since they both point to the same output file? Why does the test apk try to > > read locales/en-US.pak instead of locales/ui/en-US.pak. > > > Tony, Chris, could this being caused by > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/ui_base_te... > > From my understanding of > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/ui_base_pa..., > DIR_LOCALES on Android won't be out/Debug/locales/en-US.pak. Yeah, it's some local device path. The test running scripts should ensure that whatever is specified in the isolate stuff ends up in the right place on disk. Android should probably be using locales/ui/en-US.pak, though and overriding DIR_LOCALES for ui unittests like other non-ios platforms. > > > > We should probably split this into a separate change. > Yeah, I was thinking on that. The fix to the clobber issue on Android deserves a > separate CL. Agreed.
Scott, I addressed Stuart's comment and added the gypi to share the body between the two targets. I also removed the fix for the clobber issue on Android, will do that in a separate CL. PTAL. Thanks,
LGTM
The CQ bit was checked by tfarina@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/588963002/260001
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/588963002/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as 75aa9e7433ed539851f196c418fa5ae4413ed0cb
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/b5e7760a55f0450b2c9fb615b0aa0f030443a57e Cr-Commit-Position: refs/heads/master@{#296182} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
