appstream-glib exports private symbols

[pwithnall]

appstream-glib.map currently exports every symbol which starts with as_. However, various *-private.h headers have symbols which start with as_, which means that those symbols are exported as public API from the installed library.

That’s not a major issue, but it’s also not ideal.

Two ways to fix it:

1. Explicitly list API to expose in appstream-glib.map, without wildcards, and keep that list up to date as new API is added.
2. Use a different prefix (maybe _as_) for private methods so the existing wildcard pattern doesn’t match them.

Option 1 would give you more explicit control over your public API (it’s a form of whitelisting); option 2 would give more opportunity for slip-ups (both in accidentally exposing private API as public, and in accidentally not using the right prefix when calling a private function and wondering why appstream-glib will no longer compile) but would be easier to implement retrospectively.

I’m not planning to work on this (sorry), but I thought the bug should be filed.

[hughsie]

Urgh, so known issue which has existed since the beginning of the project. In fwupd and libxmlb we keep the .map file up to date and verify it on each build, but libappstream-glib never had this. I suppose we could do one of two things:

• Add a .map file now, adding all the exported and private symbols so that ABI is 100% as before

• Add a .map file now, and only add the exported symbols. This means bumping soname which seems a lot of busy work for not a lot of gain.

Comments welcome.

[ximion]

In libappstream, all symbols are public except for the ones in *-private.h headers where GCC symbol visibility is changed explicitly. Some of the symbols even in there are however exported explicitly via a macro, to be used by the testsuite or internal tools easily. Generally though, I consider everything that isn't in a header that's installed in /usr/include to be private API and I remove and change it freely. 99% of that stuff isn't even exported and can't be linked to, and for the remaining few functions, people would have to write their own headers or do other tricks to actually use those symbols. As soon as someone does that, I don't consider that supported and they are on their own to fix any breakage. Since asglib also doesn't export -private headers, maybe making all symbols in those headers private won't actually break anything as well, as nothing uses the symbols in there?

[hughsie]

Since asglib also doesn't export -private headers, maybe making all symbols in those headers private won't actually break anything as well, as nothing uses the symbols in there?

Ohh, nothing will break, but Debian will get upset that random symbols are getting removed from the ABI.

[pwithnall]

I think they can deal with a one-time change if it means that the ABI is more well-defined in future.

[ximion]

Since asglib also doesn't export -private headers, maybe making all symbols in those headers private won't actually break anything as well, as nothing uses the symbols in there?

Ohh, nothing will break, but Debian will get upset that random symbols are getting removed from the ABI.

Not really ^^ It's a build failure so the package maintainer has a look at it, but it's also a manual process because the system can't know which symbols are supposed to be public (not without parsing headers or understanding the code). So the maintainer has to judge whether a symbol removal is actually a problem, and I happen to know that the package maintainer of appstream-glib is perfectly fine with libraries hiding symbols not published in installed headers ;-) I don't know if others (and other distros) are more dogmatic about a change like this though.

[hughsie]

where GCC symbol visibility is changed explicitly

Do you know if there is a Glib macro for this? Thanks.

[pwithnall]

Do you know if there is a Glib macro for this? Thanks.

There is not. There’s G_GNUC_INTERNAL for marking things internal, but there’s no public macro for marking things as external. GLib itself uses its GLIB_AVAILABLE_IN_* and GLIB_DEPRECATED_IN_* macros for marking things as to-be-exported, and the definitions of those macros vary depending on what kinds of deprecation warnings the code using GLib wants. The underlying (GLib-internal) macro they use is _GLIB_EXTERN.

[hughsie]

@pwithnall what about something like this^^

[pwithnall]

Looks reasonable to me, but I would have done it the other way round: explicitly mark symbols which should be exported, rather than explicitly marking symbols that shouldn’t. Have symbols as private/internal by default. Because I can guarantee that with the latter approach, newly added internal symbols will (at some point) accidentally be exposed publicly because someone forgot to add G_GNUC_INTERNAL to them.

However, that commit will work better than what you currently have, and will only occasionally cause problems. I wouldn’t block it being merged (not that it’s up to me!).

[hughsie]

@pwithnall I think the next step would be to whitelist symbols in a map file. Maybe that's the only sane step?

[pwithnall]

I think what you prototyped above is also sane, just perhaps a little less maintainable in the long term. I think whitelisting is a slightly more maintainable approach, but both approaches are sane.

If you go with whitelisting, I suggest doing the whitelisting using an annotation in the header files (like G_GNUC_INTERNAL, but for changing the visibility to public), rather than in a map file, since that means all the information about a symbol is in the header, rather than split across two files. If it were in a map file, I think it would be easier to forget to add new symbols to the map file at the time. (Although you’d catch it pretty quickly afterwards when your unit tests for the new symbol didn’t link.)