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

build: Make sure to expose p11_kit_check_version #648

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ueno
Copy link
Member

@ueno ueno commented Jul 7, 2024

Previously, version.h was not installed when building with Autotools, and the function symbol was not exposed from libp11-kit when building with Meson.

Reported by Andreas Metzler in:
#637 (comment)

@coveralls
Copy link

Coverage Status

coverage: 69.533%. remained the same
when pulling 0b9137b on ueno:wip/dueno/version-api3
into 078acdd on p11-glue:master.

@coveralls
Copy link

Coverage Status

coverage: 69.533%. remained the same
when pulling 34aa02a on ueno:wip/dueno/version-api3
into 078acdd on p11-glue:master.

@ametzler
Copy link

ametzler commented Jul 7, 2024

The newly added symbol should not be versioned LIBP11_KIT_1.1 but like all other symbols LIBP11_KIT_1.0. The symbol needs to be added to p11-kit/libp11-kit-0.dll.def, too.

@ueno
Copy link
Member Author

ueno commented Jul 7, 2024

The newly added symbol should not be versioned LIBP11_KIT_1.1 but like all other symbols LIBP11_KIT_1.0.

Could you elaborate why? I think versioning symbols is a good thing to do.

Previously, version.h was not installed when building with Autotools,
and the function symbol was not exposed from libp11-kit when building
with Meson.

Reported by Andreas Metzler in:
p11-glue#637 (comment)

Signed-off-by: Daiki Ueno <[email protected]>
@coveralls
Copy link

Coverage Status

coverage: 69.537% (+0.004%) from 69.533%
when pulling 26bc8ed on ueno:wip/dueno/version-api3
into 078acdd on p11-glue:master.

@ametzler
Copy link

ametzler commented Jul 7, 2024

The newly added symbol should not be versioned LIBP11_KIT_1.1 but like all other symbols LIBP11_KIT_1.0.

Could you elaborate why? I think versioning symbols is a good thing to do.

Afaik there are 2.5 major usecases for versioned symbols:

  • The glibc way: Use symbol versioning to allow extending the ABI without breaking it by providing the same symbol with different versions. Software built against the older glibc version sees the old compatibility version of the symbol, while software built against the latest glibc uses the newer version.
  • Version symbols to avoid crashes when sotfware is linked transitively against two versions of a library (directly against libfoo.so.2 and indirectly via libbar.so.3 against libfoo.so.1) The versioning is used to make sure func() from libfoo.so.2 is used by the software and func() from libfoo.so.1 is used by libbar.so.3. To get this the versioning for all symbols needs to change whenever the soname changes. That is what p11-kit has been using.
  • As an extension to the previous usecase also use the version to provide finer grained dependency information for rpm based distributions, by versioning all new symbols in a release with a different tag. That is what gnutls is using. Afaik the rpm side basically works like this: When rpm sees a reference to gnutls_privkey_derive_secret@GNUTLS_3_8_2 it adds a dependency on libgnutls.so.(GNUTLS_3_8_2) and the respective gnutls packages provides libgnutls.so.(GNUTLS_3_8_2). Whenever the soname changes the versioning still changes. The minimal downside is that feature backports must be avoided, i.e. I may not patch 3.7.9 to add gnutls_privkey_derive_secret().

My assertion "the newly added symbol should not be" is based on how addition of new symbols was handled in p11-kit previously, most recently in 466a31b and the fact that CONTRIBUTING.md does not describe the gnutls' way like gnutls' CONTRIBUTING.md does.

An old thread on gnutls-devel provides some more background: https://lists.gnupg.org/pipermail/gnutls-devel/2017-August/008475.html

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.

3 participants