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

Avoid embedding major version number in library name #1417

Closed
wants to merge 1 commit into from

Conversation

dkg
Copy link
Contributor

@dkg dkg commented Feb 12, 2021

The indication of API and ABI stability for a C library is the SONAME,
which already includes the necessary versioning information.

We don't want users of librnp to try to build against two versions of
the library at once, so there isn't a strong reason to have two copies
of devel info (headers/pkg-config/etc) installed at once.

Two different binary shared objects with SONAMEs should already be
able to be installed concurrently, so this doesn't affect the
situation with multiple distinct tools on a system that are built
against different API or ABIs.

Closes: #1406

The indication of API and ABI stability for a C library is the SONAME,
which already includes the necessary versioning information.

We don't want users of librnp to try to build against two versions of
the library at once, so there isn't a strong reason to have two copies
of devel info (headers/pkg-config/etc) installed at once.

Two different binary shared objects with SONAMEs should already be
able to be installed concurrently, so this doesn't affect the
situation with multiple distinct tools on a system that are built
against different API or ABIs.

Closes: rnpgp#1406
@ronaldtse
Copy link
Contributor

Great thanks to @dkg for this!

There’s a fail in the cmake pkgconfig workflow and on FreeBSD — since you’ve already done a lot, perhaps we should first merge this to a separate branch, and fix those remaining builds ourselves.

Would that be okay with you?

@ronaldtse
Copy link
Contributor

And yes I am prompted by the urgency of #1447 😉

@dkg
Copy link
Contributor Author

dkg commented Mar 8, 2021

@ronaldtse i'm not sure exactly what you're asking of me -- you're welcome to merge (or rebase, or cherry-pick) any of my offered patches to whatever branch you like of course. And i also don't understand the cmake/FreeBSD issues, so yes, i would appreciate someone else™ dealing with that :)

I'd definitely prefer to see the library rename happen sooner rather than later, if there is a consensus that it should happen.

@dewyatt
Copy link
Contributor

dewyatt commented Mar 8, 2021

I think everyone is fine with the changes, but there are some issues with the pkgconfig etc.
I can fix it up here if you want @ronaldtse so we can get it into the next release maybe?

@dewyatt
Copy link
Contributor

dewyatt commented Mar 8, 2021

Oh right but our next release would probably be a major bump not a minor if we're including this.

EDIT: To expand on that, I think @ni4 is already geared up for a 0.14.1 so this would go into 0.15.0. Or did we want to change it up and target doing a 0.15.0 release instead?

@ni4
Copy link
Contributor

ni4 commented Mar 8, 2021

@dewyatt as for me it should be enough changes and updates to release v0.15.0 instead of v0.14.1.

@dkg
Copy link
Contributor Author

dkg commented Mar 8, 2021

v0.15.0 with package rename + whatever fixes needed for pkg-config/FreeBSD sounds good to me. thanks for doing this!

@dewyatt
Copy link
Contributor

dewyatt commented Mar 8, 2021

I think this ought to work (I can't seem to edit the PR):

diff --git a/.github/workflows/centos7.yml b/.github/workflows/centos7.yml
index ae542f4b..208a789a 100644
--- a/.github/workflows/centos7.yml
+++ b/.github/workflows/centos7.yml
@@ -138,15 +138,15 @@ jobs:
           ./generate
           readelf -d generate
           if [ $BUILD_SHARED_LIBS == "yes" ]; then
-            readelf -d generate | grep -q librnp-
+            readelf -d generate | grep -q librnp
           else
-            readelf -d generate | grep -qv librnp-
+            readelf -d generate | grep -qv librnp
           fi
           # remove the pkgconfig for the next test
-          pkg-config --list-all | grep -q ^librnp-
-          rm /usr/lib64/pkgconfig/librnp-*
+          pkg-config --list-all | grep -q ^librnp
+          rm /usr/lib64/pkgconfig/librnp*
           # should not be found
-          pkg-config --list-all | grep -qv ^librnp-
+          pkg-config --list-all | grep -qv ^librnp
           # build an example using cmake targets
           mkdir rnp-project
           cd rnp-project
diff --git a/cmake/librnp.pc.in b/cmake/librnp.pc.in
index 1ece62f4..0284d14b 100644
--- a/cmake/librnp.pc.in
+++ b/cmake/librnp.pc.in
@@ -1,6 +1,7 @@
 prefix=@CMAKE_INSTALL_PREFIX@
 exec_prefix=${prefix}
 libdir=${prefix}/@CMAKE_INSTALL_LIBDIR@
+includedir=${prefix}/include
 
 Name: rnp
 Description: @PACKAGE_DESCRIPTION_SHORT@

@ronaldtse ronaldtse changed the base branch from release/0.x to dkg-drop-version-suffix March 8, 2021 20:32
@ronaldtse ronaldtse changed the base branch from dkg-drop-version-suffix to release/0.x March 8, 2021 20:42
@ronaldtse
Copy link
Contributor

I have manually rebased this on top of master at a new dkg-drop-version-suffix branch (2e9096a).

Since @dewyatt is now completing it, let's close this PR. Thank you again @dkg!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants