|
|
Created:
4 years, 2 months ago by michaelbai Modified:
3 years, 2 months ago CC:
chromium-reviews, mikecase+watch_chromium.org, jbudorick+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOnly list direct deps as java libraries
So, only direct deps are listed in NaitveLibraries.LIBRARIES because it is
not necessary to list all .so files in NativeLibrariesLIBRARIES, only direct dep shared libraries need to be loaded explicitly.
BUG=649445
Patch Set 1 #Patch Set 2 : add c++_shared #
Total comments: 2
Messages
Total messages: 39 (7 generated)
The CQ bit was checked by michaelbai@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Only list direct deps as java libraries So, only direct deps is listed in NaitveLibraries.LIBRARIES BUG= ========== to ========== Only list direct deps as java libraries So, only direct deps are listed in NaitveLibraries.LIBRARIES because it is not necessary to list all .so files in NativeLibrariesLIBRARIES, only direct dep shared libraries need to be loaded explicitly. BUG=649445 ==========
michaelbai@chromium.org changed reviewers: + agrieve@chromium.org
michaelbai@chromium.org changed reviewers: + torne@chromium.org
Not really familiar enough with how the android gn rules work to review this code, but what's the actual result here? Do we end up with just one library listed in each APK because it's extracted from deps, or are we still doing the topological sort and taking all the roots (i.e. including the unused components)? Also, does this cause any problems for the incremental install workflow? I'm not sure I follow how that works from agrieve's comments on the bug; did you test it?
On 2016/10/24 12:10:30, Torne wrote: > Not really familiar enough with how the android gn rules work to review this > code, but what's the actual result here? Do we end up with just one library > listed in each APK because it's extracted from deps, or are we still doing the > topological sort and taking all the roots (i.e. including the unused > components)? > Only libraries in android_apk's dep are listed, currently WebView, Chrome and Monochrome has only one lib. > Also, does this cause any problems for the incremental install workflow? I'm not > sure I follow how that works from agrieve's comments on the bug; did you test > it? It shouldn't break incremental install, because only libraries list used by NativeLibraries.java is changed, the gn's build config file still has full list of libraries but not sorted, i.e the unused libraries are still in the APK, we just didn't load them.
On 2016/10/24 18:04:54, michaelbai wrote: > On 2016/10/24 12:10:30, Torne wrote: > > Not really familiar enough with how the android gn rules work to review this > > code, but what's the actual result here? Do we end up with just one library > > listed in each APK because it's extracted from deps, or are we still doing the > > topological sort and taking all the roots (i.e. including the unused > > components)? > > > > Only libraries in android_apk's dep are listed, currently WebView, Chrome and > Monochrome has only one lib. > > > Also, does this cause any problems for the incremental install workflow? I'm > not > > sure I follow how that works from agrieve's comments on the bug; did you test > > it? > > It shouldn't break incremental install, because only libraries list used by > NativeLibraries.java is changed, the gn's build config file still has full list > of libraries but not sorted, i.e the unused libraries are still in the APK, we > just didn't load them. I'll test this on my kitkat device tomorrow just to be sure.
On 2016/10/24 18:07:26, agrieve (OOO oct 17-21) wrote: > On 2016/10/24 18:04:54, michaelbai wrote: > > On 2016/10/24 12:10:30, Torne wrote: > > > Not really familiar enough with how the android gn rules work to review this > > > code, but what's the actual result here? Do we end up with just one library > > > listed in each APK because it's extracted from deps, or are we still doing > the > > > topological sort and taking all the roots (i.e. including the unused > > > components)? > > > > > > > Only libraries in android_apk's dep are listed, currently WebView, Chrome and > > Monochrome has only one lib. > > > > > Also, does this cause any problems for the incremental install workflow? I'm > > not > > > sure I follow how that works from agrieve's comments on the bug; did you > test > > > it? > > > > It shouldn't break incremental install, because only libraries list used by > > NativeLibraries.java is changed, the gn's build config file still has full > list > > of libraries but not sorted, i.e the unused libraries are still in the APK, we > > just didn't load them. > > I'll test this on my kitkat device tomorrow just to be sure. Alright, applying this patching and installing chrome_public_apk_incremental on my nexus 4 running Android 4.3 does cause it to break. The logcat shows: D/dalvikvm(22579): Trying to load lib /data/data/org.chromium.chrome/incremental-install-files/lib/libchrome.cr.so 0x448c6730 E/dalvikvm(22579): dlopen("/data/data/org.chromium.chrome/incremental-install-files/lib/libchrome.cr.so") failed: dlopen failed: could not load library "libbase.cr.so" needed by "libchrome.cr.so"; caused by could not load library "libc++_shared.so" needed by "libbase.cr.so"; caused by library "libc++_shared.so" not found E/NativeInitializationController(22579): Unable to load native library. E/NativeInitializationController(22579): org.chromium.base.library_loader.ProcessInitException E/NativeInitializationController(22579): at org.chromium.base.library_loader.LibraryLoader.loadAlreadyLocked(LibraryLoader.java:323) E/NativeInitializationController(22579): at org.chromium.base.library_loader.LibraryLoader.ensureInitialized(LibraryLoader.java:140) E/NativeInitializationController(22579): at org.chromium.chrome.browser.init.NativeInitializationController$1.run(NativeInitializationController.java:83) E/NativeInitializationController(22579): Caused by: java.lang.UnsatisfiedLinkError: dlopen failed: could not load library "libbase.cr.so" needed by "libchrome.cr.so"; caused by could not load library "libc++_shared.so" needed by "libbase.cr.so"; caused by library "libc++_shared.so" not found E/NativeInitializationController(22579): at java.lang.Runtime.loadLibrary(Runtime.java:361) E/NativeInitializationController(22579): at java.lang.System.loadLibrary(System.java:525) E/NativeInitializationController(22579): at org.chromium.base.library_loader.LibraryLoader.loadAlreadyLocked(LibraryLoader.java:309) E/NativeInitializationController(22579): ... 2 more
On 2016/10/25 20:26:33, agrieve (OOO oct 17-21) wrote: > On 2016/10/24 18:07:26, agrieve (OOO oct 17-21) wrote: > > On 2016/10/24 18:04:54, michaelbai wrote: > > > On 2016/10/24 12:10:30, Torne wrote: > > > > Not really familiar enough with how the android gn rules work to review > this > > > > code, but what's the actual result here? Do we end up with just one > library > > > > listed in each APK because it's extracted from deps, or are we still doing > > the > > > > topological sort and taking all the roots (i.e. including the unused > > > > components)? > > > > > > > > > > Only libraries in android_apk's dep are listed, currently WebView, Chrome > and > > > Monochrome has only one lib. > > > > > > > Also, does this cause any problems for the incremental install workflow? > I'm > > > not > > > > sure I follow how that works from agrieve's comments on the bug; did you > > test > > > > it? > > > > > > It shouldn't break incremental install, because only libraries list used by > > > NativeLibraries.java is changed, the gn's build config file still has full > > list > > > of libraries but not sorted, i.e the unused libraries are still in the APK, > we > > > just didn't load them. > > > > I'll test this on my kitkat device tomorrow just to be sure. > > Alright, applying this patching and installing chrome_public_apk_incremental on > my nexus 4 running Android 4.3 does cause it to break. The logcat shows: > > D/dalvikvm(22579): Trying to load lib > /data/data/org.chromium.chrome/incremental-install-files/lib/libchrome.cr.so > 0x448c6730 > E/dalvikvm(22579): > dlopen("/data/data/org.chromium.chrome/incremental-install-files/lib/libchrome.cr.so") > failed: dlopen failed: could not load library "libbase.cr.so" needed by > "libchrome.cr.so"; caused by could not load library "libc++_shared.so" needed by > "libbase.cr.so"; caused by library "libc++_shared.so" not found > E/NativeInitializationController(22579): Unable to load native library. > E/NativeInitializationController(22579): > org.chromium.base.library_loader.ProcessInitException > E/NativeInitializationController(22579): at > org.chromium.base.library_loader.LibraryLoader.loadAlreadyLocked(LibraryLoader.java:323) > E/NativeInitializationController(22579): at > org.chromium.base.library_loader.LibraryLoader.ensureInitialized(LibraryLoader.java:140) > E/NativeInitializationController(22579): at > org.chromium.chrome.browser.init.NativeInitializationController$1.run(NativeInitializationController.java:83) > E/NativeInitializationController(22579): Caused by: > java.lang.UnsatisfiedLinkError: dlopen failed: could not load library > "libbase.cr.so" needed by "libchrome.cr.so"; caused by could not load library > "libc++_shared.so" needed by "libbase.cr.so"; caused by library > "libc++_shared.so" not found > E/NativeInitializationController(22579): at > java.lang.Runtime.loadLibrary(Runtime.java:361) > E/NativeInitializationController(22579): at > java.lang.System.loadLibrary(System.java:525) > E/NativeInitializationController(22579): at > org.chromium.base.library_loader.LibraryLoader.loadAlreadyLocked(LibraryLoader.java:309) > E/NativeInitializationController(22579): ... 2 more Here's a pastbin since the wrapping here is pretty bad: http://pastebin.com/HEb2jGrh Interesting to note that error messages makes it seems as that libchrome.cr.so loaded libbase.cr.so just fine, but then libbase.cr.so failed to find libc++.cr.so
On 2016/10/25 20:28:52, agrieve (OOO oct 17-21) wrote: > On 2016/10/25 20:26:33, agrieve (OOO oct 17-21) wrote: > > On 2016/10/24 18:07:26, agrieve (OOO oct 17-21) wrote: > > > On 2016/10/24 18:04:54, michaelbai wrote: > > > > On 2016/10/24 12:10:30, Torne wrote: > > > > > Not really familiar enough with how the android gn rules work to review > > this > > > > > code, but what's the actual result here? Do we end up with just one > > library > > > > > listed in each APK because it's extracted from deps, or are we still > doing > > > the > > > > > topological sort and taking all the roots (i.e. including the unused > > > > > components)? > > > > > > > > > > > > > Only libraries in android_apk's dep are listed, currently WebView, Chrome > > and > > > > Monochrome has only one lib. > > > > > > > > > Also, does this cause any problems for the incremental install workflow? > > I'm > > > > not > > > > > sure I follow how that works from agrieve's comments on the bug; did you > > > test > > > > > it? > > > > > > > > It shouldn't break incremental install, because only libraries list used > by > > > > NativeLibraries.java is changed, the gn's build config file still has full > > > list > > > > of libraries but not sorted, i.e the unused libraries are still in the > APK, > > we > > > > just didn't load them. > > > > > > I'll test this on my kitkat device tomorrow just to be sure. > > > > Alright, applying this patching and installing chrome_public_apk_incremental > on > > my nexus 4 running Android 4.3 does cause it to break. The logcat shows: > > > > D/dalvikvm(22579): Trying to load lib > > /data/data/org.chromium.chrome/incremental-install-files/lib/libchrome.cr.so > > 0x448c6730 > > E/dalvikvm(22579): > > > dlopen("/data/data/org.chromium.chrome/incremental-install-files/lib/libchrome.cr.so") > > failed: dlopen failed: could not load library "libbase.cr.so" needed by > > "libchrome.cr.so"; caused by could not load library "libc++_shared.so" needed > by > > "libbase.cr.so"; caused by library "libc++_shared.so" not found > > E/NativeInitializationController(22579): Unable to load native library. > > E/NativeInitializationController(22579): > > org.chromium.base.library_loader.ProcessInitException > > E/NativeInitializationController(22579): at > > > org.chromium.base.library_loader.LibraryLoader.loadAlreadyLocked(LibraryLoader.java:323) > > E/NativeInitializationController(22579): at > > > org.chromium.base.library_loader.LibraryLoader.ensureInitialized(LibraryLoader.java:140) > > E/NativeInitializationController(22579): at > > > org.chromium.chrome.browser.init.NativeInitializationController$1.run(NativeInitializationController.java:83) > > E/NativeInitializationController(22579): Caused by: > > java.lang.UnsatisfiedLinkError: dlopen failed: could not load library > > "libbase.cr.so" needed by "libchrome.cr.so"; caused by could not load library > > "libc++_shared.so" needed by "libbase.cr.so"; caused by library > > "libc++_shared.so" not found > > E/NativeInitializationController(22579): at > > java.lang.Runtime.loadLibrary(Runtime.java:361) > > E/NativeInitializationController(22579): at > > java.lang.System.loadLibrary(System.java:525) > > E/NativeInitializationController(22579): at > > > org.chromium.base.library_loader.LibraryLoader.loadAlreadyLocked(LibraryLoader.java:309) > > E/NativeInitializationController(22579): ... 2 more > > Here's a pastbin since the wrapping here is pretty bad: > http://pastebin.com/HEb2jGrh > > Interesting to note that error messages makes it seems as that libchrome.cr.so > loaded libbase.cr.so just fine, but then libbase.cr.so failed to find > libc++.cr.so Even more info: - The bug where this was broken in the past is: https://bugs.chromium.org/p/chromium/issues/detail?id=624791 It looks like the issue with this patch as-is is that libc++.so is no longer listed in the build_config under native.libraries. I tried out reverting this patch and hardcoding write_build_config.py to return "chrome.cr" in _CreateJavaLibrariesList, and the app loaded fine on JB and M, and N MR1. So... It think this should be fixable, but am actually still not sure what caused the linked bug to fail...
On 2016/10/25 20:58:47, agrieve (OOO oct 17-21) wrote: > On 2016/10/25 20:28:52, agrieve (OOO oct 17-21) wrote: > > On 2016/10/25 20:26:33, agrieve (OOO oct 17-21) wrote: > > > On 2016/10/24 18:07:26, agrieve (OOO oct 17-21) wrote: > > > > On 2016/10/24 18:04:54, michaelbai wrote: > > > > > On 2016/10/24 12:10:30, Torne wrote: > > > > > > Not really familiar enough with how the android gn rules work to > review > > > this > > > > > > code, but what's the actual result here? Do we end up with just one > > > library > > > > > > listed in each APK because it's extracted from deps, or are we still > > doing > > > > the > > > > > > topological sort and taking all the roots (i.e. including the unused > > > > > > components)? > > > > > > > > > > > > > > > > Only libraries in android_apk's dep are listed, currently WebView, > Chrome > > > and > > > > > Monochrome has only one lib. > > > > > > > > > > > Also, does this cause any problems for the incremental install > workflow? > > > I'm > > > > > not > > > > > > sure I follow how that works from agrieve's comments on the bug; did > you > > > > test > > > > > > it? > > > > > > > > > > It shouldn't break incremental install, because only libraries list used > > by > > > > > NativeLibraries.java is changed, the gn's build config file still has > full > > > > list > > > > > of libraries but not sorted, i.e the unused libraries are still in the > > APK, > > > we > > > > > just didn't load them. > > > > > > > > I'll test this on my kitkat device tomorrow just to be sure. > > > > > > Alright, applying this patching and installing chrome_public_apk_incremental > > on > > > my nexus 4 running Android 4.3 does cause it to break. The logcat shows: > > > > > > D/dalvikvm(22579): Trying to load lib > > > /data/data/org.chromium.chrome/incremental-install-files/lib/libchrome.cr.so > > > 0x448c6730 > > > E/dalvikvm(22579): > > > > > > dlopen("/data/data/org.chromium.chrome/incremental-install-files/lib/libchrome.cr.so") > > > failed: dlopen failed: could not load library "libbase.cr.so" needed by > > > "libchrome.cr.so"; caused by could not load library "libc++_shared.so" > needed > > by > > > "libbase.cr.so"; caused by library "libc++_shared.so" not found > > > E/NativeInitializationController(22579): Unable to load native library. > > > E/NativeInitializationController(22579): > > > org.chromium.base.library_loader.ProcessInitException > > > E/NativeInitializationController(22579): at > > > > > > org.chromium.base.library_loader.LibraryLoader.loadAlreadyLocked(LibraryLoader.java:323) > > > E/NativeInitializationController(22579): at > > > > > > org.chromium.base.library_loader.LibraryLoader.ensureInitialized(LibraryLoader.java:140) > > > E/NativeInitializationController(22579): at > > > > > > org.chromium.chrome.browser.init.NativeInitializationController$1.run(NativeInitializationController.java:83) > > > E/NativeInitializationController(22579): Caused by: > > > java.lang.UnsatisfiedLinkError: dlopen failed: could not load library > > > "libbase.cr.so" needed by "libchrome.cr.so"; caused by could not load > library > > > "libc++_shared.so" needed by "libbase.cr.so"; caused by library > > > "libc++_shared.so" not found > > > E/NativeInitializationController(22579): at > > > java.lang.Runtime.loadLibrary(Runtime.java:361) > > > E/NativeInitializationController(22579): at > > > java.lang.System.loadLibrary(System.java:525) > > > E/NativeInitializationController(22579): at > > > > > > org.chromium.base.library_loader.LibraryLoader.loadAlreadyLocked(LibraryLoader.java:309) > > > E/NativeInitializationController(22579): ... 2 more > > > > Here's a pastbin since the wrapping here is pretty bad: > > http://pastebin.com/HEb2jGrh > > > > Interesting to note that error messages makes it seems as that libchrome.cr.so > > loaded libbase.cr.so just fine, but then libbase.cr.so failed to find > > libc++.cr.so > > Even more info: > - The bug where this was broken in the past is: > https://bugs.chromium.org/p/chromium/issues/detail?id=624791 > > It looks like the issue with this patch as-is is that libc++.so is no longer > listed in the build_config under native.libraries. > > I tried out reverting this patch and hardcoding write_build_config.py to return > "chrome.cr" in _CreateJavaLibrariesList, and the app loaded fine on JB and M, > and N MR1. > > So... It think this should be fixable, but am actually still not sure what > caused the linked bug to fail... One thing I don't understand, why did you test this patch on 'nexus 4 running Android 4.3', doesn't this issue only happen in this device?
On 2016/10/25 21:23:55, michaelbai wrote: > On 2016/10/25 20:58:47, agrieve (OOO oct 17-21) wrote: > > On 2016/10/25 20:28:52, agrieve (OOO oct 17-21) wrote: > > > On 2016/10/25 20:26:33, agrieve (OOO oct 17-21) wrote: > > > > On 2016/10/24 18:07:26, agrieve (OOO oct 17-21) wrote: > > > > > On 2016/10/24 18:04:54, michaelbai wrote: > > > > > > On 2016/10/24 12:10:30, Torne wrote: > > > > > > > Not really familiar enough with how the android gn rules work to > > review > > > > this > > > > > > > code, but what's the actual result here? Do we end up with just one > > > > library > > > > > > > listed in each APK because it's extracted from deps, or are we still > > > doing > > > > > the > > > > > > > topological sort and taking all the roots (i.e. including the unused > > > > > > > components)? > > > > > > > > > > > > > > > > > > > Only libraries in android_apk's dep are listed, currently WebView, > > Chrome > > > > and > > > > > > Monochrome has only one lib. > > > > > > > > > > > > > Also, does this cause any problems for the incremental install > > workflow? > > > > I'm > > > > > > not > > > > > > > sure I follow how that works from agrieve's comments on the bug; did > > you > > > > > test > > > > > > > it? > > > > > > > > > > > > It shouldn't break incremental install, because only libraries list > used > > > by > > > > > > NativeLibraries.java is changed, the gn's build config file still has > > full > > > > > list > > > > > > of libraries but not sorted, i.e the unused libraries are still in the > > > APK, > > > > we > > > > > > just didn't load them. > > > > > > > > > > I'll test this on my kitkat device tomorrow just to be sure. > > > > > > > > Alright, applying this patching and installing > chrome_public_apk_incremental > > > on > > > > my nexus 4 running Android 4.3 does cause it to break. The logcat shows: > > > > > > > > D/dalvikvm(22579): Trying to load lib > > > > > /data/data/org.chromium.chrome/incremental-install-files/lib/libchrome.cr.so > > > > 0x448c6730 > > > > E/dalvikvm(22579): > > > > > > > > > > dlopen("/data/data/org.chromium.chrome/incremental-install-files/lib/libchrome.cr.so") > > > > failed: dlopen failed: could not load library "libbase.cr.so" needed by > > > > "libchrome.cr.so"; caused by could not load library "libc++_shared.so" > > needed > > > by > > > > "libbase.cr.so"; caused by library "libc++_shared.so" not found > > > > E/NativeInitializationController(22579): Unable to load native library. > > > > E/NativeInitializationController(22579): > > > > org.chromium.base.library_loader.ProcessInitException > > > > E/NativeInitializationController(22579): at > > > > > > > > > > org.chromium.base.library_loader.LibraryLoader.loadAlreadyLocked(LibraryLoader.java:323) > > > > E/NativeInitializationController(22579): at > > > > > > > > > > org.chromium.base.library_loader.LibraryLoader.ensureInitialized(LibraryLoader.java:140) > > > > E/NativeInitializationController(22579): at > > > > > > > > > > org.chromium.chrome.browser.init.NativeInitializationController$1.run(NativeInitializationController.java:83) > > > > E/NativeInitializationController(22579): Caused by: > > > > java.lang.UnsatisfiedLinkError: dlopen failed: could not load library > > > > "libbase.cr.so" needed by "libchrome.cr.so"; caused by could not load > > library > > > > "libc++_shared.so" needed by "libbase.cr.so"; caused by library > > > > "libc++_shared.so" not found > > > > E/NativeInitializationController(22579): at > > > > java.lang.Runtime.loadLibrary(Runtime.java:361) > > > > E/NativeInitializationController(22579): at > > > > java.lang.System.loadLibrary(System.java:525) > > > > E/NativeInitializationController(22579): at > > > > > > > > > > org.chromium.base.library_loader.LibraryLoader.loadAlreadyLocked(LibraryLoader.java:309) > > > > E/NativeInitializationController(22579): ... 2 more > > > > > > Here's a pastbin since the wrapping here is pretty bad: > > > http://pastebin.com/HEb2jGrh > > > > > > Interesting to note that error messages makes it seems as that > libchrome.cr.so > > > loaded libbase.cr.so just fine, but then libbase.cr.so failed to find > > > libc++.cr.so > > > > Even more info: > > - The bug where this was broken in the past is: > > https://bugs.chromium.org/p/chromium/issues/detail?id=624791 > > > > It looks like the issue with this patch as-is is that libc++.so is no longer > > listed in the build_config under native.libraries. > > > > I tried out reverting this patch and hardcoding write_build_config.py to > return > > "chrome.cr" in _CreateJavaLibrariesList, and the app loaded fine on JB and M, > > and N MR1. > > > > So... It think this should be fixable, but am actually still not sure what > > caused the linked bug to fail... > > One thing I don't understand, why did you test this patch on 'nexus 4 running > Android 4.3', doesn't this issue only happen in this device? I think I found reason the libc++_shared.so wasn't packed into APK, don't know why it worked for me, and no buildbot found this issue.
Please review patchset #2, Note, there are 2 libraries listed in NativeLibraries.java, the additional one is "c++_shared", it is added in android_apk template for component build, and is direct dep of APK, unless we add a special case for c++_shared, there is no simple way to remove it from list in NativeLibraries.java. I think it is fine to have c++_shared out there.
I'm not sure I understand what the issue is here, and I don't see anything to do with c++_shared in the latest patch. Shouldn't it be the case that all shared libraries depend on c++_shared in the component build? They all use the C++ runtime, no?
On 2016/10/26 10:33:26, Torne wrote: > I'm not sure I understand what the issue is here, and I don't see anything to do > with c++_shared in the latest patch. > > Shouldn't it be the case that all shared libraries depend on c++_shared in the > component build? They all use the C++ runtime, no? The related code is in line 1499-1506 of new patch, the shared libraries in component build do use C++ runtime lib, this is about how we pack the c++_shared into APK, the c++_shared can't be find in GN's dep graph and need to be added it explicitly.
lgtm (assuming you've tested that chrome_public_apk_incremental works). https://codereview.chromium.org/2416073003/diff/20001/build/android/gyp/write... File build/android/gyp/write_build_config.py (right): https://codereview.chromium.org/2416073003/diff/20001/build/android/gyp/write... build/android/gyp/write_build_config.py:201: """Returns sorted direct and all deps. Not sure this makes sense. How about: Returns a tuple of (root_libs, all_libs). https://codereview.chromium.org/2416073003/diff/20001/build/config/android/in... File build/config/android/internal_rules.gni (right): https://codereview.chromium.org/2416073003/diff/20001/build/config/android/in... build/config/android/internal_rules.gni:290: _rebased_files = rebase_path(invoker.shared_libraries_runtime_deps_file, nit: can you pluralize this: shared_libraries_runtime_deps_files
On 2016/10/26 16:19:14, michaelbai wrote: > On 2016/10/26 10:33:26, Torne wrote: > > I'm not sure I understand what the issue is here, and I don't see anything to > do > > with c++_shared in the latest patch. > > > > Shouldn't it be the case that all shared libraries depend on c++_shared in the > > component build? They all use the C++ runtime, no? > > The related code is in line 1499-1506 of new patch, the shared libraries in > component build do use C++ runtime lib, this is about how we pack the c++_shared > into APK, the c++_shared can't be find in GN's dep graph and need to be added > it explicitly. Oh, is the problem that c++_shared isn't actually a target and we're just pulling it in by having the NDK in the linker search path? Or am I misunderstanding it? If this works with the incremental build then LGTM :)
On 2016/10/27 10:11:37, Torne wrote: > On 2016/10/26 16:19:14, michaelbai wrote: > > On 2016/10/26 10:33:26, Torne wrote: > > > I'm not sure I understand what the issue is here, and I don't see anything > to > > do > > > with c++_shared in the latest patch. > > > > > > Shouldn't it be the case that all shared libraries depend on c++_shared in > the > > > component build? They all use the C++ runtime, no? > > > > The related code is in line 1499-1506 of new patch, the shared libraries in > > component build do use C++ runtime lib, this is about how we pack the > c++_shared > > into APK, the c++_shared can't be find in GN's dep graph and need to be added > > it explicitly. > > Oh, is the problem that c++_shared isn't actually a target and we're just > pulling it in by having the NDK in the linker search path? Or am I > misunderstanding it? > > If this works with the incremental build then LGTM :) That's exactly right. It's linked in via ldflags, so GN doesn't know that it's also a data_dep.
Just tried the incremental build, unfortunately, the patch set #2 didn't work with it. I also tried to only load chrome.cr without my patch (by hardcode lib in NativeLibraries.template), got same error. Andrew, don't know how it worked for you, I think your change might not take effect. The error is NativeInitializationController: Caused by: java.lang.UnsatisfiedLinkError: dlopen failed: library "libbase.cr.so" not found The system linker had issue to find the libs.
Pathset #2 worked in M, it seem N prevent library being loaded from other place.
The failure on N probably relates to system linker's namespace
On 2016/10/28 00:42:41, michaelbai wrote: > The failure on N probably relates to system linker's namespace The linker namespace code should permit libraries to be loaded from a minimum of three places: 1) The app's APK 2) The directory that contains the libraries that the package installer automatically extracted at install time 3) The app's data directory If the behaviour is different on N then it is probably related to the namespacing changes, but it's not supposed to be preventing apps from loading native libraries from their own data directories, and I suspect we are either triggering a bug or just not setting up the search path correctly (it's possible that the exact details of search path handling has changed).
So am I right that the current status here is: 1) with PS#2 everything (including incremental install) works on pre-N 2) with PS#2 normal installs work on N, but incremental install is broken 3) with no changes at all, incremental installs do work on N Or is that not correct?
On 2016/10/28 16:37:43, Torne wrote: > So am I right that the current status here is: > > 1) with PS#2 everything (including incremental install) works on pre-N > 2) with PS#2 normal installs work on N, but incremental install is broken > 3) with no changes at all, incremental installs do work on N > > Or is that not correct? I believe that's all correct.
On 2016/10/28 16:53:40, agrieve wrote: > On 2016/10/28 16:37:43, Torne wrote: > > So am I right that the current status here is: > > > > 1) with PS#2 everything (including incremental install) works on pre-N > > 2) with PS#2 normal installs work on N, but incremental install is broken > > 3) with no changes at all, incremental installs do work on N > > > > Or is that not correct? > > I believe that's all correct. So, yeah, I would guess that the way we're tweaking the linker search path just doesn't quite work right on N, due to changes in the linker (which were probably done to support namespaces, but I don't think this is directly *caused* by namespaces). Loading all the individual libraries explicitly works because as I said in the bug, the algorithm for System.loadLibrary to find the actual library requested is separate from teh algorithm for the linker to find its dependencies.
Michael, are you still working on this? It seems like we should try to find a way for this to work.
On 2017/09/25 18:38:35, Torne wrote: > Michael, are you still working on this? It seems like we should try to find a > way for this to work. No, I am not, any ideal about how to make this work?
On 2017/09/25 18:54:50, michaelbai wrote: > On 2017/09/25 18:38:35, Torne wrote: > > Michael, are you still working on this? It seems like we should try to find a > > way for this to work. > > No, I am not, any ideal about how to make this work? Re-reading the conversation here it sounds like this change as-is is correct, and the only reason we haven't landed it is because it breaks incremental installs due to the incremental install code not doing the right thing on N (and only happening to work because of this behaviour of explicitly loading all the libraries one at a time). Fixing the incremental install stuff on N to set the paths properly seems like the right thing to do. Is there a bug tracking this work specifically? The referenced bug is just about monochrome apk differences, which isn't a great summary of this. Rietveld is going read-only on Friday so any important information should be captured in a bug so we can follow up easily.
On 2017/09/25 19:15:17, Torne wrote: > On 2017/09/25 18:54:50, michaelbai wrote: > > On 2017/09/25 18:38:35, Torne wrote: > > > Michael, are you still working on this? It seems like we should try to find > a > > > way for this to work. > > > > No, I am not, any ideal about how to make this work? > > Re-reading the conversation here it sounds like this change as-is is correct, > and the only reason we haven't landed it is because it breaks incremental > installs due to the incremental install code not doing the right thing on N (and > only happening to work because of this behaviour of explicitly loading all the > libraries one at a time). Fixing the incremental install stuff on N to set the > paths properly seems like the right thing to do. > > Is there a bug tracking this work specifically? The referenced bug is just about > monochrome apk differences, which isn't a great summary of this. Rietveld is > going read-only on Friday so any important information should be captured in a > bug so we can follow up easily. I have impression that the failure related to N platform, maybe I am wrong, it would be great if it can be fixed in incremental install.
On 2017/09/25 19:29:50, michaelbai wrote: > On 2017/09/25 19:15:17, Torne wrote: > > On 2017/09/25 18:54:50, michaelbai wrote: > > > On 2017/09/25 18:38:35, Torne wrote: > > > > Michael, are you still working on this? It seems like we should try to > find > > a > > > > way for this to work. > > > > > > No, I am not, any ideal about how to make this work? > > > > Re-reading the conversation here it sounds like this change as-is is correct, > > and the only reason we haven't landed it is because it breaks incremental > > installs due to the incremental install code not doing the right thing on N > (and > > only happening to work because of this behaviour of explicitly loading all the > > libraries one at a time). Fixing the incremental install stuff on N to set the > > paths properly seems like the right thing to do. > > > > Is there a bug tracking this work specifically? The referenced bug is just > about > > monochrome apk differences, which isn't a great summary of this. Rietveld is > > going read-only on Friday so any important information should be captured in a > > bug so we can follow up easily. > > I have impression that the failure related to N platform, maybe I am wrong, it > would be great if it can be fixed in incremental install. The incremental install code is messing around with internals of the platform via reflection to do its trickery. That trick isn't working any more on N - that's not the platform's fault that we're messing with non-public APIs to do unsupported things :)
On 2017/09/25 19:31:05, Torne wrote: > On 2017/09/25 19:29:50, michaelbai wrote: > > On 2017/09/25 19:15:17, Torne wrote: > > > On 2017/09/25 18:54:50, michaelbai wrote: > > > > On 2017/09/25 18:38:35, Torne wrote: > > > > > Michael, are you still working on this? It seems like we should try to > > find > > > a > > > > > way for this to work. > > > > > > > > No, I am not, any ideal about how to make this work? > > > > > > Re-reading the conversation here it sounds like this change as-is is > correct, > > > and the only reason we haven't landed it is because it breaks incremental > > > installs due to the incremental install code not doing the right thing on N > > (and > > > only happening to work because of this behaviour of explicitly loading all > the > > > libraries one at a time). Fixing the incremental install stuff on N to set > the > > > paths properly seems like the right thing to do. > > > > > > Is there a bug tracking this work specifically? The referenced bug is just > > about > > > monochrome apk differences, which isn't a great summary of this. Rietveld is > > > going read-only on Friday so any important information should be captured in > a > > > bug so we can follow up easily. > > > > I have impression that the failure related to N platform, maybe I am wrong, it > > would be great if it can be fixed in incremental install. > > The incremental install code is messing around with internals of the platform > via reflection to do its trickery. That trick isn't working any more on N - > that's not the platform's fault that we're messing with non-public APIs to do > unsupported things :) Sounds good, you can take this over.
On 2017/09/25 20:36:23, michaelbai wrote: > On 2017/09/25 19:31:05, Torne wrote: > > On 2017/09/25 19:29:50, michaelbai wrote: > > > On 2017/09/25 19:15:17, Torne wrote: > > > > On 2017/09/25 18:54:50, michaelbai wrote: > > > > > On 2017/09/25 18:38:35, Torne wrote: > > > > > > Michael, are you still working on this? It seems like we should try to > > > find > > > > a > > > > > > way for this to work. > > > > > > > > > > No, I am not, any ideal about how to make this work? > > > > > > > > Re-reading the conversation here it sounds like this change as-is is > > correct, > > > > and the only reason we haven't landed it is because it breaks incremental > > > > installs due to the incremental install code not doing the right thing on > N > > > (and > > > > only happening to work because of this behaviour of explicitly loading all > > the > > > > libraries one at a time). Fixing the incremental install stuff on N to set > > the > > > > paths properly seems like the right thing to do. > > > > > > > > Is there a bug tracking this work specifically? The referenced bug is just > > > about > > > > monochrome apk differences, which isn't a great summary of this. Rietveld > is > > > > going read-only on Friday so any important information should be captured > in > > a > > > > bug so we can follow up easily. > > > > > > I have impression that the failure related to N platform, maybe I am wrong, > it > > > would be great if it can be fixed in incremental install. > > > > The incremental install code is messing around with internals of the platform > > via reflection to do its trickery. That trick isn't working any more on N - > > that's not the platform's fault that we're messing with non-public APIs to do > > unsupported things :) > > Sounds good, you can take this over. I don't want to take this over particularly. Do we have a bug tracking this particular work?
On 2017/09/25 20:37:27, Torne wrote: > On 2017/09/25 20:36:23, michaelbai wrote: > > On 2017/09/25 19:31:05, Torne wrote: > > > On 2017/09/25 19:29:50, michaelbai wrote: > > > > On 2017/09/25 19:15:17, Torne wrote: > > > > > On 2017/09/25 18:54:50, michaelbai wrote: > > > > > > On 2017/09/25 18:38:35, Torne wrote: > > > > > > > Michael, are you still working on this? It seems like we should try > to > > > > find > > > > > a > > > > > > > way for this to work. > > > > > > > > > > > > No, I am not, any ideal about how to make this work? > > > > > > > > > > Re-reading the conversation here it sounds like this change as-is is > > > correct, > > > > > and the only reason we haven't landed it is because it breaks > incremental > > > > > installs due to the incremental install code not doing the right thing > on > > N > > > > (and > > > > > only happening to work because of this behaviour of explicitly loading > all > > > the > > > > > libraries one at a time). Fixing the incremental install stuff on N to > set > > > the > > > > > paths properly seems like the right thing to do. > > > > > > > > > > Is there a bug tracking this work specifically? The referenced bug is > just > > > > about > > > > > monochrome apk differences, which isn't a great summary of this. > Rietveld > > is > > > > > going read-only on Friday so any important information should be > captured > > in > > > a > > > > > bug so we can follow up easily. > > > > > > > > I have impression that the failure related to N platform, maybe I am > wrong, > > it > > > > would be great if it can be fixed in incremental install. > > > > > > The incremental install code is messing around with internals of the > platform > > > via reflection to do its trickery. That trick isn't working any more on N - > > > that's not the platform's fault that we're messing with non-public APIs to > do > > > unsupported things :) > > > > Sounds good, you can take this over. > > I don't want to take this over particularly. Do we have a bug tracking this > particular work? We don't have bug for this specifically, I found this issue when I investigated the merge failures :(
On 2017/09/25 20:41:17, michaelbai wrote: > On 2017/09/25 20:37:27, Torne wrote: > > On 2017/09/25 20:36:23, michaelbai wrote: > > > On 2017/09/25 19:31:05, Torne wrote: > > > > On 2017/09/25 19:29:50, michaelbai wrote: > > > > > On 2017/09/25 19:15:17, Torne wrote: > > > > > > On 2017/09/25 18:54:50, michaelbai wrote: > > > > > > > On 2017/09/25 18:38:35, Torne wrote: > > > > > > > > Michael, are you still working on this? It seems like we should > try > > to > > > > > find > > > > > > a > > > > > > > > way for this to work. > > > > > > > > > > > > > > No, I am not, any ideal about how to make this work? > > > > > > > > > > > > Re-reading the conversation here it sounds like this change as-is is > > > > correct, > > > > > > and the only reason we haven't landed it is because it breaks > > incremental > > > > > > installs due to the incremental install code not doing the right thing > > on > > > N > > > > > (and > > > > > > only happening to work because of this behaviour of explicitly loading > > all > > > > the > > > > > > libraries one at a time). Fixing the incremental install stuff on N to > > set > > > > the > > > > > > paths properly seems like the right thing to do. > > > > > > > > > > > > Is there a bug tracking this work specifically? The referenced bug is > > just > > > > > about > > > > > > monochrome apk differences, which isn't a great summary of this. > > Rietveld > > > is > > > > > > going read-only on Friday so any important information should be > > captured > > > in > > > > a > > > > > > bug so we can follow up easily. > > > > > > > > > > I have impression that the failure related to N platform, maybe I am > > wrong, > > > it > > > > > would be great if it can be fixed in incremental install. > > > > > > > > The incremental install code is messing around with internals of the > > platform > > > > via reflection to do its trickery. That trick isn't working any more on N > - > > > > that's not the platform's fault that we're messing with non-public APIs to > > do > > > > unsupported things :) > > > > > > Sounds good, you can take this over. > > > > I don't want to take this over particularly. Do we have a bug tracking this > > particular work? > > We don't have bug for this specifically, I found this issue when I investigated > the merge failures :( Can you file a tracking bug for this and summarise the state from this CL (and link to this so we can find it again later)? |