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

Provide pkg-config packages for optional modules #675

Closed
wants to merge 1 commit into from

Conversation

ysangkok
Copy link

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

Fixes #666

This makes it way easier for dependent projects
to probe whether libsecp256k1 was built with an
optional module they might require.
@real-or-random
Copy link
Contributor

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

@gmaxwell
Copy link
Contributor

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.

@sipa
Copy link
Contributor

sipa commented Oct 12, 2019

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.

@gmaxwell
Copy link
Contributor

gmaxwell commented Oct 13, 2019

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.

@ysangkok
Copy link
Author

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.

@real-or-random
Copy link
Contributor

real-or-random commented Apr 15, 2020

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.

@gmaxwell
Copy link
Contributor

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 ... ).

@ysangkok
Copy link
Author

ysangkok commented Apr 15, 2020

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.

@real-or-random
Copy link
Contributor

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.

@ysangkok
Copy link
Author

ysangkok commented Nov 1, 2020

@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.

@luke-jr
Copy link
Member

luke-jr commented Nov 1, 2020

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.

@sipa
Copy link
Contributor

sipa commented May 8, 2023

Closing this. All modules which we recommend for use are now enabled by default.

@sipa sipa closed this May 8, 2023
@ysangkok ysangkok deleted the master branch May 8, 2023 16:01
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.

Provide pkg-config modules for optional modules
5 participants