Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10519005:
Fix libusb dependecy for Chromium with Android OS. (Closed)
|
Created:
8 years, 6 months ago by felipeg Modified:
8 years, 6 months ago CC:
chromium-reviews Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionFix libusb dependecy for Chromium with Android OS.
BUG=
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140759
Patch Set 1 #
Total comments: 6
Patch Set 2 : code review comments done. #
Total comments: 2
Patch Set 3 : #
Messages
Total messages: 16 (0 generated)
What problem exactly is this fixing? Are you building with system libusb on Android? Generally this change seems wrong/misguided to me, and I'd like to know more context of this CL.
https://chromiumcodereview.appspot.com/10519005/diff/1/third_party/libusb/lib... File third_party/libusb/libusb.gyp (right): https://chromiumcodereview.appspot.com/10519005/diff/1/third_party/libusb/lib... third_party/libusb/libusb.gyp:13: ['OS == "android"', { Why is this in a separate section by itself, and not in the condition block below at line 60?
On 2012/06/05 11:15:45, Paweł Hajdan Jr. wrote: > What problem exactly is this fixing? Are you building with system libusb on > Android? > > Generally this change seems wrong/misguided to me, and I'd like to know more > context of this CL. To give you a bit more context, most Android system libraries are not available to applications. Even if libusb is used by a given Android system release, there is no guarantee that this will hold true in the future (or that OEMs aren't going to heavily customize the system and replace it with something else). There are a few system libraries that *are* exposed by the NDK and are guaranteed to be available on all systems, with a stable ABI, but they're very strictly limited. And that's why Chrome needs to statically link libusb into its own shared library. It also has its own version of skia, ICU, stlport and plenty of other stuff. On the other hand, it doesn't have to wait for system updates to get bugfixes and performance improvements in said libraries. Hope this helps
On 2012/06/05 19:50:37, digit wrote: > On 2012/06/05 11:15:45, Paweł Hajdan Jr. wrote: > > What problem exactly is this fixing? Are you building with system libusb on > > Android? > > > > Generally this change seems wrong/misguided to me, and I'd like to know more > > context of this CL. > > To give you a bit more context, most Android system libraries are not available > to applications. True, even Google Chrome follows similar logic. I believe the default is use_system_libusb=0, so you should be using the bundled copy anyway. That's why I asked whether you're using use_system_libusb=1 (you shouldn't), and you haven't answered that question. > It also has its own version of skia, ICU, stlport and plenty of other > stuff. On the other hand, it doesn't have to wait for system updates to get > bugfixes and performance improvements in said libraries. Same is true for Google Chrome. With use_system_libusb=0 (again, the *default*), you should have exactly what you need. Are you hitting some other problems with this?
On 2012/06/06 06:39:01, Paweł Hajdan Jr. wrote: > > > What problem exactly is this fixing? Are you building with system libusb on > > > Android? > > > > > > Generally this change seems wrong/misguided to me, and I'd like to know more > > > context of this CL. > > > > To give you a bit more context, most Android system libraries are not > available > > to applications. > > True, even Google Chrome follows similar logic. I believe the default is > use_system_libusb=0, so you should be using the bundled copy anyway. That's why > I asked whether you're using use_system_libusb=1 (you shouldn't), and you > haven't answered that question. > > > It also has its own version of skia, ICU, stlport and plenty of other > > stuff. On the other hand, it doesn't have to wait for system updates to get > > bugfixes and performance improvements in said libraries. > > Same is true for Google Chrome. With use_system_libusb=0 (again, the *default*), > you should have exactly what you need. Are you hitting some other problems with > this? Ok, sorry, the more specific context is the following: - we use/require "use_system_libusb==0" for Android, this patch does not modify this in any way (it doesn't reference or modifies the variable anyway). - this patch allows third_party/libusb to be compiled when building Chromium for Android. This never happened before this patch due to a specific condition in chrome_browser.gypi that always excluded the library for these builds. - the patch also provides a few required 'defines' and 'sources' to make the Android build succeed. I agree with Lei Zhang that the 'OS=="android"' condition block should probably be moved to the 'else' clause of the 'OS=="linux" and use_system_libusb' condition. However, this is mostly stylistic (and to be clear, should be fixed). In other words, we don't believe this patch affects any other Chromium build. If this is not the case, please point us to the problematic bits and we will fix them. Apart from that, this is part of our general upstreaming effort. Since the project was initally confidential, it was developed as a private internal fork of the Chromium sources. Now that we're public, we're (slowly) sending our changes to the respective upstream projects to have as many things open-sourced as possible.
http://chromiumcodereview.appspot.com/10519005/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (left): http://chromiumcodereview.appspot.com/10519005/diff/1/chrome/chrome_browser.g... chrome/chrome_browser.gypi:4689: ['OS!="android"', { I guess you want to make it _always_ depend on libusb, rather than *never* . Before, on POSIX this would depend on libusb. After your change, it wouldn't. http://chromiumcodereview.appspot.com/10519005/diff/1/third_party/libusb/libu... File third_party/libusb/libusb.gyp (right): http://chromiumcodereview.appspot.com/10519005/diff/1/third_party/libusb/libu... third_party/libusb/libusb.gyp:13: ['OS == "android"', { On 2012/06/05 18:22:53, Lei Zhang wrote: > Why is this in a separate section by itself, and not in the condition block > below at line 60? Please do what Lei said. It makes no sense to duplicate this code. This is _not_ stylistic.
http://chromiumcodereview.appspot.com/10519005/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (left): http://chromiumcodereview.appspot.com/10519005/diff/1/chrome/chrome_browser.g... chrome/chrome_browser.gypi:4689: ['OS!="android"', { OOops, that's right. that's totally not what we want to do. thanks for spotting this. Damn.
Hey guys, Thanks for all the comments. Now I understand it better. Sorry for the confusion, my initial change was just a dumbly applying the diff from downstream/clank to upstream/chromium. After this code review I will also fix the downstream code as requested here. http://chromiumcodereview.appspot.com/10519005/diff/1/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (left): http://chromiumcodereview.appspot.com/10519005/diff/1/chrome/chrome_browser.g... chrome/chrome_browser.gypi:4689: ['OS!="android"', { On 2012/06/06 08:20:00, Paweł Hajdan Jr. wrote: > I guess you want to make it _always_ depend on libusb, rather than *never* . > > Before, on POSIX this would depend on libusb. After your change, it wouldn't. Good catch. Done. http://chromiumcodereview.appspot.com/10519005/diff/1/third_party/libusb/libu... File third_party/libusb/libusb.gyp (right): http://chromiumcodereview.appspot.com/10519005/diff/1/third_party/libusb/libu... third_party/libusb/libusb.gyp:13: ['OS == "android"', { On 2012/06/06 08:20:00, Paweł Hajdan Jr. wrote: > On 2012/06/05 18:22:53, Lei Zhang wrote: > > Why is this in a separate section by itself, and not in the condition block > > below at line 60? > > Please do what Lei said. It makes no sense to duplicate this code. > > This is _not_ stylistic. Done.
http://chromiumcodereview.appspot.com/10519005/diff/14001/third_party/libusb/... File third_party/libusb/libusb.gyp (left): http://chromiumcodereview.appspot.com/10519005/diff/14001/third_party/libusb/... third_party/libusb/libusb.gyp:12: 'conditions': [ Does this pass gyp syntax?
Compiling locally and running the bots now. http://chromiumcodereview.appspot.com/10519005/diff/14001/third_party/libusb/... File third_party/libusb/libusb.gyp (left): http://chromiumcodereview.appspot.com/10519005/diff/14001/third_party/libusb/... third_party/libusb/libusb.gyp:12: 'conditions': [ On 2012/06/06 10:02:32, Paweł Hajdan Jr. wrote: > Does this pass gyp syntax? Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felipeg@chromium.org/10519005/8003
Change committed as 140759
lgtm++ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
