GNOME Bugzilla – Bug 732669
Use visibility/dllexport to export symbols for GObject-Introspection
Last modified: 2015-02-07 16:49:42 UTC
Hi, From Colin's comments in bug 681820 (comments 25 & 26): ---------------------- This patch will require the most ongoing maintenance; is there a way we can use say the "dllexport" annotation on the header files instead? ... Similar for this one...these symbol lists are going to be annoying extra work to maintain. If there's a way to use an attribute in the header files, or failing that, generate these files, that'd be much preferred. ---------------------- I thought it might be a good idea, especially as GLib, GTK+, Clutter (and so) have been doing this for some time, that we export of symbols for the g-i main library, and also for the building of the test libraries, by using the visibility/dllexport method, as we wouldn't have to worry about the .symbols/.def files running out of date (and hence drop them). I will post patches to this bug in the next few days or so. With blessings, thank you!
Created attachment 279890 [details] [review] Add a version header to define macros to decorate public symbols (and the 2 private symbols, as needed) Hi, This adds a version macro header that is included either directly or indirectly by girepository's headers, so that each public symbol (and the 2 symbols in gitypelib-internal.h that is used by the tools) is decorated by a version macro, which can be later used to export a symbol and possibly display compile-time warnings for the use of deprecated API in the future. Note that as g-i follows GLib development closely, and share the same minor version numbers, the version macros follow the GLib minor numbers, so there are no GI_VERSION_1_xx as a result, but instead follows the GLIB_VERSION_2_xx convention in GLib.
Created attachment 279891 [details] [review] girepository: Include config.h first in all the C-sources Hi, This updates all the C-sources for girepository so that they will include config.h first, so that the correct symbol export compiler directive can be used to export the symbols as necessary.
Created attachment 279892 [details] [review] Update the generation of the sources for the 'Everything' test Hi, Please see the comments in this patch...
Created attachment 279893 [details] [review] Add to the test sources/headers macros to export the symbols Hi, This adds to the tests sources and headers macros that can be used to export the symbols when the test libraries are built, and includes config.h first in the C-sources so that the correct compiler directive for symbol exporting can be used during compile time. I will go into updating the autotools files for determing the compiler directive to export the symbols at configure time in the next few days, but by using "__declspec(dllexport) extern" in config.h.win32(.in) did work for me for the Visual C++ builds (i.e. no .def/.symbols files were used in the build), and the patches applied in their current state does pass 'make distcheck'. With blessings, thank you!
Review of attachment 279890 [details] [review]: looks generally good, but everything seems to be annotated in AVAILABLE_IN_ALL, so I don't know whether we should have versioned symbols for 1.32 and later.
Review of attachment 279891 [details] [review]: looks good, and should be committed regardless.
Review of attachment 279892 [details] [review]: gitestmacros.h does not exist in the previous commits; did you forget to add it?
Review of attachment 279893 [details] [review]: needs gitestmacros.h.
Review of attachment 279890 [details] [review]: Hello Emmanuele, In fact there are (such as in giinterfaceinfo.h and girepository.h, for example), but they are far between-I deduced the available version by the commit dates (vs the stable version release date). Let me know if I missed something. With blessings, thank you!
Created attachment 279899 [details] [review] Tests: Add a header for sumbol exporting Hello Emmanuele, Sorry, missed that part...oops. With blessings, thank you!
Review of attachment 279891 [details] [review]: Hello Emmanuele, Thanks for the reviews, the patch was pushed as 0ad96c5a. With blessings.
Created attachment 280040 [details] [review] Build: determine compiler directives for symbol export at configure time and drop the .def/.symbols files Hi, Here comes the patch that updates the autotools files to: -Determine the compiler directive for using visibility attributes/dllexport to export symbols for both libgirepository and the test libraries at configure time to be used during build time. -Drop all the .symbols and .def files. With blessings, thank you!
Created attachment 280041 [details] [review] Update the pre-defined config.h.win32(.in) for exporting the symbols for MSVC Builds .
Created attachment 280042 [details] [review] MSVC Project: Don't try to create and use a .def file when building libgirepository Hi, This is the last proposed patch for this bug... With blessings, and thanks for the time.
Hi, Any comments with the patchset (a.k.a ping)? Much appreciated With blessings, thank you!
Hello Emmanuele, > looks generally good, but everything seems to be annotated in AVAILABLE_IN_ALL, > so I don't know whether we should have versioned symbols for 1.32 and later. hmm..., I checked the g-i documentation page[1], and I think that I had the symbols in Index of new symbols in... for 1.34 and later annotated as such in attachment 279890 [details] [review], plus some newer APIs that were added in the development cycles after 1.32, but were not marked as "Since: ..." in the GTK-Doc blocks, as they seem to me (I think this would belong to another bug, though). Please let me know if this is okay or I missed something. With blessings, thank you! [1]: https://developer.gnome.org/gi/unstable/
Created attachment 283153 [details] [review] tests/: Add a Header For Symbol Exporting (take ii) Hi, This re-does the part where we add a header in tests/ so that the headers of the test programs/libs can include it, which can then be used to export the annotated symbols, and this patch is a dependency of attachment 278892 [details] [review]. This is re-done because: -This file needs to be installed for the autotools builds, as the installed test sources/headers will look for it. -Probably it is better that we don't depend on the girepository headers at all, as the tests that involved building introspection for themselves seem not to include them
Created attachment 283154 [details] [review] tests/: Annotate the gimarshallingtest for exporting symbols Hi, This updates the gimarshallingtests.h to include gitestmacros.h (in attachment 283153 [details] [review]) in order to annotate the symbols for export, and includes config.h first for gimarshallingtests.c...
Created attachment 283155 [details] [review] tests/scanner/: Annotate the tests headers and sources for exporting symbols Hi, This annotates the various test sources and headers to include gitestmacros.h and config.h so that their various symbols can be exported during the build (like what was done in in attachment 283154 [details] [review]). This incorporates the tests that were added a few days ago or so...
Created attachment 283156 [details] [review] Update MSVC NMake file used to build the tests Hi, This updates the NMake Makefile that is being used to build the test programs and DLLs. (In reply to comment #17) > Created an attachment (id=283153) [details] [review] > tests/: Add a Header For Symbol Exporting (take ii) > annotated symbols, and this patch is a dependency of attachment 278892 [details] [review]. Sorry, this should read "a dependency of attachment 279892 [details] [review]". With blessings, thank you!
Review of attachment 280040 [details] [review]: This looks fine to me.
Review of attachment 280041 [details] [review]: Not a win32 expert, but looks reasonable.
Review of attachment 280042 [details] [review]: Right.
Review of attachment 283153 [details] [review]: Looks OK, though I'm curious if there's a specific reason we do the indirection - i.e. why not annotate the tests directly as GI_EXTERN? Just a general sense of separating API and tests?
Review of attachment 283154 [details] [review]: Ok.
Review of attachment 283155 [details] [review]: Yep.
Review of attachment 283156 [details] [review]: Ok.
Hello Colin, Just wondering, as attachment 279890 [details] [review] is needed for many of the other patches, would attachment 279890 [details] [review] be alright for consumption? Needed to know before I go ahead to commit these. Would be nice if attachment 279892 [details] [review] gets another look, as we have attachment 283153 [details] [review], which contains the file that is needed. (In reply to comment #24) > - i.e. why not annotate the tests directly as GI_EXTERN? Just a general sense > of separating API and tests? Yeah, that's my take at it though, and thought that I didn't want to define _GLIB_EXTERN as extern again if it is not previously defined. I could use _GLIB_EXTERN if that is preferred though. With blessings, thank you!
Review of attachment 279890 [details] [review]: Right, I see that. Looks good to me then, thanks!
Created attachment 283430 [details] [review] Update the generation of the sources for the 'Everything' test (take ii) Hello Colin, Thanks for the reviews, the patches were pushed in chronological order as: attachment 279890 [details] [review]: 94380459 attachment 282153 [details] [review]: 4bfe7f1d attachment 283154 [details] [review]: 28b01ad7 attachment 283155 [details] [review]: eab36c00 attachment 280040 [details] [review]: d281b07c attachment 280041 [details] [review]: d2ff2e13 attachment 280042 [details] [review]: 902cd3f9 attachment 283156 [details] [review]: fffc66a6 Sorry, I pushed attachment 279892 [details] [review] by mistake just now and reverted it, and here is an updated version of it, as everything.[c|h] seems to be installed with the autotools builds, so it needs to be updated so that gitestmacros.h can be found after it is installed. With blessings, thank you!
Sorry, "attachment 282153 [details] [review]" should have read "attachment 283153 [details] [review]"...
Created attachment 284186 [details] [review] Update the generation of the sources for the 'Everything' test (take iii) Hi, For the generation of everything.[c|h], it also seems that as it is also installed as test data, it needs to include gitestmacros.h directly, and so the autotools build files need to have -I$(srcdir) so that gitestmacros.h can be found when builddir != srcdir. This is the updated patch for this. With blessings, thank you!
Created attachment 284187 [details] [review] Export symbols using visibility for the offsets test library Hi, This adds annotation to the offset test library sources, so that symbols from it can be exported via visibility/dllexport (sorry, offsets.[c|h] was left out in the previous patches). The autotools files are updated as well so that gitestmacros.h can be found during building. With blessings, thank you!
Created attachment 284367 [details] [review] Export symbols using visibility for the offsets test library (take ii) Hi, Apparently the build files for the offsets library and test program needed some more tinkering so that it can run alright on make check/distcheck, so here it is... With blessings, thank you!
Hi, (ping mode) Is there any chance that the remaining 2 patches be reviewed so that we can say bye to this bug by this release cycle? Greatly appreciated. Thanks for your time, with blessings!
Review of attachment 284186 [details] [review]: ::: giscanner/codegen.py @@ +52,3 @@ def _write_prelude(self, out, func): out.write(""" +_GI_TEST_EXTERN Mmmm...now we're injecting this into every use of g-ir-scanner. I don't think that's right. Possibly make use of the existing UNINSTALLED_INTROSPECTION_SRCDIR environment variable? Or a more generic fix would be to add a --with-function-prelude=_GI_TEST_EXTERN environment variable. @@ +119,3 @@ self.out_c.write(warning) + self.out_c.write(""" +#include "config.h" If we went with a command line arg, this would have to be --with-prelude='#include "config.h"' or so.
Review of attachment 284367 [details] [review]: This looks fine to me.
Created attachment 285219 [details] [review] giscanner: Add optional options to decorate symbols and include headers in generated code Hello Colin, Thanks for the reviews, attachment 284367 [details] [review] was pushed to master as 5143afb1. (In reply to comment #36) > Or a more generic fix would be to add a --with-function-prelude=_GI_TEST_EXTERN > environment variable. > --with-prelude='#include "config.h"' or so. OK, I think it's probably better to go with the generic route too, so this is my take at updating the scripts to take additional options for doing these if needed, albeit with different names. I will update the build files in the next 2 patches, one for autotools and the other for MSVC builds...
Created attachment 285220 [details] [review] Update tests/Makefile.am on generating everything.[c|h] Hi, This updates the autotools builds for generating everything.[c|h] test program sources, so that the sources will include the annotation for symbol export and the headers that define those annotations...
Created attachment 285221 [details] [review] MSVC Builds: Update Generation of everything.[c|h] Hi, This patch is like attachment 285220 [details] [review], which updates the MSVC build files that is used to build the tests for the same purpose. With blessings, thank you!
Review of attachment 285219 [details] [review]: This looks OK to me. Though I do wonder whether it would be better to instead just not pass the visibility linker flags while scanning. Did we try that? Anyways, I'm fine with this approach.
Review of attachment 285220 [details] [review]: Ok.
Review of attachment 285221 [details] [review]: Right.
Hello Colin, Thanks for the reviews. The patches were pushed as: attachment 285219 [details] [review]: c112e9830 attachment 285220 [details] [review]: 99423f410 attachment 285221 [details] [review]: 9b2effedf (In reply to comment #41) > This looks OK to me. Though I do wonder whether it would be better to instead > just not pass the visibility linker flags while scanning. Did we try that? I think I will poke into it tomorrow or so, though I will close this bug now for records, as we are getting close to a stable release. With blessings, thank you!
When upgrading from 1.41.4 to 1.41.91 I'm seeing these changed symbols: http://paste.debian.net/plain/119539 Most of them go away if I do this: http://paste.debian.net/plain/119544 Only the missing symbol left... anyone know what happened to g_typelib_error_quark ?
Created attachment 285523 [details] [review] Hide more symbols that shouldn't be exported This fixes some fallouts from commit d281b07c4aba18d3 "build: Export Symbols Using Compiler Directives"
Created attachment 285524 [details] [review] Export missing g_typelib_error_quark This symbol went missing in the conversion to -fvisibility=hidden in commit d281b07c4aba18d30c4365ef "build: Export Symbols Using Compiler Directives" Maybe it was mistakenly exported from the start since it's from an internal header...
With the two above patches there are no symbol mismatches between what 1.41.4 used to export and what 1.41.91 + patches exports ....
Review of attachment 285523 [details] [review]: Definitely, thanks!
Review of attachment 285524 [details] [review]: We should rather move it (and the #define) to girepository.h; the error quark is intended to be public. (Not that its super useful, but anyways)
Comment on attachment 285523 [details] [review] Hide more symbols that shouldn't be exported Attachment 285523 [details] pushed as c9195de - Hide more symbols that shouldn't be exported
[Mass-moving gobject-introspection tickets to its own Bugzilla product - see bug 708029. Mass-filter your bugmail for this message: introspection20150207 ]