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

Added pkg-config file and .gitignore #86

Closed
wants to merge 1 commit into from

Conversation

lavoiesl
Copy link

@lavoiesl lavoiesl commented Oct 28, 2019

This is a redo of #55, which had merge conflicts and seemed abandoned.

I implemented the suggested fix of using the variables provided by GNUInstallDirs

A notable difference is that the snappy.pc is now installed in <prefix>/lib/pkgconfig instead of <prefix>/share/pkgconfig, which seems to be more in line with the other packages installing libraries.

Grabbed the absolute/relative logic from rtrlib/rtrlib#150; when building in Nixos, it overrides GNUInstallDirs to absolute paths, which may not be in the prefix:

Verification using a relative CMAKE_INSTALL_LIBDIR:

$ cat libsnappy.pc
prefix=/usr/local
exec_prefix=/usr/local
libdir=${prefix}/lib
includedir=${prefix}/include

Name: snappy
Description: Fast compressor/decompressor library.
Version: 1.1.7
Libs: -L${prefix}/lib -lsnappy
Cflags: -I${prefix}/include
$ pkg-config libsnappy --libs
-L/usr/local/lib -lsnappy
$ pkg-config libsnappy --cflags
-I/usr/local/include

With an absolute CMAKE_INSTALL_LIBDIR, building with nix:

prefix=/nix/store/hknzbw87dnmz7zq0lf99vjf7bf2l2hps-snappy-1.1.7
exec_prefix=/nix/store/hknzbw87dnmz7zq0lf99vjf7bf2l2hps-snappy-1.1.7
libdir=/nix/store/hknzbw87dnmz7zq0lf99vjf7bf2l2hps-snappy-1.1.7/lib
includedir=/nix/store/z67qhiawswr2jq1p71r4f2987czx3slh-snappy-1.1.7-dev/include

Name: snappy
Description: Fast compressor/decompressor library.
Version: 1.1.7
Libs: -L/nix/store/hknzbw87dnmz7zq0lf99vjf7bf2l2hps-snappy-1.1.7/lib -lsnappy
Cflags: -I/nix/store/z67qhiawswr2jq1p71r4f2987czx3slh-snappy-1.1.7-dev/include
$ pkg-config libsnappy --libs
-L/nix/store/hknzbw87dnmz7zq0lf99vjf7bf2l2hps-snappy-1.1.7/lib -lsnappy
$ pkg-config libsnappy --cflags
-I/nix/store/z67qhiawswr2jq1p71r4f2987czx3slh-snappy-1.1.7-dev/include

Installing the snappy gem (i.e. it properly detected it and didn't recompile snappy):

$ time gem install snappy
Fetching snappy-0.0.17.gem
Building native extensions. This could take a while...
Successfully installed snappy-0.0.17
Parsing documentation for snappy-0.0.17
Installing ri documentation for snappy-0.0.17
Done installing documentation for snappy after 0 seconds
1 gem installed
gem install snappy  1.25s user 0.57s system 80% cpu 2.270 total

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@lavoiesl
Copy link
Author

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@lavoiesl
Copy link
Author

lavoiesl commented Nov 4, 2019

Is there anything else needed on this PR?

@pwnall pwnall added the wontfix label Nov 10, 2019
@pwnall
Copy link
Member

pwnall commented Nov 10, 2019

We will not add pkg-config support for now.

We don't want to get into supporting individual package managers, because the C++ ecosystem hasn't converged into a single solution, and there are quite a few equally reasonable candidates. We can't justify the investment into supporting all the package managers, or into supporting a couple of package managers and trying to rationalize where we drew the line.

@pwnall pwnall closed this Nov 10, 2019
@lavoiesl
Copy link
Author

I am confused by this decision. pkg-config is fairly standard and the vast majority of libraries I work with that provide headers and dylibs also provide a pkg-confg file.

Without this PR, I am left with the option to add the file via the package managers I would use (homebrew, apt, yum, nix, etc.), which is a ton of repetitive configuration.

It only works currently without a pkg-config because people typically install on a prefix like /usr/local, which most tools look into automatically.

i.e. It only works by accident.

@lavoiesl
Copy link
Author

lavoiesl commented Dec 19, 2019

Someone else from my team has also experienced problems compiling the snappy gem on macOS catalina.

They fixed by:

  1. export SDKROOT=$(xcrun --show-sdk-path) (this might only be needed on my machine)
  2. export CPPFLAGS="-I/usr/local/opt/snappy/include"
  3. export LDFLAGS="-L/usr/local/opt/snappy/lib" (not sure if both of these flags are required, only one might be enough)
  4. brew remove --force --ignore-dependencies snappy
  5. gem pristine snappy -v '0.0.17'

I believe this patch would have fixed his problem.
Please reconsider.

@Soleone
Copy link

Soleone commented Dec 19, 2019

👍 for reconsidering this patch, thanks!

@bryanparadis
Copy link

Thanks for the attempt to get this fixed up @lavoiesl and the workaround instructions.

@mlhamel
Copy link

mlhamel commented Aug 11, 2020

Any chance this be reconsider?? 🙏

It would be super helpful for many different package managers to have snappy supporting pkg-config.

@pwnall
Copy link
Member

pwnall commented Aug 16, 2020

Sorry, my answer above still stands. We'd only reconsider if there is a Google use case that would justify the ongoing maintenance cost.

I'm going to lock this issue. If googlers want to discuss this, please reach out internally.

@google google locked as resolved and limited conversation to collaborators Aug 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants