Generate and install `libgap.pc` for use with `pkg-config` #5080

dimpase · 2022-10-04T19:26:44Z

Implement generation and installation of libgap.pc, to be used with pkg-config, as discussed in #275

Text for release notes

see title

fingolfin

Thank you for working on this, much appreciated!

is there a good way to test this works as intended? Like a minimal invocation of the pkg-config tool we could put into a test script?

libgap.pc.in

dimpase · 2022-10-05T13:28:51Z

I'll see how to add a test with pkg-config.

dimpase · 2022-10-05T20:48:32Z

By the way, shouldn't install-libgap make target have install-headers as a dependency?

dimpase · 2022-10-05T22:10:53Z

it's also a bit strange that install does not depend on all, and so make install fails if make was not called before.

dimpase · 2022-10-06T16:48:27Z

Not sure where the tests I added should be chained. Please advice.

dimpase · 2022-10-06T16:56:20Z

I guess we want to have it tested on CI.

dimpase · 2022-10-06T17:01:12Z

I don't grok .github/workflows/CI.yml - e.g. where is what's happening in teststandard specified?

configure.ac

libgap.pc.in

Makefile.rules

dimpase · 2022-10-06T22:54:50Z

One more discrepancy I noticed: in libgap tests code, includes don't have gap/ prefix, and this could potentially confuse
downstream, who would use installed GAP headers, which do come with gap/ prefix.
In the test I work around it by appending /gap to the -I flag, but this shouldn't be needed.

IMHO libgap tests should use gap/ prefix in includes, maybe at expense of creating a symbolic link to (uninstalled) headers?

dimpase · 2022-10-07T12:44:55Z

As far as I can see, pkg-config is not available in testmakeinstall CI "matrix row"

no, it's actually a bug in makefile I introduced. I can't call pkg-config ahead of make install-libgap done

dimpase · 2022-10-07T13:51:49Z

OK, we're almost there. The test now fails as follows:

