Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pkgconf package as a replacement for pkgconfig #923

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dhomeier
Copy link
Contributor

@dhomeier dhomeier commented Jul 2, 2022

As pointed out in #918 (comment) some of the circular dependency problems caused by Freedesktop's pkgconfig are not resolved by switching to ppkg-config, since e.g. meson builds do not accept the latter as a pkg-config replacement (#996 (comment)).
This is an initial attempt to package the suggested alternative https://github.com/pkgconf/pkgconf.
Could not run tests due to a missing dependency, and shlibs layout may need discussion.
pkgconf was tested to build and work in building Pythons 3.8-3.11 and the meson build of scipy 1.10.0 on 12.4+13.2/arm64 and 10.14.6.

@dhomeier dhomeier added the new package Packages that do not yet exist in Fink. label Jul 2, 2022
@dhomeier dhomeier requested a review from dmacks July 2, 2022 00:43
@nieder
Copy link
Member

nieder commented Jul 2, 2022

  • pkgconf and pkgconfig both install %p/share/aclocal/pkg.m4.
  • There are also some packages that Dep (not just BDep) on pkg-config (I think because they wrap foo-config around pkg-config). Do we want to allow that with pkgconf?

@dhomeier
Copy link
Contributor Author

dhomeier commented Jul 2, 2022

I just added the BuildDependsOnly since the validator complained about dylibs and headers in the same package. Could of course set it to false, not sure what is preferable from the point of packaging policy.
I had @eli-schwartz's remark about meson in mind, which might use pkgconfig or a substitute at runtime, but actually they are rather BDeps of the packages using meson for the respective build.

I guess one could also create a -dev package as second splitoff including pkg.m4 etc., which then might also install a symlink to %p/bin/pkg-config as suggested in the README.md, to make it a full Replaces/Conflicts of pkgconfig.

