secp256k1 icon indicating copy to clipboard operation
secp256k1 copied to clipboard

Provide pkg-config packages for optional modules

Open ysangkok opened this issue 4 years ago • 11 comments

This makes it way easier for dependent projects to probe whether libsecp256k1 was built with an optional module they might require.

Fixes #666

ysangkok avatar Oct 11 '19 23:10 ysangkok

Thanks! I think this looks good but I have no idea about pkg-config. @theuni Can you review this?

real-or-random avatar Oct 12 '19 00:10 real-or-random

Doesn't it kind of defeat the purpose of having modules which are experimental because they are unreiewed or patent problematic just exposed to callers like that? IMO that really just make the experimental stuff seem like blame shifting onto end users who are less qualified to deal with the consequences.

gmaxwell avatar Oct 12 '19 08:10 gmaxwell

Not every optional module is experimental, though, and I expect that longer term there will be more non-experimental but optional pieces of the codebase (simply because not every user needs them and they're clearly separated functionality). For those, something like this makes perfect sense.

Right now, only the ECDH module is experimental. If the recovery stuff would be added today, it would probably be marked as experimental too, but for historical reasons it wasn't.

There is probably a different discussion to be had about what ought to be/remain experimental, and how to properly avoid people relying on it.

sipa avatar Oct 12 '19 15:10 sipa

My long time experience is that optional caller-visible features in libraries on 'big systems' is pretty negative: All programs on the system share the same library so the system wide library needs to have all the optional parts turned on. When parts get left off (e.g. when things get upgraded without package configs changing), you get mysterious failures. Or you get stuff like applications where chunks of their functionality are silently omitted because some dependency three levels down had a build flag forgotten.

Having components which can be optionally disabled in specialized embedded cases is another matter-- I think that works out fine. But those cases don't use pkg-config, so this wouldn't help them, nor do embedded-on-big-system cases like libsecp256k1 in bitcoin use pkg-config. AFAIK only the system-library sort of usage does, and that's precisely where optional library features mostly don't work.

I don't think this change is harmful (except for the point about experimental features) but I think it's not a very good alternative to making all first class features (stuff that desktop applications could depend on being available) on by default unless specifically disabled.

For those, something like this makes perfect sense.

Pkg-conf can detect the incompatibility but it doesn't do anything about it, you're still stuck with something that doesn't work, it can't tell you how to fix it. A functionality test (e.g. trying to link the relevant functions) is an much stronger test for the functionality being available (which also does nothing about making it work).

Historically the standard way of dealing with interface compatibility is via tests rather than sniffing for a variety of reasons, but mostly because a test is definitive. I would argue that it would be much better to just ship m4 tests for the modules that users could use, except not everything that uses pkg-config uses configure.

What happens when something that used to be an optional module becomes a mandatory feature, e.g. because it matures or because some other component starts using it internally? A test would pass (presuming the interface didn't also change at the same time), but checking for flags wouldn't unless a special effort was made to preserve all historical flags.

gmaxwell avatar Oct 13 '19 00:10 gmaxwell

The error messages from linkers are much more incomprehensible than error messages from build systems.

Of course you are right that you only know if it works if you can run and test it, but these package offer an extra option of probing. In case the flag becomes a standard feature, the file can be installed always, the pkg-config files are tiny. And it is not likely to happen very often given that experimental features tend to go in the zkp library.

The reason I am requesting this, is because I'd like to probe for ECDH support from Cabal. Cabal supports pkg-config, but it doesn't use m4.

Cabal's solver can take your pkg-config list into consideration before choosing build flags for packages. It cannot probe for symbol existence.

If compilation were really the only proper way to probe, we wouldn't need packages or version numbers at all! But this is not the status quo.

If there is no easy way to probe for ECDH support, people are pushed towards just bundling libsecp256k1 builds everywhere. This remains an option either way, but I think it is too extreme if secp256k1 actively discourages OS packaging.

ysangkok avatar Oct 13 '19 22:10 ysangkok

I don't think this change is harmful (except for the point about experimental features) but I think it's not a very good alternative to making all first class features (stuff that desktop applications could depend on being available) on by default unless specifically disabled.

We should indeed enable all non-experimental features by default. Let's assume we do this. Would it still make sense to add the additional package files? Is this common?

edit: @gmaxwell As far as I understand your comment, we should enable features by default and then the main problem is gone. But my understanding is also that the additional packages files would still be useful metadata for others to query.

real-or-random avatar Apr 15 '20 14:04 real-or-random

I don't believe I've ever personally encountered multiple package files for build time optional features, though I'm sure it exists. It's much more common to use feature testing (or... even more common for downstream users to just have linking fail ... ).

gmaxwell avatar Apr 15 '20 16:04 gmaxwell

It's much more common to use feature testing

Normally, people use version numbers to solve this problem. For example, almost all Linux distributions have version numbers bounds on a package they depend on. Packages typically do not have optional features.

That's why you're not gonna find many packages providing more than one pkg-config module.

The version number approach translates well to binary packages since a package remains backwards compatible after adding symbols. Most people use binary package managers. Windows even had the practice of installing multiple versions of the C++ runtime library implementation, where you could consider the version number part of the package name.

Since libsecp256k1 doesn't have releases or version numbers, that excludes this project from officially interacting with any binary package manager. This is what makes libsecp256k1 different.

The alternative of trying to use a symbol and then failing if it doesn't link, is AFAIK popularized by autotools, which predates pkg-config.

As previously mentioned, pkg-config is an addition that doesn't interfere with users who are not using pkg-config, so you are not forced to use it. Consumers can use AC_CHECK_DECL if they so desire. But since pkg-config is already used in this project, I don't see why you wouldn't use it properly.

ysangkok avatar Apr 15 '20 17:04 ysangkok

Since libsecp256k1 doesn't have releases or version numbers, that excludes this project from officially interacting with any binary package manager. This is what makes libsecp256k1 different.

That's why I'm reviving this PR. If we make a release and we want this to be included, then it will be good to merge this before the release.

So far with my limited pkg-config background, I still think this is a useful addition to the project.

real-or-random avatar Apr 15 '20 19:04 real-or-random

@luke-jr I saw you commented on #817 regarding pkg-config. I thought I'd like to notify you of this PR since it uses pkg-config to signal available modules. You suggested multiple having multiple shared objects, which is a bit different. Of course this is mutually exclusive with #817, if the deprecation means removal.

ysangkok avatar Nov 01 '20 01:11 ysangkok

I see several other packages on my system with module pkg-config files:

  • OktetaCore OktetaGui
  • Qt5Concurrent Qt5Core Qt5DBus ... (many)
  • SDL_image SDL_mixer SDL_net SDL_ttf
  • alsa alsa-topology
  • avahi-client avahi-compat-libdns_sd avahi-core avahi-glib avahi-qt5 ...
  • cairo-egl cairo-fc cairo-ft cairo-gl ...
  • fftw3 fftw3_omp fftw3_threads
  • gstreamer-1.0 gstreamer-allocators-1.0 gstreamer-app-1.0 gstreamer-audio-1.0 ...
  • gtk+-3.0 gtk+-unix-print-3.0 gtk+-x11-3.0
  • libevent libevent_core libevent_extra libevent_openssl libevent_pthreads

nor do embedded-on-big-system cases like libsecp256k1 in bitcoin use pkg-config.

Bitcoin Knots does, and could arguably benefit from this now that the consensus code requires optional features.

luke-jr avatar Nov 01 '20 01:11 luke-jr