Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
258043003:
Eliminate components_unittests' dependence on chrome resources. (Closed)
|
Created:
6 years, 7 months ago by blundell Modified:
5 years, 10 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionEliminate components_unittests' dependence on chrome resources.
This CL changes components_unittests to create its own pakfile rather than
relying on the chrome pakfile. To do this it adds a repack step that repacks
the pakfiles that components_unittests needs into a
components_unittests_resources.pak file, and then loads that pakfile
explicitly.
This change means that components_unittests now passes after a clean build,
whereas before it would fail due to missing resources.
BUG=364622, 348563
Patch Set 1 #
Total comments: 5
Patch Set 2 : Fix for Android #Patch Set 3 : Address review comments #
Total comments: 2
Patch Set 4 : Restore ui path provider #
Total comments: 1
Messages
Total messages: 31 (0 generated)
Jochen: For review. Thanks for the help with this one! Thiago: FYI.
Colin, this is awesome :) Thanks for fixing this. https://codereview.chromium.org/258043003/diff/1/components/components_tests.gyp File components/components_tests.gyp (right): https://codereview.chromium.org/258043003/diff/1/components/components_tests.... components/components_tests.gyp:251: 'action_name': 'repack_components_pack', I like this. s/repack_components_pack/repack_components_pak ? In ui we have this as a separate target though, ui_test_pak. If you like, you may consider also having this as a separate target, components_test_pak
https://codereview.chromium.org/258043003/diff/1/components/test/run_all_unit... File components/test/run_all_unittests.cc (left): https://codereview.chromium.org/258043003/diff/1/components/test/run_all_unit... components/test/run_all_unittests.cc:68: ui::RegisterPathProvider(); are you sure this can go away? Last time I looked this, we were using some enum constants from ui_base_paths.h. If it can go away, can we remove the ui_base_paths.h include?
Bots are green, so LGTM. https://codereview.chromium.org/258043003/diff/1/components/components_tests.gyp File components/components_tests.gyp (right): https://codereview.chromium.org/258043003/diff/1/components/components_tests.... components/components_tests.gyp:251: 'action_name': 'repack_components_pack', On 2014/04/29 15:58:25, tfarina wrote: > I like this. s/repack_components_pack/repack_components_pak ? > > In ui we have this as a separate target though, ui_test_pak. > > If you like, you may consider also having this as a separate target, > components_test_pak I don't think it matters much either way. If you're generating MSVS files, then it's a bit more efficient to have it this way. If you make a new target, gyp generates a separate .proj file. https://codereview.chromium.org/258043003/diff/1/components/components_tests.... components/components_tests.gyp:255: '<(SHARED_INTERMEDIATE_DIR)/components/strings/component_strings_en-US.pak', Do you need to include ui_strings* or ui_resources.pak?
lgtm, thanks!
I'm checking the CQ box. It seems fine to land this with Tony's approval.
The CQ bit was checked by tfarina@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/258043003/1
The CQ bit was unchecked by commit-bot@chromium.org
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
The CQ bit was checked by tfarina@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/258043003/1
The CQ bit was unchecked by commit-bot@chromium.org
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
This fails on Android: For example: [ RUN ] WalletClientTest.CancelRequest [FATAL:scoped_ptr.h(376)] Assert failed: impl_.get() != __null. http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...
https://codereview.chromium.org/258043003/diff/1/components/components_tests.gyp File components/components_tests.gyp (right): https://codereview.chromium.org/258043003/diff/1/components/components_tests.... components/components_tests.gyp:315: 'includes': ['../chrome/chrome_ios_bundle_resources.gypi'], Can this also be removed?
+bulach, torne, yfriedman Hi Android folks, The Android port is not happy with my CL eliminating components_unittests' dependency on chrome resources (see android_dbg_triggered_tests:components_unittests). I can't see what I'm missing, but clearly I *am* missing something (and it's probably fairly obvious :P). Anyone able to shine a light? Thanks, Colin
On 2014/05/05 14:29:28, blundell wrote: > +bulach, torne, yfriedman > > Hi Android folks, > > The Android port is not happy with my CL eliminating components_unittests' > dependency on chrome resources (see > android_dbg_triggered_tests:components_unittests). I can't see what I'm missing, > but clearly I *am* missing something (and it's probably fairly obvious :P). > Anyone able to shine a light? > > Thanks, > > Colin hard to say, but there's a bunch of: [ERROR:memory_mapped_file.cc(23)] Couldn't open /storage/emulated/0/components_unittests_resources.pak [ERROR:data_pack.cc(78)] Failed to mmap datapack and also a few others: ... [FATAL:scoped_ptr.h(376)] Assert failed: impl_.get() != __null. [ RUN ] WalletClientTest.SaveAddressFailedMalformedResponse [FATAL:scoped_ptr.h(376)] Assert failed: impl_.get() != __null. ... the .pak seem to have been built and downloaded: http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered... inflating: full-build-linux/components_unittests_resources.pak the push to the device didn't seem to fail though... :(
Thanks for taking a look! Sorry, I should have been more explicit. It's clear that what's going wrong is that the pakfile isn't being loaded correctly: there's some step in the setup of the //chrome pakfile that I'm missing now that I'm using my own components_unittests_resources pakfile. What I can't see is what that step is :(.
On 2014/05/06 10:51:02, blundell wrote: > Thanks for taking a look! Sorry, I should have been more explicit. It's clear > that what's going wrong is that the pakfile isn't being loaded correctly: > there's some step in the setup of the //chrome pakfile that I'm missing now that > I'm using my own components_unittests_resources pakfile. What I can't see is > what that step is :(. sorry, no idea, but wild guesses :) does it need to be in a .grd file? https://code.google.com/p/chromium/codesearch#chromium/src/content/content_re... https://code.google.com/p/chromium/codesearch#chromium/src/content/content_re... how about "resource extractor", is it part of the component tests? https://code.google.com/p/chromium/codesearch#chromium/src/content/public/and...
On 2014/05/06 14:17:02, bulach wrote: > On 2014/05/06 10:51:02, blundell wrote: > > Thanks for taking a look! Sorry, I should have been more explicit. It's clear > > that what's going wrong is that the pakfile isn't being loaded correctly: > > there's some step in the setup of the //chrome pakfile that I'm missing now > that > > I'm using my own components_unittests_resources pakfile. What I can't see is > > what that step is :(. > > sorry, no idea, but wild guesses :) > does it need to be in a .grd file? > https://code.google.com/p/chromium/codesearch#chromium/src/content/content_re... > https://code.google.com/p/chromium/codesearch#chromium/src/content/content_re... > > how about "resource extractor", is it part of the component tests? > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/and...
On 2014/05/06 14:17:02, bulach wrote: > On 2014/05/06 10:51:02, blundell wrote: > > Thanks for taking a look! Sorry, I should have been more explicit. It's clear > > that what's going wrong is that the pakfile isn't being loaded correctly: > > there's some step in the setup of the //chrome pakfile that I'm missing now > that > > I'm using my own components_unittests_resources pakfile. What I can't see is > > what that step is :(. > > sorry, no idea, but wild guesses :) > does it need to be in a .grd file? > https://code.google.com/p/chromium/codesearch#chromium/src/content/content_re... > https://code.google.com/p/chromium/codesearch#chromium/src/content/content_re... > Sorry the spam in the last message, my mouse is broken :/ To answer that, no. The pak file is being generated in by the components_unittests target: https://codereview.chromium.org/258043003/diff/40001/components/components_te... - line 257 > how about "resource extractor", is it part of the component tests? > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/and...
Maybe you can change MemoryMappedFile::Initialize() to write out file_.error_details() and run it through the try bots to get an error code?
https://codereview.chromium.org/258043003/diff/40001/components/test/run_all_... File components/test/run_all_unittests.cc (right): https://codereview.chromium.org/258043003/diff/40001/components/test/run_all_... components/test/run_all_unittests.cc:47: resources_pack_path.AppendASCII("components_unittests_resources.pak")); There's an example of doing a similar pattern in components/dom_distiller/content/distiller_page_web_contents_browsertest.cc Have you given that a look?
The comment in https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/test/run_a..., seems to indicate that components_unittests_apk provides the framework necessary, but still the call to IniSharedInstanceWithLocale is still necessary. https://codereview.chromium.org/258043003/diff/40001/components/test/run_all_... File components/test/run_all_unittests.cc (right): https://codereview.chromium.org/258043003/diff/40001/components/test/run_all_... components/test/run_all_unittests.cc:47: resources_pack_path.AppendASCII("components_unittests_resources.pak")); On 2014/05/06 17:50:58, Yaron wrote: > There's an example of doing a similar pattern in > components/dom_distiller/content/distiller_page_web_contents_browsertest.cc > Are you sure that above example works without the call to InitSharedInstanceWithLocale? I doubt. > Have you given that a look?
On 2014/05/06 18:03:08, tfarina wrote: > The comment in > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/test/run_a..., > seems to indicate that components_unittests_apk provides the framework > necessary, but still the call to IniSharedInstanceWithLocale is still necessary. > > https://codereview.chromium.org/258043003/diff/40001/components/test/run_all_... > File components/test/run_all_unittests.cc (right): > > https://codereview.chromium.org/258043003/diff/40001/components/test/run_all_... > components/test/run_all_unittests.cc:47: > resources_pack_path.AppendASCII("components_unittests_resources.pak")); > On 2014/05/06 17:50:58, Yaron wrote: > > There's an example of doing a similar pattern in > > components/dom_distiller/content/distiller_page_web_contents_browsertest.cc > > > Are you sure that above example works without the call to > InitSharedInstanceWithLocale? I doubt. > > > Have you given that a look? components_browsertests doesn't run run_all_unittests.cc, so the difference isn't that. My suspicion is that the difference is that components_unittests is built as an APK whereas components_browsertests doesn't seem to be. From looking at ui_unittests (thanks for the pointer Thiago!), which is also built as an APK, it looks like on Android the //chrome resources are assumed to be present and are loaded. That is to say, I would guess that after a clean build of only ui_unittests on Android, the resources wouldn't be found (similar to the bug I'm trying to fix here).
That sounds plausible. And actually, it's a good point; component_browsertests doesn't yet run on android. I need to fix that but it's a separate issue. On Tue, May 6, 2014 at 12:08 PM, <blundell@chromium.org> wrote: > On 2014/05/06 18:03:08, tfarina wrote: > >> The comment in >> > > https://code.google.com/p/chromium/codesearch#chromium/ > src/ui/base/test/run_all_unittests.cc&l=67, > >> seems to indicate that components_unittests_apk provides the framework >> necessary, but still the call to IniSharedInstanceWithLocale is still >> > necessary. > > > https://codereview.chromium.org/258043003/diff/40001/ > components/test/run_all_unittests.cc > >> File components/test/run_all_unittests.cc (right): >> > > > https://codereview.chromium.org/258043003/diff/40001/ > components/test/run_all_unittests.cc#newcode47 > >> components/test/run_all_unittests.cc:47: >> resources_pack_path.AppendASCII("components_unittests_resources.pak")); >> On 2014/05/06 17:50:58, Yaron wrote: >> > There's an example of doing a similar pattern in >> > components/dom_distiller/content/distiller_page_web_ >> contents_browsertest.cc >> > >> Are you sure that above example works without the call to >> InitSharedInstanceWithLocale? I doubt. >> > > > Have you given that a look? >> > > components_browsertests doesn't run run_all_unittests.cc, so the difference > isn't that. My suspicion is that the difference is that > components_unittests is > built as an APK whereas components_browsertests doesn't seem to be. From > looking > at ui_unittests (thanks for the pointer Thiago!), which is also built as > an APK, > it looks like on Android the //chrome resources are assumed to be present > and > are loaded. That is to say, I would guess that after a clean build of only > ui_unittests on Android, the resources wouldn't be found (similar to the > bug I'm > trying to fix here). > > https://codereview.chromium.org/258043003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/05/06 20:30:04, Yaron wrote: > That sounds plausible. And actually, it's a good point; > component_browsertests doesn't yet run on android. I need to fix that but > it's a separate issue. > > > On Tue, May 6, 2014 at 12:08 PM, <mailto:blundell@chromium.org> wrote: > > > On 2014/05/06 18:03:08, tfarina wrote: > > > >> The comment in > >> > > > > https://code.google.com/p/chromium/codesearch#chromium/ > > src/ui/base/test/run_all_unittests.cc&l=67, > > > >> seems to indicate that components_unittests_apk provides the framework > >> necessary, but still the call to IniSharedInstanceWithLocale is still > >> > > necessary. > > > > > > https://codereview.chromium.org/258043003/diff/40001/ > > components/test/run_all_unittests.cc > > > >> File components/test/run_all_unittests.cc (right): > >> > > > > > > https://codereview.chromium.org/258043003/diff/40001/ > > components/test/run_all_unittests.cc#newcode47 > > > >> components/test/run_all_unittests.cc:47: > >> resources_pack_path.AppendASCII("components_unittests_resources.pak")); > >> On 2014/05/06 17:50:58, Yaron wrote: > >> > There's an example of doing a similar pattern in > >> > components/dom_distiller/content/distiller_page_web_ > >> contents_browsertest.cc > >> > > >> Are you sure that above example works without the call to > >> InitSharedInstanceWithLocale? I doubt. > >> > > > > > Have you given that a look? > >> > > > > components_browsertests doesn't run run_all_unittests.cc, so the difference > > isn't that. My suspicion is that the difference is that > > components_unittests is > > built as an APK whereas components_browsertests doesn't seem to be. From > > looking > > at ui_unittests (thanks for the pointer Thiago!), which is also built as > > an APK, > > it looks like on Android the //chrome resources are assumed to be present > > and > > are loaded. That is to say, I would guess that after a clean build of only > > ui_unittests on Android, the resources wouldn't be found (similar to the > > bug I'm > > trying to fix here). > > > > https://codereview.chromium.org/258043003/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Ok, I think we do something screwy with paths. If you look at the failure log: http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered... you can see that we push to /storage/emulated/legacy/paks However the error messages says we're trying to read /storage/emulated/0/paks If you grep for DIR_RESOURCE_PAKS_ANDROID, in some path_providers, we're doing custom logic for pak files but in others we aren't. I suspect that components_unittests' PathProvider isn't setting up the right path. Somethjing like that.
https://codereview.chromium.org/258043003/diff/60001/components/test/run_all_... File components/test/run_all_unittests.cc (right): https://codereview.chromium.org/258043003/diff/60001/components/test/run_all_... components/test/run_all_unittests.cc:45: ui::RegisterPathProvider(); Yaron, there is no components_paths.cc:RegisterPathProvider. This is one of the reasons we call this here (I think). ui_base_paths.cc:RegisterPathProvider has the logic for DIR_RESOURCE_PAKS_ANDROID, which is used in https://chromium.googlesource.com/chromium/chromium/+/trunk/ui/base/resource/....
[ERROR:memory_mapped_file.cc(23)] Couldn't open /storage/emulated/0/components_unittests_resources.pak So is that the place (/storage/emulated/0) android looks for pak files? I think the question is, where should we copy the pak so that android can find it? Then we could add a copy rule to components_unittests target for the repack_components_pak action. Does this make any sense? (I hope I'm not suggesting anything non plausible).
On 2014/05/07 21:09:25, tfarina wrote: > [ERROR:memory_mapped_file.cc(23)] Couldn't open > /storage/emulated/0/components_unittests_resources.pak > > So is that the place (/storage/emulated/0) android looks for pak files? > > I think the question is, where should we copy the pak so that android can find > it? > > Then we could add a copy rule to components_unittests target for the > repack_components_pak action. > > Does this make any sense? > > (I hope I'm not suggesting anything non plausible). I don't think that's right. I still think it's an issue with the test binary. paks are copied to /storage/emulated/legacy/paks as part of the test. You don't need a copy rule as other targets dont' either, and this is about pushing the pak files to the device and not out dir. Also, the test runner already pushes these pak files to the device as it does with all other unit test binaries for android |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
