Define XML_STATIC in pkg-config file when building a static expat

[haileys]

XML_STATIC is required to be defined when using expat as a static library, but it is not set in the generated pkgconfig when expat is built as a static library. This sets it up so that apps using a static expat via pkgconfig will automatically set the right definition.

[hartwork]

Hi @haileys before reviewing the technical details of the pull request I would like to understand the bug report side of this better, the requirements engineering side of things. I have a guess but I'd like to be sure. Could you elaborate what you did to see things fail with -DEXPAT_SHARED_LIBS=OFF, what the related error output was, which operating system(s) and compiler(s) are involved? Thanks!

[haileys]

Thanks for the quick reply @hartwork. I am experimenting with building a statically linked GTK stack to ease deployment on non-Linux systems, and the target for my work is Windows. I am indeed using -DBUILD_SHARED_LIBS=OFF, and the build system I'm using is based on pkg-config.

When XML_STATIC is not passed through to downstream libraries, the XMLIMPORT macro expands to __declspec(dllimport) and the linker fails to resolve references to libexpat, because the symbols that downstream code is expecting to link with are called __imp_XML_ParserCreate etc. This is standard name mangling on Windows for symbols imported from a DLL, but this presents a problem in my case because, as libexpat was built as a static library, it was not built expecting to be imported as a DLL and does not use the corresponding name mangling.

[hartwork]

@haileys thanks for elaborating, that makes sense. Also tested your code locally with static and shared mode and both seem to work as expected 👍

I'm a bit surprised by the existing use of _MSC_EXTENSIONS at…

libexpat/expat/lib/expat_external.h

Lines 87 to 96 in 86a3623

	#if ! defined(XML_STATIC) && ! defined(XMLIMPORT)
	# ifndef XML_BUILDING_EXPAT
	/* using Expat from an application */
	# if defined(_MSC_EXTENSIONS) && ! defined(__BEOS__) && ! defined(__CYGWIN__)
	# define XMLIMPORT __declspec(dllimport)
	# endif
	# endif
	#endif /* not defined XML_STATIC */

…which according to…

• https://learn.microsoft.com/en-us/cpp/preprocessor/predefined-macros?view=msvc-170
• https://learn.microsoft.com/en-us/cpp/build/reference/za-ze-disable-language-extensions?view=msvc-170

…is enabled by default but also means that mangling only happens with MSVC but not MinGW? My Windows is getting a little rusty, it's been 10+ years now. Do you happen to know if MinGW would need the same or not in this context? Does not need to be part of the pull request in any case, but would be great to know.

One more thing: We have two build systems here — CMake and GNU Autotools — and I wonder if expat.pc.in of the Autotools build system should do the same or not (in general), I try to keep them in sync as much as feasibly possible. Is there some hard reason why ./configure and MSVC are known to be impossible to be combined? Related again here is the question whether this is an MSVC-only topic.

[hartwork]

One more thing: We have two build systems here — CMake and GNU Autotools — and I wonder if expat.pc.in of the Autotools build system should do the same or not (in general), I try to keep them in sync as much as feasibly possible. Is there some hard reason why ./configure and MSVC are known to be impossible to be combined? Related again here is the question whether this is an MSVC-only topic.

@haileys PS: This just turned out to also cause red CI at https://github.com/libexpat/libexpat/actions/runs/7591776257/job/20687698564?pr=805 where the diff is a trailing space at the end of the line. So I guess we do need this times two. Would you be comfortable to also extend configure.ac and expat.pc.in the same way? I should mention file configure-ac-style.md also. What do you think?

[haileys]

@hartwork I had a look at making similar changes to the autotools build system, but to be honest I've never really been able to figure out autotools. I managed to generate a configure script on mingw64, but it seems to have been generated with a syntax error and failed due to that.

It doesn't look like configure.ac has any reference to XML_STATIC anyway, so I wonder, would it be acceptable if I fixed the trailing space issue so that we don't get mismatches in the generated pkgconfig files in the usual path (building a shared library), and leaving the MSVC static build support for CMake only?

[haileys]

My Windows is getting a little rusty, it's been 10+ years now. Do you happen to know if MinGW would need the same or not in this context?

Mine too I'm afraid, I use Linux usually, and I'm having to re-learn everything for this project! :) I'm not sure about MinGW

[hartwork]

@haileys is there a reason why we cannot or should not use Cflags.private?

For example:

# grep -F Cflags.private /usr/lib64/pkgconfig/xmlsec1-openssl.pc
Cflags.private: -DXMLSEC_STATIC

# diff -u0 <(pkg-config --cflags --static xmlsec1-nss | xargs -n1) <(pkg-config --cflags xmlsec1-nss | xargs -n1)
--- /dev/fd/63  2024-01-23 00:26:22.782678240 +0100
+++ /dev/fd/62  2024-01-23 00:26:22.783678250 +0100
@@ -13 +12,0 @@
--DXMLSEC_STATIC

# pkg-config --version
2.1.0

[haileys]

@hartwork Oh, I didn't know about that option! I have tried it out, applying this patch to my build instead:

diff --git a/expat/expat.pc.cmake b/expat/expat.pc.cmake
index da7a0549..56da4137 100644
--- a/expat/expat.pc.cmake
+++ b/expat/expat.pc.cmake
@@ -10,3 +10,4 @@ URL: https://libexpat.github.io/
 Libs: -L${libdir} -l$<TARGET_PROPERTY:expat,pkgconfig_$<LOWER_CASE:$<CONFIG>>_output_name>
 Libs.private: $<TARGET_PROPERTY:expat,pkgconfig_libm>
 Cflags: -I${includedir}
+Cflags.private: -DXML_STATIC
diff --git a/expat/expat.pc.in b/expat/expat.pc.in
index db080653..a53ab118 100644
--- a/expat/expat.pc.in
+++ b/expat/expat.pc.in
@@ -10,3 +10,4 @@ URL: https://libexpat.github.io/
 Libs: -L${libdir} -l@PACKAGE_NAME@
 Libs.private: @LIBM@
 Cflags: -I${includedir}
+Cflags.private: -DXML_STATIC

But it appears that it's not getting picked up for some reason when building fontconfig, which depends on expat. The unresolved external symbol errors are back, and I have confirmed by inspecting the compile_commands.json generated by meson for fontconfig that the -DXML_STATIC flag is not being used.

Including -DXML_STATIC in Cflags does however cause the flag to be passed through to the cc invocations for fontconfig

[hartwork]

Hi @haileys , thanks for playing with Cflags.private! Let me first be general and then address fontconfig.
My impression with Cflags.private was that while it's only used by a handfull of projects…

# grep -R -i -F cflags.private /usr/lib64/pkgconfig/
/usr/lib64/pkgconfig/libarchive.pc:Cflags.private: -DLIBARCHIVE_STATIC
/usr/lib64/pkgconfig/xmlsec1-nss.pc:Cflags.private: -DXMLSEC_STATIC
/usr/lib64/pkgconfig/liblzma.pc:Cflags.private: -DLZMA_API_STATIC
/usr/lib64/pkgconfig/xmlsec1-openssl.pc:Cflags.private: -DXMLSEC_STATIC

…and not mentioned at e.g. https://people.freedesktop.org/~dbn/pkg-config-guide.html (which seems to rank high on Google) that .private is in line with both Requires.private and Libs.private in function and that it would literally solve all our problems in libexpat because at least with Autotools, both a static and a shared library can be built at the same time and then putting it right into Cflags would then mean that we'd need to render two different pkg-config files — one for shared and one for static (and the trouble that may cause in ripple effect on distro and application level) — while for Libs and Requires that was not needed, while playing in the exact same realm of static-or-not problem. So my impression is that at least in theory, adding Cflags.private: -DXML_STATIC in two places would be all we need and work great for both build systems in Expat.

Now regarding fontconfig, it could either be that (a) something needs fixing on their side or (b) that their compile error revels some misunderstanding in the picture I just painted in the paragraph above. Also, there are two(?) implementations of pkg-config out there (at least histortically), maybe one is dead already, may need a closer look. In Gentoo mine is from https://gitea.treehouse.systems/ariadne/pkgconf , package name is dev-util/pkgconf, latest package version 2.1.0. The other one seems to be gone from Gentoo already, haven't looked closer yet.

For moving forward: I'd like to learn more about the fontconfig issue you run into and also which implementation of pkg(-)conf(ig) you are exposed to. What do you think?

[hartwork]

@haileys I would like to add two things:

• if you're interested in a voice call on the fontconfig topic e.g. for pair-debugging, I'd be in, my profile has my e-mail address for contact, just an idea. I'm a bit of a night owl so the time zone different should not be a killer there.
• with where we are today, a pull request adding Cflags.private: -DXML_STATIC in two places is something I can merge in no time. If you could make the pull request be about a single commit doing just that for now, that would be great.

[AsifKhan-78]

@haileys , @hartwork
I want to keep expat as dynamic (can't use it as static) as it's used by different modules, but the module I am working and consuming expat builds as static (on windows via /MT i.e. multi-threaded static linking). When I use expat build as dynamic via /MD switch, I get the error. Any suggestion

libexpat.lib(xmlparse.c.obj) : error LNK2019: unresolved external symbol __imp_rand_s referenced in function generate_hash_secret_salt

[hartwork]

@AsifKhan-78 your report/question is a bit out of place here and I don't understand your scenario in detail, yet. You can either create a new issue with more details or we can jump on a call in English or German. For the latter please find my e-mail address in my profile, so that we can co-ordinate the when and how of a call. Thank you.

[hartwork]

@haileys I would like to add two things:

• if you're interested in a voice call on the fontconfig topic e.g. for pair-debugging, I'd be in, my profile has my e-mail address for contact, just an idea. I'm a bit of a night owl so the time zone different should not be a killer there.

• with where we are today, a pull request adding Cflags.private: -DXML_STATIC in two places is something I can merge in no time. If you could make the pull request be about a single commit doing just that for now, that would be great.

@haileys any news?

[hartwork]

Closing due to no reply for two weeks, superseded by pull request #815…