Linux: use generated library loader for gsettings.

tools/generate_library_loader/generate_library_loader.py

 #!/usr/bin/env python
 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """
 Creates a library loader (a header and implementation file),
 which is a wrapper for dlopen or direct linking with given library.
 
 The loader makes it possible to have the same client code for both cases,
@@ skipping 128 matching lines @@
 IMPL_MEMBER_CLEANUP_TEMPLATE = """  %(function_name)s = NULL;
 """
 
 def main():
   parser = optparse.OptionParser()
   parser.add_option('--name')
   parser.add_option('--output-cc')
   parser.add_option('--output-h')
   parser.add_option('--header')
 
+  parser.add_option('--bundled-header')
   parser.add_option('--use-extern-c', action='store_true', default=False)
   parser.add_option('--link-directly', type=int, default=0)
 
   options, args = parser.parse_args()
 
   if not options.name:
     parser.error('Missing --name parameter')
   if not options.output_cc:
     parser.error('Missing --output-cc parameter')
   if not options.output_h:
@@ skipping 24 matching lines @@
     })
     member_init.append(IMPL_MEMBER_INIT_TEMPLATE % {
       'function_name': fn,
       'unique_prefix': unique_prefix
     })
     member_cleanup.append(IMPL_MEMBER_CLEANUP_TEMPLATE % {
       'function_name': fn,
       'unique_prefix': unique_prefix
     })
 
-  wrapped_header_include = '#include %s' % options.header
+  header = options.header
+  if options.link_directly == 0 and options.bundled_header:
+    header = options.bundled_header

[Ryan Sleevi, 2012/11/29 21:51:04]

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

[Paweł Hajdan Jr., 2012/11/29 22:29:00]

Yup. Why worry though? If you copy an existing example, it's going to do the
right thing.
That would be wrong. It may not always be present. We have cases where we only
use bundled headers for dlopen.
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.
Nope, that doesn't satisfy all the use cases this is intended to handle.

[Ryan Sleevi, 2012/11/29 23:02:58]

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.
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"
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.
See above. I think it's cleaner to handle this in the header, not the generator.

+  wrapped_header_include = '#include %s\n' % header
 
   # Some libraries (e.g. libpci) have headers that cannot be included
   # without extern "C", otherwise they cause the link to fail.
   # TODO(phajdan.jr): This is a workaround for broken headers. Remove it.
   if options.use_extern_c:
     wrapped_header_include = 'extern "C" {\n%s\n}\n' % wrapped_header_include
 
   # It seems cleaner just to have a single #define here and #ifdefs in bunch
   # of places, rather than having a different set of templates, duplicating
   # or complicating more code.
   if options.link_directly == 0:
     wrapped_header_include += '#define %s_DLOPEN\n' % unique_prefix
   elif options.link_directly == 1:
     wrapped_header_include += '#define %s_DT_NEEDED\n' % unique_prefix

[Ryan Sleevi, 2012/11/29 23:02:58]

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".

[Mark Mentovai, 2012/11/30 20:22:27]

Ryan Sleevi wrote:
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.

   else:
     parser.error('Invalid value for --link-directly. Should be 0 or 1.')
 
   # Make it easier for people to find the code generator just in case.
   # Doing it this way is more maintainable, because it's going to work
   # even if file gets moved without updating the contents.
   generator_path = os.path.abspath(__file__)
 
   header_contents = HEADER_TEMPLATE % {
     'generator_path': generator_path,
@@ skipping 21 matching lines @@
   impl_file = open(options.output_cc, 'w')
   try:
     impl_file.write(impl_contents)
   finally:
     impl_file.close()
 
   return 0
 
 if __name__ == '__main__':
   sys.exit(main())