drobilla / serd

A lightweight C library for RDF syntax
https://gitlab.com/drobilla/serd
ISC License
86 stars 15 forks source link

pkg-config file should container -DSERD_STATIC on static build #36

Closed BtbN closed 1 year ago

BtbN commented 1 year ago

With --default-library=static, using the resulting static libraries results in a bunch of linker errors, because serd.h adds __declspec(dllimport), which changes the symbol mangling, and obviously tries to import it from a DLL.

When building a static library, -DSERD_STATIC needs to be part of the cflags of the resulting pkg-config file.

This same issue exists on the entire lilv dependency chain, including lilv itself. All with their respective own macro. Not sure if I should open issues there individually?

drobilla commented 1 year ago

I don't really know if pkg-config is capable of dealing with this properly itself? There's only one Cflags. If you build just a static library, sure, but you can also build/install both. I suppose it would be an improvement to add that flag if it is just a static build, but since it seems generally broken anyway, I haven't bothered and leave this to the calling project (i.e. yeah, to build statically you need to define things, which is indeed annoying).

Support for static libraries (specifically different flags like this), and especially default-library=both is generally broken in meson so I've more or less been chalking this up as just another outcome of that. I suppose I could do as you suggest for static-only builds, but it would certainly be nice if both meson and pkg-config supported all of this properly... :/

(One ticket is fine, I'm generally applying build system changes across the whole stack at once right now anyway)

drobilla commented 1 year ago

Scratch that, after some grepping around I realize I was wrong there: there is a Cflags.private (used by libarchive, libpkgconf, twolame, etc), which seems to be where this would go.

I'm not sure if it's possible to make meson emit it, though.

drobilla commented 1 year ago

... or not quite: https://github.com/fribidi/fribidi/issues/156

drobilla commented 1 year ago

From #mesonbuild:

<drobilla> Is it possible to have meson emit a Cflags.private field in pc files (for -DLIBFOO_STATIC defines and such)?
<elibrokeit> drobilla: that would be simple to add, but does not currently exist
BtbN commented 1 year ago

pkg-config does not support Cflags.private. pkgconf however, which is getting more and more popular, does.

Yeah, there is no good way to fix a "both" build. But at least for a fully static build, emitting the flag in normal Cflags would be an improvement and at least fix my usecase, which only involves static libraries. Once meson adds support for Cflags.private, it can permanently reside in there.

drobilla commented 1 year ago

Sigh. I'm tempted to just error out for default_library=both, not just on Windows but everywhere.

Anyway, try this https://github.com/drobilla/serd/tree/static (https://github.com/drobilla/serd/commit/f20b05298bcd9b4bcc8593b10e950985f0bf9e32)

That also seems like a more proper way to do it from within meson (which is how I usually depend on serd) but I haven't really tested it.

BtbN commented 1 year ago

On 13.07.2022 22:01, David Robillard wrote:

/Sigh/. I'm tempted to just error out for default_library=both, not just on Windows but everywhere.

That seems a bit too much, and might break peoples setups that set both but actually only care about the shared lib.

Anyway, try this https://github.com/drobilla/serd/tree/static https://github.com/drobilla/serd/tree/static (f20b052 https://github.com/drobilla/serd/commit/f20b05298bcd9b4bcc8593b10e950985f0bf9e32)

That also seems like a more proper way to do it from within meson (which is how I usually depend on serd) but I haven't really tested it.

This produces the pkg config file as expected, and also looks like a good implementation to me.

drobilla commented 1 year ago

Okay, thanks. Merged as 47adc7d3d.

I'll do the same in the meson ports for the rest of the LV2 stack. Meson seems to cascade them so the defines pile up in each .pc file up a dependency chain. That seems a bit unnecessarily spammy/redundant (since the dependencies are there), but... whatever, it should work. Most build systems trim duplicates anyway.

eli-schwartz commented 1 year ago

Sigh. I'm tempted to just error out for default_library=both, not just on Windows but everywhere.

Ah, but honestly this really is just yet another Windows thing. :) The define does nothing on non-Windows platforms (it is only needed for declspec, and it literally exists as the second component of a preprocessor check that first checks if _WIN32 is defined).

So it's just the same story as Meson not supporting the Windows-specific idea of compiling both shared+static libraries using that self-same flag. Linux doesn't really care because GCC visibility attributes work differently.

drobilla commented 1 year ago

@eli-schwartz Yes, my defines are written specifically to take advantage of that pile of coincidences, but the ability to pass different flags for static and shared builds isn't just "yet another Windows thing", and even if it was... Windows has like 80% market share. You, I, or whoever else might not like it, but I don't think a build system having features that are fundamentally broken for the most common operating system is a great look...

eli-schwartz commented 1 year ago

My point was not that this doesn't need improving on Meson's end, and rather that it's only buggy for 80% of people, not 100%. :)

but the ability to pass different flags for static and shared builds isn't just "yet another Windows thing"

Well, off the top of my head that's the only case I can think of...