Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12374068:
Make it possible to disable udev in linux (Closed)
|
Created:
7 years, 9 months ago by Mostyn Bramley-Moore Modified:
7 years, 1 month ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, rogerj Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionMake it possible to disable udev in the content API on linux
This is useful for embedded linux setups, which often don't include udev support.
TEST=Build content_shell on linux with use_udev=0 then run ldd on output binaries to verify libudev is not listed (and is listed if built with use_udev=1 or unspecified)
BUG=318315, 318413
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236204
Patch Set 1 #Patch Set 2 : move use_udev setting to build/common.gypi #
Total comments: 2
Patch Set 3 : move USE_UDEV define to a linux-specific spot #
Total comments: 2
Patch Set 4 : dust off and rebase on master #
Total comments: 4
Patch Set 5 : review fixups #
Total comments: 6
Patch Set 6 : fix jam's nits #
Total comments: 4
Patch Set 7 : rename device_monitor_linux.* to device_monitor_udev.* #
Total comments: 4
Patch Set 8 : update guard #Patch Set 9 : clarify use_udev scope #
Total comments: 3
Patch Set 10 : unbreak mac #
Messages
Total messages: 41 (0 generated)
This patch makes it possible to build for linux systems without udev (eg embedded systems).
Can you put the use_udev gyp variable in build/common.gypi instead?
If you prefer- done in the second patch set.
lgtm but wait for Lei.
There's more udev usage in chrome. If we are going to disable udev, we probably should do it everywhere. https://codereview.chromium.org/12374068/diff/7003/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/12374068/diff/7003/content/content_browser.gy... content/content_browser.gypi:1157: 'defines': [ This would define USE_UDEV on Win/Mac. How about we move this up to ~line 1075, inside a OS==linux block?
https://codereview.chromium.org/12374068/diff/7003/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/12374068/diff/7003/content/content_browser.gy... content/content_browser.gypi:1157: 'defines': [ On 2013/03/04 21:10:25, Lei Zhang wrote: > This would define USE_UDEV on Win/Mac. How about we move this up to ~line 1075, > inside a OS==linux block? Done.
On 2013/03/04 21:10:25, Lei Zhang wrote: > There's more udev usage in chrome. If we are going to disable udev, we probably > should do it everywhere. I am most interested in the content api- chrome builds are more likely run on desktop-like systems and need not be so careful with extra libraries like udev. Would it make sense to give this variable a better name to make it clear that it only affects the "core"/content api, or would you prefer a general "disable udev" patch?
+jam since we need a content/ OWNER anyway. Do you have an opinion on content-only build flags? https://codereview.chromium.org/12374068/diff/3004/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/12374068/diff/3004/content/content_browser.gy... content/content_browser.gypi:1147: ['OS=="linux" and use_udev==0', { You can keep this together with the use_udev block above: ['use_udev == 1', { # defines, etc }, { # sources! etc }],
On 2013/03/05 00:27:24, Lei Zhang wrote: > +jam since we need a content/ OWNER anyway. > Do you have an opinion on content-only build flags? let's see what the conclusion of the discussion in https://codereview.chromium.org/12385071/ is, and apply it here
@rjkroege: is this relevant to ozone embedded content_shell / crbug.com/318315 ? If so, I will rebase and update this CL...
On 2013/11/13 23:03:22, Mostyn Bramley-Moore wrote: > @rjkroege: is this relevant to ozone embedded content_shell / crbug.com/318315 ? > > If so, I will rebase and update this CL... Yes libudev should be removable. Overlooked this in the bug. Please reference bugs 318315, 318413.
Rebased and retested, this still seems to work. https://codereview.chromium.org/12374068/diff/3004/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/12374068/diff/3004/content/content_browser.gy... content/content_browser.gypi:1147: ['OS=="linux" and use_udev==0', { On 2013/03/05 00:27:24, Lei Zhang wrote: > You can keep this together with the use_udev block above: > > ['use_udev == 1', { > # defines, etc > }, { > # sources! etc > }], Done.
content/browser/device_monitor_linux.h can use a defensive check: #if !defined(USE_UDEV) #error USE_UDEV must be defined. #endif https://codereview.chromium.org/12374068/diff/34001/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/12374068/diff/34001/content/content_browser.g... content/content_browser.gypi:1413: 'browser/device_monitor_linux.cc', nit: alphabetical order https://codereview.chromium.org/12374068/diff/34001/content/content_browser.g... content/content_browser.gypi:1420: '../build/linux/system.gyp:udev', Can you remove this, and instead move line 1400 into the use_udev block?
Done. https://codereview.chromium.org/12374068/diff/34001/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/12374068/diff/34001/content/content_browser.g... content/content_browser.gypi:1413: 'browser/device_monitor_linux.cc', On 2013/11/14 00:35:00, Lei Zhang wrote: > nit: alphabetical order Done. https://codereview.chromium.org/12374068/diff/34001/content/content_browser.g... content/content_browser.gypi:1420: '../build/linux/system.gyp:udev', On 2013/11/14 00:35:00, Lei Zhang wrote: > Can you remove this, and instead move line 1400 into the use_udev block? Done.
lgtm, but you probably still need an OWNER
On 2013/11/14 01:28:37, Lei Zhang wrote: > lgtm, but you probably still need an OWNER @jam: PTAL.
On 2013/11/14 07:50:37, Mostyn Bramley-Moore wrote: > On 2013/11/14 01:28:37, Lei Zhang wrote: > > lgtm, but you probably still need an OWNER > > @jam: PTAL. Oh, and to recap... this patched stalled ~8 months ago because I didn't get around to discussing the number of these new gyp settings that I wanted to add (since they weren't directly useful to Google ~8 months ago). But now this is needed for products like chromecast, and similar patches are being landed by Google in bugs 318315 and 318413.
https://codereview.chromium.org/12374068/diff/94001/content/browser/device_mo... File content/browser/device_monitor_linux.h (right): https://codereview.chromium.org/12374068/diff/94001/content/browser/device_mo... content/browser/device_monitor_linux.h:13: #endif this doesn't seem very pretty to me Given that this file is not just for linux, maybe rename it to device_monitor_udev.h? Then a naming rule can exclude it. I think this would end simplify the CL.
https://codereview.chromium.org/12374068/diff/94001/content/browser/device_mo... File content/browser/device_monitor_linux.h (right): https://codereview.chromium.org/12374068/diff/94001/content/browser/device_mo... content/browser/device_monitor_linux.h:13: #endif On 2013/11/14 15:44:23, rjkroege wrote: > this doesn't seem very pretty to me > > Given that this file is not just for linux, maybe rename it to > device_monitor_udev.h? Then a naming rule can exclude it. I think this would end > simplify the CL. What other operating systems use this file? All I can see is content/browser/browser_main_loop.cc, where the inclusion is wrapped in #if defined(OS_LINUX) && defined(USE_UDEV). I believe udev is linux-specific (and I'm pretty sure that doesn't include android).
https://codereview.chromium.org/12374068/diff/94001/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/12374068/diff/94001/content/content_browser.g... content/content_browser.gypi:1408: 'USE_UDEV', nit: the convention is that this is done in build\common.gypi, see the big section there starting at line 2000 https://codereview.chromium.org/12374068/diff/94001/content/content_browser.g... content/content_browser.gypi:1419: 'browser/gamepad/gamepad_platform_data_fetcher.cc', can you just update the condition on line 1245 to be (OS!="linux" || use_udev==0)
https://codereview.chromium.org/12374068/diff/94001/content/content_browser.gypi File content/content_browser.gypi (right): https://codereview.chromium.org/12374068/diff/94001/content/content_browser.g... content/content_browser.gypi:1408: 'USE_UDEV', On 2013/11/14 17:37:29, jam wrote: > nit: the convention is that this is done in build\common.gypi, see the big > section there starting at line 2000 Done. (Though this has the small downside of invalidating lots of ccache on the next build.) https://codereview.chromium.org/12374068/diff/94001/content/content_browser.g... content/content_browser.gypi:1419: 'browser/gamepad/gamepad_platform_data_fetcher.cc', On 2013/11/14 17:37:29, jam wrote: > can you just update the condition on line 1245 to be > (OS!="linux" || use_udev==0) Done.
lgtm
On 2013/11/14 16:42:01, Mostyn Bramley-Moore wrote: > https://codereview.chromium.org/12374068/diff/94001/content/browser/device_mo... > File content/browser/device_monitor_linux.h (right): > > https://codereview.chromium.org/12374068/diff/94001/content/browser/device_mo... > content/browser/device_monitor_linux.h:13: #endif > On 2013/11/14 15:44:23, rjkroege wrote: > > this doesn't seem very pretty to me > > > > Given that this file is not just for linux, maybe rename it to > > device_monitor_udev.h? Then a naming rule can exclude it. I think this would > end > > simplify the CL. > > What other operating systems use this file? All I can see is > content/browser/browser_main_loop.cc, where the inclusion is wrapped in #if > defined(OS_LINUX) && defined(USE_UDEV). > > I believe udev is linux-specific (and I'm pretty sure that doesn't include > android). *ping* ?
lgtm
https://codereview.chromium.org/12374068/diff/214001/content/browser/browser_... File content/browser/browser_main_loop.h (right): https://codereview.chromium.org/12374068/diff/214001/content/browser/browser_... content/browser/browser_main_loop.h:144: #elif defined(OS_LINUX) && defined(USE_UDEV) isn't the defined(OS_LINUX) superfluous here an elsewhere as LINUX implies USE_UDEV? https://codereview.chromium.org/12374068/diff/214001/content/browser/device_m... File content/browser/device_monitor_linux.h (right): https://codereview.chromium.org/12374068/diff/214001/content/browser/device_m... content/browser/device_monitor_linux.h:11: #if !defined(USE_UDEV) my very badly worded comment from before remains. if you renamed this file device_monitor_udev.h and added a name pattern, then this test here would be unnecessary: the build system would not compile the file unless USE_UDEV was defined?
https://codereview.chromium.org/12374068/diff/214001/content/browser/browser_... File content/browser/browser_main_loop.h (right): https://codereview.chromium.org/12374068/diff/214001/content/browser/browser_... content/browser/browser_main_loop.h:144: #elif defined(OS_LINUX) && defined(USE_UDEV) On 2013/11/15 23:16:31, rjkroege wrote: > isn't the defined(OS_LINUX) superfluous here an elsewhere as LINUX implies > USE_UDEV? I assume you mean USE_UDEV implies OS_LINUX, done. https://codereview.chromium.org/12374068/diff/214001/content/browser/device_m... File content/browser/device_monitor_linux.h (right): https://codereview.chromium.org/12374068/diff/214001/content/browser/device_m... content/browser/device_monitor_linux.h:11: #if !defined(USE_UDEV) On 2013/11/15 23:16:31, rjkroege wrote: > my very badly worded comment from before remains. > > if you renamed this file device_monitor_udev.h and added a name pattern, then > this test here would be unnecessary: the build system would not compile the file > unless USE_UDEV was defined? > Done.
https://codereview.chromium.org/12374068/diff/324001/content/browser/device_m... File content/browser/device_monitor_udev.h (right): https://codereview.chromium.org/12374068/diff/324001/content/browser/device_m... content/browser/device_monitor_udev.h:8: #ifndef CONTENT_BROWSER_DEVICE_MONITOR_LINUX_H_ you need to change this and the name of the class. Sorry for the inconvenience. https://codereview.chromium.org/12374068/diff/324001/content/content_browser.... File content/content_browser.gypi (right): https://codereview.chromium.org/12374068/diff/324001/content/content_browser.... content/content_browser.gypi:1408: 'sources!': [ I didn't provide adequate information. I meant you to use an entry in build/filename_rules.gypi to make this unnecessary. See the use_pango section for example. The use of a naming rule can keep the gyp* file simpler. All of these files if named with _udev.* would be excluded by the naming rule. What do you think?
https://codereview.chromium.org/12374068/diff/324001/content/browser/device_m... File content/browser/device_monitor_udev.h (right): https://codereview.chromium.org/12374068/diff/324001/content/browser/device_m... content/browser/device_monitor_udev.h:8: #ifndef CONTENT_BROWSER_DEVICE_MONITOR_LINUX_H_ On 2013/11/18 22:32:19, rjkroege wrote: > you need to change this and the name of the class. Sorry for the inconvenience. Done. https://codereview.chromium.org/12374068/diff/324001/content/content_browser.... File content/content_browser.gypi (right): https://codereview.chromium.org/12374068/diff/324001/content/content_browser.... content/content_browser.gypi:1408: 'sources!': [ On 2013/11/18 22:32:19, rjkroege wrote: > I didn't provide adequate information. I meant you to use an entry in > build/filename_rules.gypi to make this unnecessary. See the use_pango section > for example. The use of a naming rule can keep the gyp* file simpler. > > All of these files if named with _udev.* would be excluded by the naming rule. > What do you think? It might be a little confusing since there are other uses of udev in the chrome layer, and these files don't end with _udev.*. Which reminds me of something from my old CL- this setting only affects the content layer, should we rename it to something like content_use_udev? I think this is a reasonable distinction to make (ie disabling udev in content but not in chrome).
On 2013/11/18 23:32:25, Mostyn Bramley-Moore wrote: > https://codereview.chromium.org/12374068/diff/324001/content/browser/device_m... > File content/browser/device_monitor_udev.h (right): > > https://codereview.chromium.org/12374068/diff/324001/content/browser/device_m... > content/browser/device_monitor_udev.h:8: #ifndef > CONTENT_BROWSER_DEVICE_MONITOR_LINUX_H_ > On 2013/11/18 22:32:19, rjkroege wrote: > > you need to change this and the name of the class. Sorry for the > inconvenience. > > Done. > > https://codereview.chromium.org/12374068/diff/324001/content/content_browser.... > File content/content_browser.gypi (right): > > https://codereview.chromium.org/12374068/diff/324001/content/content_browser.... > content/content_browser.gypi:1408: 'sources!': [ > On 2013/11/18 22:32:19, rjkroege wrote: > > I didn't provide adequate information. I meant you to use an entry in > > build/filename_rules.gypi to make this unnecessary. See the use_pango section > > for example. The use of a naming rule can keep the gyp* file simpler. > > > > All of these files if named with _udev.* would be excluded by the naming rule. > > What do you think? > > It might be a little confusing since there are other uses of udev in the chrome > layer, and these files don't end with _udev.*. > > Which reminds me of something from my old CL- this setting only affects the > content layer, should we rename it to something like content_use_udev? I think > this is a reasonable distinction to make (ie disabling udev in content but not > in chrome). Please use the same style as the existing flags. It is not useful to have different flags for using udev in content and chrome. The reason we are removing dependencies is because we are trying to build on platforms that do not have them. So if you eventually wanted to build chrome, you would have to fix chrome to support use_udev as well. I don't think we would ever want to have both a content_use_udev and a chrome_use_udev flag.
On 2013/11/19 16:28:22, spang wrote: > On 2013/11/18 23:32:25, Mostyn Bramley-Moore wrote: > > > https://codereview.chromium.org/12374068/diff/324001/content/browser/device_m... > > File content/browser/device_monitor_udev.h (right): > > > > > https://codereview.chromium.org/12374068/diff/324001/content/browser/device_m... > > content/browser/device_monitor_udev.h:8: #ifndef > > CONTENT_BROWSER_DEVICE_MONITOR_LINUX_H_ > > On 2013/11/18 22:32:19, rjkroege wrote: > > > you need to change this and the name of the class. Sorry for the > > inconvenience. > > > > Done. > > > > > https://codereview.chromium.org/12374068/diff/324001/content/content_browser.... > > File content/content_browser.gypi (right): > > > > > https://codereview.chromium.org/12374068/diff/324001/content/content_browser.... > > content/content_browser.gypi:1408: 'sources!': [ > > On 2013/11/18 22:32:19, rjkroege wrote: > > > I didn't provide adequate information. I meant you to use an entry in > > > build/filename_rules.gypi to make this unnecessary. See the use_pango > section > > > for example. The use of a naming rule can keep the gyp* file simpler. > > > > > > All of these files if named with _udev.* would be excluded by the naming > rule. > > > What do you think? > > > > It might be a little confusing since there are other uses of udev in the > chrome > > layer, and these files don't end with _udev.*. > > > > Which reminds me of something from my old CL- this setting only affects the > > content layer, should we rename it to something like content_use_udev? I > think > > this is a reasonable distinction to make (ie disabling udev in content but not > > in chrome). > > Please use the same style as the existing flags. > > It is not useful to have different flags for using udev in content and chrome. > The reason we are removing dependencies is because we are trying to build on > platforms that do not have them. So if you eventually wanted to build chrome, > you would have to fix chrome to support use_udev as well. I don't think we would > ever want to have both a content_use_udev and a chrome_use_udev flag. The product I'm working on is only interested in the content API, so from my point of view it would be ok to simply have a use_udev setting and leave a comment that this currently only affects content. Are you saying that you would like me to expand this to disable udev in chrome too? If so I can do that, but it will probably take a little longer. Would you prefer me to land this core part first, and follow up with the chrome layer in another CL a couple of days later? If so, I could still add a _udev.* naming rule as part of this CL.
On 2013/11/19 17:15:45, Mostyn Bramley-Moore wrote: > On 2013/11/19 16:28:22, spang wrote: > > On 2013/11/18 23:32:25, Mostyn Bramley-Moore wrote: > > > > > > https://codereview.chromium.org/12374068/diff/324001/content/browser/device_m... > > > File content/browser/device_monitor_udev.h (right): > > > > > > > > > https://codereview.chromium.org/12374068/diff/324001/content/browser/device_m... > > > content/browser/device_monitor_udev.h:8: #ifndef > > > CONTENT_BROWSER_DEVICE_MONITOR_LINUX_H_ > > > On 2013/11/18 22:32:19, rjkroege wrote: > > > > you need to change this and the name of the class. Sorry for the > > > inconvenience. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/12374068/diff/324001/content/content_browser.... > > > File content/content_browser.gypi (right): > > > > > > > > > https://codereview.chromium.org/12374068/diff/324001/content/content_browser.... > > > content/content_browser.gypi:1408: 'sources!': [ > > > On 2013/11/18 22:32:19, rjkroege wrote: > > > > I didn't provide adequate information. I meant you to use an entry in > > > > build/filename_rules.gypi to make this unnecessary. See the use_pango > > section > > > > for example. The use of a naming rule can keep the gyp* file simpler. > > > > > > > > All of these files if named with _udev.* would be excluded by the naming > > rule. > > > > What do you think? > > > > > > It might be a little confusing since there are other uses of udev in the > > chrome > > > layer, and these files don't end with _udev.*. > > > > > > Which reminds me of something from my old CL- this setting only affects the > > > content layer, should we rename it to something like content_use_udev? I > > think > > > this is a reasonable distinction to make (ie disabling udev in content but > not > > > in chrome). > > > > Please use the same style as the existing flags. > > > > It is not useful to have different flags for using udev in content and chrome. > > The reason we are removing dependencies is because we are trying to build on > > platforms that do not have them. So if you eventually wanted to build chrome, > > you would have to fix chrome to support use_udev as well. I don't think we > would > > ever want to have both a content_use_udev and a chrome_use_udev flag. > > The product I'm working on is only interested in the content API, so from my > point of view it would be ok to simply have a use_udev setting and leave a > comment that this currently only affects content. > That works for us too. > Are you saying that you would like me to expand this to disable udev in chrome > too? If so I can do that, but it will probably take a little longer. Would you > prefer me to land this core part first, and follow up with the chrome layer in > another CL a couple of days later? If so, I could still add a _udev.* naming > rule as part of this CL. No, I'm not asking you to make chrome build without udev. These dependency issues can be fixed incrementally. I actually think the current patch is fine. It's approved, even.
> > The product I'm working on is only interested in the content API, so from my > > point of view it would be ok to simply have a use_udev setting and leave a > > comment that this currently only affects content. > > > > That works for us too. OK- I clarified this in the comment, I'll queue this now.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mostynb@opera.com/12374068/484001
Retried try job too often on mac_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
https://codereview.chromium.org/12374068/diff/484001/content/content_browser.... File content/content_browser.gypi (right): https://codereview.chromium.org/12374068/diff/484001/content/content_browser.... content/content_browser.gypi:1264: ['OS!="linux" or use_udev==0', { Oops, you broke mac here.
https://codereview.chromium.org/12374068/diff/484001/content/content_browser.... File content/content_browser.gypi (right): https://codereview.chromium.org/12374068/diff/484001/content/content_browser.... content/content_browser.gypi:1264: ['OS!="linux" or use_udev==0', { On 2013/11/19 18:19:35, spang wrote: > Oops, you broke mac here. I think I have fixed this now, running a try job to find out (I don't have a mac handy).
https://codereview.chromium.org/12374068/diff/484001/content/content_browser.... File content/content_browser.gypi (right): https://codereview.chromium.org/12374068/diff/484001/content/content_browser.... content/content_browser.gypi:1264: ['OS!="linux" or use_udev==0', { On 2013/11/19 18:28:36, Mostyn Bramley-Moore wrote: > On 2013/11/19 18:19:35, spang wrote: > > Oops, you broke mac here. > > I think I have fixed this now, running a try job to find out (I don't have a mac > handy). You're welcome to reupload and let the CQ run the tryjobs for you.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mostynb@opera.com/12374068/744002
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mostynb@opera.com/12374068/744002
Message was sent while issue was closed.
Change committed as 236204 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
