Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11348279:
Linux: use generated library loader for gsettings. (Closed)
|
Created:
8 years ago by Paweł Hajdan Jr. Modified:
8 years ago CC:
chromium-reviews, pam+watch_chromium.org, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionLinux: use generated library loader for gsettings.
BUG=162733
TEST=Test proxy settings on a system with GNOME3.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170544
Patch Set 1 #
Total comments: 5
Patch Set 2 : fixes #Patch Set 3 : fix Lucid #
Total comments: 19
Patch Set 4 : comment #
Messages
Total messages: 11 (0 generated)
https://codereview.chromium.org/11348279/diff/1/build/linux/system.gyp File build/linux/system.gyp (right): https://codereview.chromium.org/11348279/diff/1/build/linux/system.gyp#newcod... build/linux/system.gyp:245: 'type': 'static_library', Shouldn't this library have a 'dependency' on 'base', since you link in base/ concepts (like native_library.h) in the generated file? https://codereview.chromium.org/11348279/diff/1/net/proxy/proxy_config_servic... File net/proxy/proxy_config_service_linux.cc (left): https://codereview.chromium.org/11348279/diff/1/net/proxy/proxy_config_servic... net/proxy/proxy_config_service_linux.cc:722: typedef struct _GSettings GSettings; It's not clear to me whether this removal is safe or not. Note that in the version before your change, <gio/gio.h> was already being included (see line 13) As a result, it seems that if 1) You're compiling with USE_GIO 2) You're compiling with DLOPEN_GSETTINGS 3) You're on a system without GSettings (but with GIO) then it's possible you'll end up with missing types. It seems that your LibGioLoader should hold this forward declaration. Which complicates the generator.
https://codereview.chromium.org/11348279/diff/1/build/linux/system.gyp File build/linux/system.gyp (right): https://codereview.chromium.org/11348279/diff/1/build/linux/system.gyp#newcod... build/linux/system.gyp:245: 'type': 'static_library', On 2012/11/28 20:38:33, Ryan Sleevi wrote: > Shouldn't this library have a 'dependency' on 'base', since you link in base/ > concepts (like native_library.h) in the generated file? Done. https://codereview.chromium.org/11348279/diff/1/net/proxy/proxy_config_servic... File net/proxy/proxy_config_service_linux.cc (left): https://codereview.chromium.org/11348279/diff/1/net/proxy/proxy_config_servic... net/proxy/proxy_config_service_linux.cc:722: typedef struct _GSettings GSettings; On 2012/11/28 20:38:33, Ryan Sleevi wrote: > As a result, it seems that if > 1) You're compiling with USE_GIO > 2) You're compiling with DLOPEN_GSETTINGS > 3) You're on a system without GSettings (but with GIO) > > then it's possible you'll end up with missing types. It will fail compile. We have trybots for that. We also have use_gio gyp switch for that. I think it's fine. > It seems that your LibGioLoader should hold this forward declaration. No, that's the point of it. You should never have to forward declare anything. It introduces more potential for error, and also is additional manual step. It is *not* needed, seriously.
https://codereview.chromium.org/11348279/diff/1/net/proxy/proxy_config_servic... File net/proxy/proxy_config_service_linux.cc (left): https://codereview.chromium.org/11348279/diff/1/net/proxy/proxy_config_servic... net/proxy/proxy_config_service_linux.cc:722: typedef struct _GSettings GSettings; On 2012/11/28 21:19:46, Paweł Hajdan Jr. wrote: > On 2012/11/28 20:38:33, Ryan Sleevi wrote: > > As a result, it seems that if > > 1) You're compiling with USE_GIO > > 2) You're compiling with DLOPEN_GSETTINGS > > 3) You're on a system without GSettings (but with GIO) > > > > then it's possible you'll end up with missing types. > > It will fail compile. We have trybots for that. We also have use_gio gyp switch > for that. I think it's fine. Considering how often you're adding flags that trybots never test (use_system_foo), I think you'd be more receptive to understanding the concern of wanting to understand if and how this could break others. > > > It seems that your LibGioLoader should hold this forward declaration. > > No, that's the point of it. You should never have to forward declare anything. > It introduces more potential for error, and also is additional manual step. > > It is *not* needed, seriously. While I understand the concern, what you've proposed is not equivalent. Even when dynamically linking methods, it can be at times necessary to forward declare. A prime example of this is using ver-1.0 of a header, but attempting to use functionality introduced in ver-2.0. If linking (rather than dlopening), you can guarantee that the linker will ensure that ver-2.0 is present at *compile* time and at *execution* time, because the ver-2.0 headers are needed. However, if dynamically loading, it may be that you wish to compile with a ver-1.0 header, but use the ver-2.0 features. That's where forward declaring is necessary. This is not a purely academic thought exercise - we see this with Windows SDKs whenever there is a new release. We want to use some function introduced in the latest version (say Windows 8), but not require everyone update their SDK versions to Win8 SDK immediately. By forwarding declaring types and/or headers locally (in Chromium code), we can LoadLibrary the function without requiring the latest SDK and headers to be used. In the Linux world, this is akin to permitting to compile with headers from v1.0, but dynamically loading functions from v2.0, which is fine and safe as long as 1.0 and 2.0 are ABI compatible. This CL, in effect, requires that people who want to *compile* have the v2.0 headers installed, even if this will run on a system with v1.0 (or not at all). In the existing version, it's possible to compile and use with v1.0 headers, while still using dlopen. This is a difference, more than just mechanical transformation, and I want to confirm that this is intended, expected, and has been tested on whatever platform/distro/build config that uses these settings - which may not be trybots.
Per in-person chat, Paweł will handle making sure all the Chromium & Chrome bots are happy & can meet the compile time package dependency being introduced. If there are systems/platforms where we want to compile with v1.0 headers and then run with v2.0 features (eg: compiling on an old Ubuntu version like 10.04 with features/packages that are only 'stable' in 12.04), we'll cross that bridge in the generator, possibly with a stub header or some other means. Either way, ownership for babysitting is assigned, and so this change LGTM.
It turned out Lucid needs the stubs. PTAL.
This patch highlights what I was warning about. https://codereview.chromium.org/11348279/diff/10001/build/linux/gsettings.h File build/linux/gsettings.h (right): https://codereview.chromium.org/11348279/diff/10001/build/linux/gsettings.h#n... build/linux/gsettings.h:13: // see http://crbug.com/158577 . nit: Can you rewrite this comment to provide clearer context about what's going on here. // The GSettings API was not part of GIO until GIO version XXX, while platform YYY ships with version VVV. // To allow compiling on platform YYY when using dlopen() to determine // if GSettings is available, forward declarations are provided. // // If compiling with version XXX, these won't conflict, because // they're identical to the types defined. https://codereview.chromium.org/11348279/diff/10001/build/linux/gsettings.h#n... build/linux/gsettings.h:15: typedef struct _GSettings GSettings; I don't think this will work. In the original code, it worked because of **name scoping**. That is, the symbols were part of proxy_config_service_linux's naming scope, and thus did not conflict with the global symbols from gio.h. If you attempt to compile with version XXX (on Precise?), you will probably end up with an ODR violation because the typedef already exists. https://codereview.chromium.org/11348279/diff/10001/build/linux/gsettings.h#n... build/linux/gsettings.h:22: const gchar* const* g_settings_list_schemas(); In the existing code, these forward declarations were only used when DLOPEN_GSETTINGS was defined. This ensured that it would be a *compile* failure if you attempted to compile while using version VVV and did not also use dlopen. With this change you have made, it will be a *link* failure, which is much less desirable and arguably more subtle. https://codereview.chromium.org/11348279/diff/10001/build/linux/system.gyp File build/linux/system.gyp (right): https://codereview.chromium.org/11348279/diff/10001/build/linux/system.gyp#ne... build/linux/system.gyp:296: 'action': ['python', Seems like you should indent this consistently with the rest of the GYP files (eg, put python on the next line, like 293-294 / 284-286) https://codereview.chromium.org/11348279/diff/10001/build/linux/system.gyp#ne... build/linux/system.gyp:304: '--bundled-header', '"build/linux/gsettings.h"', Better than bundled, I think it would be more appropriate to call it 'stub' https://codereview.chromium.org/11348279/diff/10001/tools/generate_library_lo... File tools/generate_library_loader/generate_library_loader.py (right): https://codereview.chromium.org/11348279/diff/10001/tools/generate_library_lo... tools/generate_library_loader/generate_library_loader.py:196: header = options.bundled_header The handling of this option creates some degree of confusion for me, in that --bundled-header ends up replacing --header It would seem more intuitive that - By default, 'header' is always included - If a stub header is specified, and link_directly is 0, *also* include the stub header Alternatively, the stub header should always be used for --header and don't even bother with the --bundled-header option
https://codereview.chromium.org/11348279/diff/10001/build/linux/gsettings.h File build/linux/gsettings.h (right): https://codereview.chromium.org/11348279/diff/10001/build/linux/gsettings.h#n... build/linux/gsettings.h:13: // see http://crbug.com/158577 . On 2012/11/29 21:51:04, Ryan Sleevi wrote: > nit: Can you rewrite this comment to provide clearer context about what's going > on here. > > // The GSettings API was not part of GIO until GIO version XXX, while platform > YYY ships with version VVV. > // To allow compiling on platform YYY when using dlopen() to determine > // if GSettings is available, forward declarations are provided. > // > // If compiling with version XXX, these won't conflict, because > // they're identical to the types defined. Done. https://codereview.chromium.org/11348279/diff/10001/build/linux/gsettings.h#n... build/linux/gsettings.h:15: typedef struct _GSettings GSettings; On 2012/11/29 21:51:04, Ryan Sleevi wrote: > I don't think this will work. Green trybot compiles contradict your comment. It works. https://codereview.chromium.org/11348279/diff/10001/build/linux/gsettings.h#n... build/linux/gsettings.h:22: const gchar* const* g_settings_list_schemas(); On 2012/11/29 21:51:04, Ryan Sleevi wrote: > In the existing code, these forward declarations were only used when > DLOPEN_GSETTINGS was defined. This ensured that it would be a *compile* failure > if you attempted to compile while using version VVV and did not also use dlopen. > > With this change you have made, it will be a *link* failure, which is much less > desirable and arguably more subtle. Nope, when linking directly system header will be used. In other words, I know what I'm doing. This is yet another reason this generator is cool. :) https://codereview.chromium.org/11348279/diff/10001/build/linux/system.gyp File build/linux/system.gyp (right): https://codereview.chromium.org/11348279/diff/10001/build/linux/system.gyp#ne... build/linux/system.gyp:296: 'action': ['python', On 2012/11/29 21:51:04, Ryan Sleevi wrote: > Seems like you should indent this consistently with the rest of the GYP files > (eg, put python on the next line, like 293-294 / 284-286) This is consistent with libpci action usage below. https://codereview.chromium.org/11348279/diff/10001/build/linux/system.gyp#ne... build/linux/system.gyp:304: '--bundled-header', '"build/linux/gsettings.h"', On 2012/11/29 21:51:04, Ryan Sleevi wrote: > Better than bundled, I think it would be more appropriate to call it 'stub' Nope, bundled is a more accurate term. It emphasizes that the header is a part of our source tree, and _may_ be out of sync with the system library. Also, the header should contain just declarations, and "stub" may imply definitions that would forward to something else. https://codereview.chromium.org/11348279/diff/10001/tools/generate_library_lo... File tools/generate_library_loader/generate_library_loader.py (right): https://codereview.chromium.org/11348279/diff/10001/tools/generate_library_lo... tools/generate_library_loader/generate_library_loader.py:196: header = options.bundled_header On 2012/11/29 21:51:04, Ryan Sleevi wrote: > The handling of this option creates some degree of confusion for me, in that > --bundled-header ends up replacing --header Yup. Why worry though? If you copy an existing example, it's going to do the right thing. > It would seem more intuitive that > - By default, 'header' is always included That would be wrong. It may not always be present. We have cases where we only use bundled headers for dlopen. > - If a stub header is specified, and link_directly is 0, *also* include the stub > header If by *also* you mean in addition to 'header', it's equivalent to your point above. Note that when link_directly is 0, we do include the bundled header. > Alternatively, the stub header should always be used for --header and don't even > bother with the --bundled-header option Nope, that doesn't satisfy all the use cases this is intended to handle.
Hi Paweł, I've reviewed this change and I think it will work, but I do have some concerns regarding the readability/clarity of the generator. Given that Mark reviewed the original generator, and had some initial concerns regarding how developers interact with it, I think he may be best to comment on these issues. As you can see, the majority of the concerns are with 'readability', which admittedly carries some subjectivity to it. However, I think we should strive for 'as obvious as possible when reading', even if it's more work for writing. https://codereview.chromium.org/11348279/diff/10001/build/linux/gsettings.h File build/linux/gsettings.h (right): https://codereview.chromium.org/11348279/diff/10001/build/linux/gsettings.h#n... build/linux/gsettings.h:15: typedef struct _GSettings GSettings; On 2012/11/29 22:29:00, Paweł Hajdan Jr. wrote: > On 2012/11/29 21:51:04, Ryan Sleevi wrote: > > I don't think this will work. > > Green trybot compiles contradict your comment. It works. C forbids this. C++ allows this. You're getting lucky. https://codereview.chromium.org/11348279/diff/10001/build/linux/system.gyp File build/linux/system.gyp (right): https://codereview.chromium.org/11348279/diff/10001/build/linux/system.gyp#ne... build/linux/system.gyp:296: 'action': ['python', On 2012/11/29 22:29:00, Paweł Hajdan Jr. wrote: > On 2012/11/29 21:51:04, Ryan Sleevi wrote: > > Seems like you should indent this consistently with the rest of the GYP files > > (eg, put python on the next line, like 293-294 / 284-286) > > This is consistent with libpci action usage below. This is a nit, and please take it as such given that we lack a formal GYP style guide, but the preponderance of GYP files within Chromium indent nested sections. You're clearly very close to the 80 character limit, and you're using 11 characters to maintain this indent. I would suggest it's more readable and more consistent with a newline and a two space indent (for both). https://codereview.chromium.org/11348279/diff/10001/build/linux/system.gyp#ne... build/linux/system.gyp:304: '--bundled-header', '"build/linux/gsettings.h"', On 2012/11/29 22:29:00, Paweł Hajdan Jr. wrote: > On 2012/11/29 21:51:04, Ryan Sleevi wrote: > > Better than bundled, I think it would be more appropriate to call it 'stub' > > Nope, bundled is a more accurate term. It emphasizes that the header is a part > of our source tree, and _may_ be out of sync with the system library. I disagree with this naming. I find "bundled" to suggest it contains the full definitions - but this doesn't, it still depends on the system library. Further, the concept of 'stub' is something we've already introduced with the various use_system_foo headers (as forwarding to either a third_party or to the system header) and has equivalent concepts and functionality in other build systems. > > Also, the header should contain just declarations, and "stub" may imply > definitions that would forward to something else. A "stub header" is understood to provide the functionality of the real header, in some conditions. I believe it's a better name. https://codereview.chromium.org/11348279/diff/10001/tools/generate_library_lo... File tools/generate_library_loader/generate_library_loader.py (right): https://codereview.chromium.org/11348279/diff/10001/tools/generate_library_lo... tools/generate_library_loader/generate_library_loader.py:196: header = options.bundled_header On 2012/11/29 22:29:00, Paweł Hajdan Jr. wrote: > On 2012/11/29 21:51:04, Ryan Sleevi wrote: > > The handling of this option creates some degree of confusion for me, in that > > --bundled-header ends up replacing --header > > Yup. Why worry though? If you copy an existing example, it's going to do the > right thing. Code should be clear and self-documenting. I do not think the way these command-lines interact is obvious nor documented. You should not have to look at how a features is being used to understand what it is supposed to do, and we should try for the principal of least surprise here. From what I can tell, this doesn't have any documentation, and I think that's a problem that should be fixed. > > > It would seem more intuitive that > > - By default, 'header' is always included > > That would be wrong. It may not always be present. We have cases where we only > use bundled headers for dlopen. Do we? This is the only use, and you seemed initially opposed to it until it was shown necessary for this patch. And when using dlopen, you're still including the same header, just via the stub header. That is reasonably surprising, even when following "the pattern" > > > - If a stub header is specified, and link_directly is 0, *also* include the > stub > > header > > If by *also* you mean in addition to 'header', it's equivalent to your point > above. Note that when link_directly is 0, we do include the bundled header. To me, the subtlety involved here is better expressed within the stub header, not within the generator. That is, the guarding based on DLOPEN_SOMEFLAG. > > > Alternatively, the stub header should always be used for --header and don't > even > > bother with the --bundled-header option > > Nope, that doesn't satisfy all the use cases this is intended to handle. See above. I think it's cleaner to handle this in the header, not the generator. https://codereview.chromium.org/11348279/diff/10001/tools/generate_library_lo... tools/generate_library_loader/generate_library_loader.py:211: wrapped_header_include += '#define %s_DT_NEEDED\n' % unique_prefix It appears your generator violates the Google C++ Style Guide regarding Pre-processor macros: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Prepro... "Don't define macros in a .h file" "#define macros right before you use them, and #undef them right after" This explains why you are unable to place the logic in the (stub) header, as the pre-processor symbols do not exist at the time they're included. That's a subtle behaviour that I think is non-obvious, even when following "the pattern".
https://codereview.chromium.org/11348279/diff/10001/build/linux/system.gyp File build/linux/system.gyp (right): https://codereview.chromium.org/11348279/diff/10001/build/linux/system.gyp#ne... build/linux/system.gyp:296: 'action': ['python', Ryan Sleevi wrote: > On 2012/11/29 22:29:00, Paweł Hajdan Jr. wrote: > > On 2012/11/29 21:51:04, Ryan Sleevi wrote: > > > Seems like you should indent this consistently with the rest of the GYP > files > > > (eg, put python on the next line, like 293-294 / 284-286) > > > > This is consistent with libpci action usage below. > > This is a nit, and please take it as such given that we lack a formal GYP style > guide, but the preponderance of GYP files within Chromium indent nested > sections. You're clearly very close to the 80 character limit, and you're using > 11 characters to maintain this indent. > > I would suggest it's more readable and more consistent with a newline and a two > space indent (for both). What Paweł wrote here is permitted, although if things started to need to wrap, certainly the first thing you’d do to avoid wrapping is to switch to what Ryan suggests. https://codereview.chromium.org/11348279/diff/10001/tools/generate_library_lo... File tools/generate_library_loader/generate_library_loader.py (right): https://codereview.chromium.org/11348279/diff/10001/tools/generate_library_lo... tools/generate_library_loader/generate_library_loader.py:211: wrapped_header_include += '#define %s_DT_NEEDED\n' % unique_prefix Ryan Sleevi wrote: > It appears your generator violates the Google C++ Style Guide regarding > Pre-processor macros: > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Prepro... > > "Don't define macros in a .h file" > "#define macros right before you use them, and #undef them right after" > > This explains why you are unable to place the logic in the (stub) header, as the > pre-processor symbols do not exist at the time they're included. That's a subtle > behaviour that I think is non-obvious, even when following "the pattern". In the original review, I said I didn’t love this either, but was more concerned about the “what if the macro isn’t defined/what if neither is defined/what if both are defined” problems. Those have since been fixed. I wasn’t as worried about the “macro leak” problem because care was taken to choose unique path-specific names for the macros. But yeah, it’d work just as well if the logic was totally confined to the generator.
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