Package gmp was not found in the pkg-config search path.
Perhaps you should add the directory containing `gmp.pc'
to the PKG_CONFIG_PATH environment variable
Package 'gmp', required by 'libgap', not found

So the question is where to look for gmp.pc ? Is it because GMP is vendored? Or we're on an outdated system which does not install gmp.pc, while installing GMP?

dimpase · 2022-10-07T14:01:50Z

Hmm, no, libgmp-dev is installed all right, but on Ubuntu Bionic gmp.pc is not installed, cf https://packages.ubuntu.com/bionic/amd64/libgmp-dev/filelist (because it's quite old, it's GMP 6.1, not 6.2).

Should we move to newer Ubuntu (Bionic will reach end of standard support in April 2023) ?

fingolfin · 2022-10-07T14:58:49Z

We want to update to a newer Ubuntu, see #5009 but also see PR #5026 and the problem there (32bit support seems to be going away)

fingolfin · 2022-10-07T15:04:09Z

I've updated PR #5026 now so that we could merge it (if e.g. @ChrisJefferson approves it), at which point the tests would be running in 20.04 or 22.04 (I forgot and didn't check but either has gmp.pc)

fingolfin · 2022-10-07T15:08:12Z

it's also a bit strange that install does not depend on all, and so make install fails if make was not called before.

It should depend on all relevant targets, such as libgap, gap etc. with one exception: the manuals; because make doc is currently not idempotent and it is a pest to have it re-run pdflatex again and again for no good reason. (This is "documented" in a comment in install-doc). If someone were to submit a PR to make make doc idempotent, we could fix that. But for now it seemed like a low priority; the general mantra for installing software using autoconf has always been ./configure && make && make install

dimpase · 2022-10-07T21:42:35Z

well, is it done, or something else needs to be done, still, here?

dimpase · 2022-10-11T13:10:12Z

CI is OK (except 32-bit one, which is due to broken system packages)

fingolfin

Thank you for working on this.

And my apologies, but yeah I still do have concerns... I can also try to take care of them myself later, but here are my remarks anyway

fingolfin · 2022-10-10T23:27:07Z

Makefile.rules

+
+# test libgap's pkg-config's libgap flags fetching
+tst/testpkgconfigbuild: install
+	$(eval PKG_REPORTED_CFLAGS := $(shell PKG_CONFIG_PATH=$(DESTDIR)$(libdir)/pkgconfig pkg-config --cflags libgap | cut -d' ' -f 1)/gap)


Why the cut?

Ahh, I think it's to append /gap to the first of include paths returned by--cflags, right? But why to the first, and why is it OK to discard any other? Are there other?

we need cut, as pkg-config --cflags adds a trailing space. For it's designed for convenience in the normal setups, not when you all of a sudden must add a nontrivial suffix for the include path. As I mentioned in a comment somewhere here, I'd rather have all the test code for libgap use headers with explicit gap/ prefix - then we'd need this weird hackery.

I have to add gap/ here as libgap testing code uses #include <foo.h> rather than #include <gap/foo.h>.

fingolfin · 2022-10-11T14:03:50Z

.gitignore

@@ -13,6 +13,7 @@
 /confdefs.h
 /conftest*
 /libtool
+/libgap.pc


BTW, why libgap.pc and not gap.pc? (I don't want to bikeshed, and I am fine with keeping it as-is! I am merely curious and wondering if there is a deeper rationale for these names that you are aware of. Looking at the pkgconfig dir of one of my machines, I see gap.pc, mpfr.pc, zlib.pc yet libcurl.pc and libzmq.pc -- so it seems pretty much random?

To me, libgap.pc is pretty much what's needed to include libgap in the project. But gap.pc is a bit unclear.

fingolfin · 2022-10-11T14:06:39Z

libgap.pc.in

+Description: Library containing the kernel of GAP, a computer algebra system
+URL: https://www.gap-system.org
+Version: @PACKAGE_VERSION@
+Requires.private: gmp


wait, doesn't it also require zlib, and possibly readline, depending on the install options? Otherwise static linking won't work after all, will it?

Did you test this at all? Static linking libgmp doesn't strike me as very useful, so perhaps this line should just be removed?

I mean, I am also fine with keeping it, and won't block this PR over it; but it seems misleading?

what, for libgap? Does libgap depend on zlib and readline?

Sure, we always link against libz. Whether we link against GNU readline depends on configure.

perhaps, we should just remove Requires line, and wait for possible bug reports (from some clever Linux distro which likes to underlink stuff...)

Removing it would be fine by me.

libgap.pc.in

fingolfin · 2022-10-11T14:13:31Z

Makefile.rules

@@ -1268,6 +1271,27 @@ testlibgap: ${LIBGAPTESTS}
 		$(v) -A -l $(top_srcdir) -q -T --nointeract >$(v).out && \
 		diff $(top_srcdir)/$(v).expect $(v).out && ) :

+# test libgap's pkg-config's libgap version reporting
+tst/testpkgconfigversion: install-libgap


This target does not actually create tst/testpkgconfigversion and as such should be added to .PHONY (and then perhaps renamed testpkgconfigversion).

Suggested change

tst/testpkgconfigversion: install-libgap

.PHONY: testpkgconfigversion

testpkgconfigversion: install-libgap

Though it also does not set an exit code, so it'll never fail... Perhaps add an exit 1 in a suitable place?

Or just remove it here and add the same code into ci.sh, where it can be rewritten without mangling shell code ;-). (The expected version could then be computed via make print-GAP_VERSION)

I'd rather keep the make version, as it allows testing locally in a normal way.

I don't quite get the removal of tst/ - I thought it's a convention for test targets, no?

No other test target has a tst/ prefix.

And you can invoke dev/ci.sh locally, I regularly do it. But as I said, i won't quibble over this

No other test target has a tst/ prefix.

how about Makefile.rules:tst/testlibgap/%: ? Or Makefile.rules:tst/testkernel/%: ?

Those are not test targets (in particular, they are not PHONY) but rather regular file targets that are then used by the test targets testlibgap and testkernel

.github/workflows/CI.yml

Makefile.rules

dev/ci.sh

.github/workflows/CI.yml

dimpase · 2022-10-11T15:46:35Z

On Tue, 11 Oct 2022, 16:27 Max Horn, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Makefile.rules <#5080 (comment)>: > @@ -535,6 +535,7 @@ clean: rm -f bin/gap.sh sysinfo.gap* rm -f gap$(EXEEXT) gac ffgen rm -f libgap.la + rm -f libgap.pc You haven't answered my question yet: is the file libgap.pc ever useful in the non-installed version? I am going to assume the answer is "no".

nothing precludes you from using it, upon making sure it is in pkg-config PKG_... path. But indeed, normally it is used installed. —

…

Reply to this email directly, view it on GitHub <#5080 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJXYHBCOUXDFD5THCL2ACLWCWBPJANCNFSM6AAAAAAQ44ALHA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

Co-authored-by: Max Horn <[email protected]>

to execute them, run make tst/testpkgconfigbuild tst/testpkgconfigversion

Co-authored-by: Max Horn <[email protected]>

use @GAP_VERSION@ for Version Co-authored-by: Max Horn <[email protected]>

Co-authored-by: Max Horn <[email protected]>

fingolfin

Thank you!

dimpase · 2022-10-14T11:27:28Z

thanks!

dimpase requested a review from fingolfin October 4, 2022 19:26

fingolfin added topic: build system release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes labels Oct 4, 2022

fingolfin requested changes Oct 5, 2022

View reviewed changes

libgap.pc.in Outdated Show resolved Hide resolved

libgap.pc.in Outdated Show resolved Hide resolved

fingolfin reviewed Oct 5, 2022

View reviewed changes

libgap.pc.in Outdated Show resolved Hide resolved

dimpase requested a review from fingolfin October 6, 2022 16:48

fingolfin reviewed Oct 6, 2022

View reviewed changes

configure.ac Show resolved Hide resolved

libgap.pc.in Show resolved Hide resolved

libgap.pc.in Outdated Show resolved Hide resolved

Makefile.rules Show resolved Hide resolved

collares mentioned this pull request Oct 9, 2022

gap: 4.11.1 -> 4.12.1 NixOS/nixpkgs#192548

Merged

13 tasks

dimpase requested a review from fingolfin October 11, 2022 13:10

fingolfin added the topic: libgap things related to libgap label Oct 11, 2022

fingolfin reviewed Oct 11, 2022

View reviewed changes

.github/workflows/CI.yml Outdated Show resolved Hide resolved

dimpase and others added 2 commits October 12, 2022 12:29

adding libgap.pc generation and installation

89335ce

Update pkg's description in libgap.pc.in

4502c27

Co-authored-by: Max Horn <[email protected]>

dimpase and others added 14 commits October 12, 2022 12:29

no need for libgpmxx in libgap.pc.in

5ba04f7

Co-authored-by: Max Horn <[email protected]>

"Requires: gmp" instead of "-lgmp"

ace09ff

added tests for pkg-config output

61ab1c2

to execute them, run make tst/testpkgconfigbuild tst/testpkgconfigversion

URL in libgap.pc.in

6fef5de

Co-authored-by: Max Horn <[email protected]>

make gmp private in libgap.pc.in

5ceffbe

Co-authored-by: Max Horn <[email protected]>

add libgap.pc target

4215559

add CI tests for pkg-config integration

b65086f

put ;; in the right place

a310a5d

lauch make in $SRCDIR

3851869

two separate make calls for better error reporing

dda5fea

moved PKG_ vars into rules' bodies

5306b4f

Update libgap.pc.in

22e8e22

Co-authored-by: Max Horn <[email protected]>

Update libgap.pc.in

4a637de

use @GAP_VERSION@ for Version Co-authored-by: Max Horn <[email protected]>

add "exit 1" to the rule testing libgap version output

fd5af12

Co-authored-by: Max Horn <[email protected]>

dimpase force-pushed the pc_file branch from 2f58c4f to fd5af12 Compare October 12, 2022 11:32

dimpase and others added 5 commits October 13, 2022 17:30

install libgap.pc with 0644 mask

635262e

Co-authored-by: Max Horn <[email protected]>

more installing pkg-config to make 32-bit happy

ebd754d

removed "Required...:"

d132d7b

remove tst/ and make targets PHONY

2bd19b0

adjust target names, remove tst/

c0b8513

fingolfin approved these changes Oct 14, 2022

View reviewed changes

fingolfin merged commit d298b42 into gap-system:master Oct 14, 2022

dimpase deleted the pc_file branch October 14, 2022 11:29

dimpase mentioned this pull request Oct 14, 2022

Add a gap-config script #275

Closed

fingolfin mentioned this pull request Oct 14, 2022

Regressions caused by libgap.pc PR #5113

Closed

fingolfin changed the title ~~adding libgap.pc generation and installation, to be used with pkg-config~~ Generate and install libgap.pc for use with pkg-config Jan 28, 2024

fingolfin mentioned this pull request Apr 18, 2024

autotools build system should create pkgconfig file Normaliz/Normaliz#114

Open

dimpase mentioned this pull request Apr 18, 2024

pkg-config to make using libnormaliz easier; for determining the correct CPPFLAGS/CXXFLAGS/LDFLAGS etc Normaliz/Normaliz#417

Open

	tst/testpkgconfigversion: install-libgap
	.PHONY: testpkgconfigversion
	testpkgconfigversion: install-libgap

Generate and install libgap.pc for use with pkg-config #5080

Generate and install libgap.pc for use with pkg-config #5080

Conversation

dimpase commented Oct 4, 2022

Text for release notes

fingolfin left a comment

Choose a reason for hiding this comment

dimpase commented Oct 5, 2022

dimpase commented Oct 5, 2022

dimpase commented Oct 5, 2022

dimpase commented Oct 6, 2022

dimpase commented Oct 6, 2022

dimpase commented Oct 6, 2022

dimpase commented Oct 6, 2022

dimpase commented Oct 7, 2022 • edited Loading

dimpase commented Oct 7, 2022

dimpase commented Oct 7, 2022

fingolfin commented Oct 7, 2022

fingolfin commented Oct 7, 2022

fingolfin commented Oct 7, 2022

dimpase commented Oct 7, 2022

dimpase commented Oct 11, 2022

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimpase commented Oct 11, 2022 via email

fingolfin left a comment

Choose a reason for hiding this comment

dimpase commented Oct 14, 2022

Generate and install `libgap.pc` for use with `pkg-config` #5080

Generate and install `libgap.pc` for use with `pkg-config` #5080

dimpase commented Oct 7, 2022 •

edited

Loading