The pkg.m4 here is slightly older (# serial 11 (pkg-config-0.29.1)) than the serial 12 from pkgconfig-0.29.2-2.

@eli-schwartz
Copy link

The serial number difference is mostly irrelevant. pkg-config bumped it in this commit: https://gitlab.freedesktop.org/pkg-config/pkg-config/-/commit/677e9248753d31c4b7ceed7805ccdc3fc508e980

There aren't any significant changes here, but this will cause aclocal to get the latest version.

In fact, other than whitespace (and post-serial-bump comment typo fixes) there's exactly one actual change, to use the lookup name rather than the saved variable name for logging. It doesn't affect usage though.

I guess pkgconf simply hasn't synced a handful of changes + the serial number.

@dhomeier
Copy link
Contributor Author

dhomeier commented Jul 3, 2022

I've removed pkg.m4 here for now; perhaps another option is to move it out off pkgconfig as well and install the latest version with pkgconfig-common – or would that confuse autoconf, expecting to find a 'pkg-config' even if none is installed?

@dmacks
Copy link
Member

dmacks commented Jul 24, 2022

We currently have each pkg-config-like program at its own name, so callers know exactly which implementation they are getting.

Debian is fairly conservative in terms of avoiding breakage during transitions. Today, they made a formal proposal that pkgconf replace pkgconfig. As part of that, they've done a full-distro rebuild and didn't see substantial problems. With that, I think we can do that switch also, having the pkgconf install bin/pkg-config as well, which means a clean Conflicts/Replaces:pkgconfig. But that would be blocked by any installed package that has Depends:pkgconfig. It feels like the clean solution is to have pkgconf:Provides:pkgconfig. However, we've had breakage at the debian level with the triplet Conflicts/Replaces/Provides:foo when foo is itself also a real package.

So we could have a final actual pkgconfig package that is a dummy on top of pkgconf (fink's usual "obsolete" mechanism for when a package is drop-in replaced by another work-alike with a different package-name). That would mean pkgconf:Replaces:pkgconfig (not also Conflicts).

@eli-schwartz
Copy link

eli-schwartz commented Aug 15, 2022

pkgconf 1.9.2 is out.

BTW it includes this change I contributed: pkgconf/pkgconf@59a56df
Which re-syncs the pkg.m4 with the freedesktop pkg-config.

@dhomeier
Copy link
Contributor Author

dhomeier commented Nov 18, 2022

I've updated to 1.9.3 and after some struggles managed to build glib2-shlibs using that; but it seems it really needs to be installed as a full replacement of pkgconfig for that (including pkg.m4).
What does not work in packages tested so far is xft2 (failing validation on building against XQuartz fontconfig etc.) and guile22 (does not compile, possibly getting wrong --std= flags).
Both are building with our old pkgconfig, so that could be a show-stopper – have to try yet if ppkg-config could serve as a replacement there.
@dmacks when making this just a Replaces but not a Conflicts to pkgconfig, Fink is still happy installing both in parallel – would the latter need to be made a dummy/obsolete package at the same time?

@eli-schwartz
Copy link

I wonder what the diff is between the generated buildsystem Makefiles / ninja files when using pkg-config vs. pkgconf?

@dhomeier
Copy link
Contributor Author

dhomeier commented Nov 21, 2022

The pkgconf build of guile22 is failing with

  CC       guile-guile.o
[gcc -std=gnu11 -DHAVE_CONFIG_H   -DBUILDING_LIBGUILE=1 -I../.. -I.. -I../../lib -I../lib -I/opt/sw3/include -I/opt/sw3/src/fink.build/guile22-2.2.7-2/guile-2.2.7 -I/opt/sw3/include -I/opt/sw3/include -D_THREAD_SAFE  -Wall -Wmissing-prototypes -Wdeclaration-after-statement -Wpointer-arith -Wswitch-enum -fno-strict-aliasing -fwrapv -fvisibility=hidden -I/opt/sw3/include -g -O2 -MT guile-guile.o -MD -MP -MF .deps/guile-guile.Tpo -c -o guile-guile.o ../../libguile/guile.c]
In file included from ../../libguile/guile.c:34:
In file included from ../../libguile.h:81:
../../libguile/pairs.h:182:1: error: static declaration of 'GC_is_heap_ptr' follows non-static declaration
GC_is_heap_ptr (void *ptr)
^
/opt/sw3/include/gc/gc.h:523:20: note: previous declaration is here
GC_API int GC_CALL GC_is_heap_ptr(const void *);
                   ^
1 error generated.
make[3]: *** [guile-guile.o] Error 1

and its differences in build/Makefile vs. the pkgconfig one are missing Fink paths in

< BDW_GC_CFLAGS = -I/opt/sw3/include
< BDW_GC_LIBS = -L/opt/sw3/lib -lgc
---
> BDW_GC_CFLAGS = 
> BDW_GC_LIBS = -lgc 
397c397
< CFLAGS = -I/opt/sw3/include -g -O2
---
> CFLAGS =  -g -O2

Although I could not spot any difference in the generated libguile headers, manually executing gcc with the same flags fails in the pkgconf build dir and succeeds in the pkgconfig-generated one; so something about dealing with the system gc/gc.h is not detected correctly by pkgconf. First I thought the Fink wrapper script (which was directly copied from the pkgconfig package description to pkgconf was not doing the right mojo with the latter, but even calling the former's binary directly with PKG_CONFIG = %p/bin/pkg-config.real gets the paths right (ppkg-config as well BTW).
The package can be built by adding

        BDW_GC_CFLAGS=-I%p/include \
        BDW_GC_LIBS="-L%p/lib -lgc" \
        LIBFFI_CFLAGS=-I%p/include \
        LIBFFI_LIBS="-L%p/lib -lffi" \

to ConfigureParams, but that's probably not very satisfactory when trying to fix the builds of future packages.

@eli-schwartz
Copy link

pkgconf probably thinks for some reason that that path is part of the "system" includes that the compiler uses even without specifying any flags. Both freedesktop pkg-config and pkgconf are supposed to skip printing those unless an option is passed to the pkg-config invocation, but the latter may have been configured differently (or autodetected the configuration differently when building the tool itself).

@dhomeier
Copy link
Contributor Author

Yes, it's obviously not needed in CFLAGS since those still come with plenty of -I%p/include anyway, but somewhere in the configuration of the gc and libffi subsystems it is missed that gc/gc.h will eventually be in the system (Fink) include path. These bits from config.log may be relevant:

pkgconf:

configure:53755: $PKG_CONFIG --exists --print-errors "$bdw_gc >= 7.2"
configure:53758: $? = 0
configure:53816: result: yes
configure:53829: checking for GC_pthread_exit
configure:53829: gcc -std=gnu11 -o conftest -g -O2 -I/opt/sw3/include conftest.c -lgc >&5
ld: library not found for -lgc
clang: error: linker command failed with exit code 1 (use -v to see invocation)

pkg-config:

configure:53755: $PKG_CONFIG --exists --print-errors "$bdw_gc >= 7.2"
configure:53758: $? = 0
configure:53816: result: yes
configure:53829: checking for GC_pthread_exit
configure:53829: gcc -std=gnu11 -o conftest -I/opt/sw3/include -g -O2 -I/opt/sw3/include conftest.c -L/opt/sw3/lib -lgc >&5
Undefined symbols for architecture x86_64:
"_GC_pthread_exit", referenced from:
_main in conftest-28e443.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

So in the first case pkgconf tries to compile without the Fink library path, probably concluding that gc is not available at all and then including the conflicting header definitions, while pkg-config finds the library at %p/lib/libgc.1.dylib, but determines that it is missing a number of pthread symbols.

Indeed the pkgconf build can also be fixed with just SetLDFLAGS: -L%p/lib instead of setting the BDW_GC_ and LIBFFI_ vars (so at leasts that's easier to figure out than the latter), but it's strange that the linker path is incomplete at all within a Fink build. Perhaps this can be fixed after all in the pkgconf wrapper script...

@dmacks
Copy link
Member

dmacks commented Nov 22, 2022

There appears to be a bug in guile's build toolchain. It's essentially never correct for a global (installed or external) -I or -L flag to come before a local (source or build directory) one. This:

-I/opt/sw3/include -I/opt/sw3/src/fink.build/guile22-2.2.7-2/guile-2.2.7

is conceptually flawed because presumably the package wants to find its own *.h whenever possible rather than seeing who-knows-what-file of a same name supplied by who-knows-what-other-package. The bug typically results from upstream confusion between *_CFLAGS variables returned from pkg-config's autoconf macros and *_CFLAGS automake variables. The pkg-config value is supposed to be passed via *_CPPFLAGS, but here it's loaded into CFLAGS in the configure script.

Edit: This is probably the same upstream bug whose same symptom was reported years ago: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=35427

@dmacks
Copy link
Member

dmacks commented Nov 22, 2022

I just hacked on guile22's build system a bit: c8a6882 might solve the problem. But if nothing else, it will give a more verbose output to help diagnose it.

@dhomeier
Copy link
Contributor Author

Edit: This is probably the same upstream bug whose same symptom was reported years ago: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=35427

Or at least its incomplete fix. pairs.h now in fact has the #ifndef HAVE_GC_IS_HEAP_PTR mentioned there, but since the macro is not defined in gc/gc.h, pkgconfig needs to add it to build/config.h

#define HAVE_GC_IS_HEAP_PTR 1

The pkgconf build doesn't, obviously incorrectly determining from not finding the gc library that the header is also not present. Without that gcc even fails with the include paths sorted correctly, probably because libguile.h includes gc/gc.h ahead of libguile/pairs.h.

I'd agree that this build is broken, and since we know how to work around it, it does not have to keep us from switching to pkgconf.

The bug typically results from upstream confusion between *_CFLAGS variables returned from pkg-config's autoconf macros and *_CFLAGS automake variables. The pkg-config value is supposed to be passed via *_CPPFLAGS, but here it's loaded into CFLAGS in the configure script.

In this case it's only done for the BDW_GC and LIBFFI versions (by pkg-config):

pkg_cv_BDW_GC_CFLAGS=${pkg_cv_BDW_GC_CFLAGS=-I/opt/sw3/include}
pkg_cv_BDW_GC_LIBS=${pkg_cv_BDW_GC_LIBS='-L/opt/sw3/lib -lgc'}
pkg_cv_LIBFFI_CFLAGS=${pkg_cv_LIBFFI_CFLAGS=-I/opt/sw3/include}
pkg_cv_LIBFFI_LIBS=${pkg_cv_LIBFFI_LIBS='-L/opt/sw3/lib -lffi'}

Again, pkgconf does not add any paths here at all.
The reason probably being that by default it does not print any system paths without the extra flags
--keep-system-cflags, --keep-system-libs; e.g. pkgconf --libs bdw-gc prints only -lgc.
Taking -L%p/lib as system path would perhaps work with one of Fink's gcc-fsf compilers, but obviously not with clang.
That is actually more or less how it's documented.

Could try to add those extra flags through the wrapper script, but if instead, following the hints in their README, I configure pkgconf with

ConfigureParams: <<
  --with-system-libdir=/usr/lib \
  --with-system-includedir=/usr/include:/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include
<<

it no longer considers the Fink paths system paths to be stripped, which lets guile22 build without any further modifications, and also fixes the validation failures in xft2-dev!
Not sure if the system-include is sufficiently general for 11.0 and later (on 12.6 the above seems to contain everything that is also in /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include).

@dhomeier
Copy link
Contributor Author

I just hacked on guile22's build system a bit: c8a6882 might solve the problem. But if nothing else, it will give a more verbose output to help diagnose it.

That came in just with my last comment on the way out.
I tested the build with that commit, and the include paths are indeed properly sorted

gcc -std=gnu11 -DHAVE_CONFIG_H -DBUILDING_LIBGUILE=1 -I../.. -I.. -I../../lib -I../lib -I/opt/x64/include -I/opt/x64/include -D_THREAD_SAFE -Wall -Wmissing-prototypes -Wdeclaration-after-statement -Wpointer-arith -Wswitch-enum -fno-strict-aliasing -fwrapv -fvisibility=hidden -g -O2 -MT guile-guile.o -MD -MP -MF .deps/guile-guile.Tpo -c -o guile-guile.o test -f 'guile.c' || echo '../../libguile/'guile.c

but with the original pkgconf that command still fails on the conflicting declarations, probably confirming that it's the order in which they are included within libguile.h. It builds with the system paths fixed in pkgconf as described in the last comment.
Note that there is also a new set of debian patches available:

Source2: http://archive.ubuntu.com/ubuntu/pool/main/g/guile-2.2/guile-2.2_2.2.7+1-4.debian.tar.xz
Source2-Checksum: SHA256(eca2e36c3ce91bd4fcadd7827d1738df19f84f63d560f751c573bc01088d6e6e)

The package builds with them the same way, but I did not notice any other differences.

@dhomeier dhomeier marked this pull request as ready for review February 15, 2023 02:13
@dhomeier
Copy link
Contributor Author

I don't see an alternative to installing this as a full Replaces/Conflicts to pkgconfig.

  1. It needs %p/share/aclocal/pkg.m4, so otherwise would have to depend on pkgconfig.
  2. With only Replaces apparently either package can install %p/bin/pkg-config.real or %p/share/aclocal/pkg.m4, leaving it undefined which package's instance is used (or opening the possibility to remove it altogether, if one of the packages is removed).
  3. Since there are packages that runtime-depend on pkgconfig, I see no easy way to split the runtime-dep part off to a pkgconf-shlibs package.

@dhomeier dhomeier changed the title Add pkgconf package as an alternative to pkgconfig Add pkgconf package as a replacement for pkgconfig Feb 15, 2023
@eli-schwartz
Copy link

pkg.m4 seems simple, they're the same file anyway. Just create another package called pkg-m4, have it use the pkgconf source tarball as its source, and implement the install routine as a single mkdir && cp.

Then have all pkg-config implementations depend on that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new package Packages that do not yet exist in Fink.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants