mirror of
https://github.com/google/brotli.git
synced 2026-07-08 17:56:58 +00:00
libbrotli #94
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @bagder on GitHub (Mar 8, 2016).
Hey,
I created the sub project libbrotli (https://github.com/bagder/libbrotli) a while ago to help build and install a library for brotli encoding/decoding, using only code from this repository. libbrotli is only a meta-project with mostly autotools to build, install and package a "library" for brotli since this original brotli home does not provide that. It only uses compression/decompression source code from the brotli tree.
This concept seems to resonate with a decent amount of users who appreciate being able to get a library (or two actually) out of a build and install process for use in various projects.
I would prefer if this functionality was provided by the brotli project itself and I'll offer to merge/translate it over to a pull-request or something if you'll agree this is interesting. I'd prefer to remove myself as a middle man here.
@Lukasa commented on GitHub (Mar 8, 2016):
I'd like to add my voice to support this idea. As the author of the Python wrapper library brotlipy, I have had to adopt the changes made by @bagder in order to extract the portions of the code that are relevant to Python library usage. This is problematic, because it forces me to make transformations on the code that make it incompatible with any brotli library installed from elsewhere, ensuring that I have to keep track of changes made in this repository rather than dynamically link against a binary library provided by the OS itself.
@anthonyryan1 commented on GitHub (Mar 16, 2016):
I'd also like to express support for this idea.
The brotli nginx module currently depends on @badger's libbrotli because it can't use this.
If we want to see Brotli adoption pick up on the web, we need to get the web server plugins packaged on various Linux distros. I'm personally very eager to get nginx + Brotli ready on Gentoo, but I'd strongly prefer to have the library and the encoder tools in the same git tree and on the same release cycle.
@fred-wang commented on GitHub (Mar 20, 2016):
I'd also like to express support for a separate library. I've worked on WOFF2 support in Gecko and the WebKit GTK port and it would be more convenient to link to a system library instead of having duplicating code in the repos that we have to build & keep in sync.
@jyrkialakuijala commented on GitHub (Mar 20, 2016):
Please go ahead with transforming libbrotli to a pull request that merges libbrotli into brotli.
@fred-wang commented on GitHub (Aug 27, 2016):
With the new CMake build system it's now easy to create such brotli libraries. FYI I've opened #421.
@fholzer commented on GitHub (Aug 28, 2016):
@fred-wang but that doesn't mean that @bagder's repo is no longer required, right? Built brotli with
-DBUILD_SHARED_LIBS=1but nginx's./configurecouldn't find the lib. Am i doing something wrong?Maintaining a docker image for nginx with brotli and was hoping i could finally build using only official google repos...
@rockdaboot commented on GitHub (Sep 28, 2016):
@fholzer You installed/copied/linked the libs (*.so on Linux) into a well known libs directory (e.g. /usr/local/libs/) and executed ldconf thereafter !?
@rockdaboot commented on GitHub (Sep 28, 2016):
BTW, the above mentioned brotli nginx module looks for libbrotlienc while cmake here creates libbrotli_enc.
@fholzer commented on GitHub (Sep 29, 2016):
@rockdaboot hi, yes, though i found the issue, which is that the header files won't be copied to
$PREFIX/includewhen runningmake install. Is this expected behavior?As a workaround you can either copy the files to a well known include directory, or use
--with-cc-opt="-I /path/to/brotli/source/include"as a configure option for nginx.@fholzer commented on GitHub (Sep 29, 2016):
and yes, that is in addition to the fact that the naming (w/ underscore) is incorrect
@fred-wang commented on GitHub (Oct 7, 2016):
You right I only considered the libraries, but I guess the header should be copied to include/ too. I'll try making a new pull request later to add them to the files to install.
As for the name of libraries (with underscore), I just kept the one that were used in the cmake by the brotli author.
@fred-wang commented on GitHub (Oct 7, 2016):
@fholzer Can you please try with https://github.com/google/brotli/pull/437 ?
@fholzer commented on GitHub (Oct 7, 2016):
Sorry, should've created a PR myself, I guess. I'm using https://github.com/fholzer/brotli/tree/installHeaders and https://github.com/fholzer/ngx_brotli/tree/fixBrotliLinking at the moment, and that works fine. One is for installing the header, the other one adds underscores to the nginx module's
configfile so it finds the library. Not sure if this is the "wanted" solution, but if so, I can create PRs for both.@fred-wang commented on GitHub (Oct 7, 2016):
@fholzer OK, I think installHeaders is essentially the same as mine (except that I also install headers on Windows, not sure what should be done on that platform). For fixBrotliLinking, I don't know what name is best but I guess it is up to the maintainer of the brotli project to decide...
@fholzer commented on GitHub (Oct 7, 2016):
Right, that's the main reason i didn't submit it yet.
Regarding the headers on windows I wasn't sure myself, and I needed it for Linux only. Will post an update once I got the chance to test this on windows.
@fred-wang commented on GitHub (Oct 8, 2016):
So my recollection is that the only thing needed is to have the libraries in the same directory as the executable, otherwise they are not found. I don't think there is a standard locations for libraries or headers on Windows, so I guess this does not really matter. There are some UNIX-like systems on Windows (e.g. MSYS2) that may have different requirements, but maintainers can write patches if they want to. Anyway, it seems nice to have the public headers installed too.
@fred-wang commented on GitHub (Oct 12, 2016):
@eustas So I guess you replied on the pull request about the WIN32 case. Can you please indicate whether you want to preserve the underscore in the library names? Thanks!
@fred-wang commented on GitHub (Oct 12, 2016):
The pull request to install the headers has been merged. Are the library names with underscore fine for everybody? Is there anything else needed? Or can we now close this issue as well as the https://github.com/google/brotli/pull/332 pull request?
I think it would also be nice to have a brotli release with the cmake library changes, so that we can start using the release as a reference for package maintainers & users (see for example the duplicate woff2 pull requests in https://github.com/google/woff2/pull/63 and https://github.com/google/woff2/pull/61).
@dvershinin commented on GitHub (Oct 12, 2016):
For the sake of consistency with the google / ngx_brotli, the underscores should not be present...
@fred-wang commented on GitHub (Oct 12, 2016):
ngx_brotli relied on the unofficial librotli so I guess it should be updated to match whatever convention the maintainers of brotli want. Anyway, I opened https://github.com/google/brotli/pull/439
@eustas commented on GitHub (Oct 17, 2016):
Hello. Sorry for the late response. I have no strong opinion about using underscores in library names.
Since it is just a trade of bit of readability (but whoever reads so filename?) for a bit of compatibility, lets to further without underscores.
@fred-wang commented on GitHub (Oct 17, 2016):
@eustas thanks for merging my pull requests.
So now that the library names match google/ngx_brotli, is there anything else needed here?
@bagder commented on GitHub (Oct 17, 2016):
Shared libraries and proper SONAME versioning?
@eustas commented on GitHub (Oct 17, 2016):
I believe one last piece is missing: .pc files.
Not sure if we should have them generated, or manually written are ok.
@bagder commented on GitHub (Oct 17, 2016):
We generate them in the libbrotli repo, although using autotools, but the same approach could be used.
@fred-wang commented on GitHub (Oct 17, 2016):
@bagder
Yes, shared libraries can now be generated. I don't know if that's what you want but the version used in the one in common/version.h and if you follow the cmake instruction in the README, you get the following libraries and soft links:
libbrotlicommon.so ->libbrotlicommon.so.1.0.0
libbrotlidec.so -> libbrotlidec.so.1.0.0
libbrotlienc.so -> libbrotlienc.so.1.0.0
@bagder commented on GitHub (Oct 17, 2016):
Oh, shared libraries are optional. I didn't expect that, sorry.
@fred-wang commented on GitHub (Oct 17, 2016):
Yes, I tried to follow CMake's usual approach i.e. to rely on the BUILD_SHARED_LIBS flag (off by default). I guess it is fine to have these shared libraries optional.
@eustas commented on GitHub (Oct 17, 2016):
I was thinking about injection this to CMakeLists.txt:
@bagder commented on GitHub (Oct 17, 2016):
We live in a shared library world these days. Most library build systems these days build shared by default and that's what most users use (compare even with other compression libraries). I think the brotli libs should build shared by default too.
@fred-wang commented on GitHub (Oct 17, 2016):
@bagder OK, that makes sense to me too.
@eustas I don't know what is best but https://cmake.org/cmake/help/v3.0/variable/BUILD_SHARED_LIBS.html mentions using the OPTION cmake command.
@fred-wang commented on GitHub (Oct 17, 2016):
i.e. https://cmake.org/cmake/help/v3.0/command/option.html
@fholzer commented on GitHub (Oct 17, 2016):
Yes, AFAIK with cmake you're expected to use this option if you want shared libs.
@bagder commented on GitHub (Oct 17, 2016):
Those docs says we can/should use an option. There's nothing there saying it should default to non-shared or that static is preferred. You could easily just switch the default to say shared by default unless you turn it off.
@fred-wang commented on GitHub (Oct 17, 2016):
I don't know if that remark was directed to me but just to clarify I was replying to https://github.com/google/brotli/issues/326#issuecomment-254208935 and just suggesting to use the OPTION command instead (but of course making it on by default as otherwise that does not change the current behavior). I'll try to submit a pull request to do that.
@fred-wang commented on GitHub (Oct 17, 2016):
@eustas Regarding pc files, it seems we could use the configure_file command to generate them, see for example
6d108a4f8corca6e7a0d55.@nemequ commented on GitHub (Oct 17, 2016):
Generating a pkg-config file from CMake using
configure_fileis problematic in a non-obvious way. I ended up writing a CMake module for Squash to do it, and explained why I didn't just useconfigure_filedirectly; see GenPkgConfig.cmake:35 It's a bit messy, but you can steal the module if you want.As for versioning, now that shared libraries are possible the version information should be added to the target. There is an example in Squash. Note that soversion isn't quite the same as version, though if brotli adopts compatible rules for versioning they could be. There is a good description of the issue in the autobook; §11.4: Library Versioning
@fred-wang commented on GitHub (Oct 19, 2016):
@nemequ That seems more complicated than the configure_file way and IIUC this would only be beneficial for Windows. Do you have any plan to clean this module up and submit it to the cmake maintainers? I would personally be more comfortable to rely on an official cmake module than to have to copy and maintain one in brotli... In any case, the configure_file way seems a reasonable and simple option, even if it's only for the short term.
@nemequ commented on GitHub (Oct 19, 2016):
I'm not 100% on the "only" part (IIRC
--dont-define-prefixand--prefix-variableare only available on Windows, but I could be wrong), but Windows is definitely the major problem.Maybe. IIRC I didn't put a great deal of thought into the API, I just wanted a quick way to resolve the problem. At some point I (or someone else) should probably review it, but I don't remember any glaring problems.
No.
They can grab it if they want (I'd even be willing to relicense it if necessary), and if someone else wants to submit it that's fine, but AFAICT the CMake people are entirely disinterested in anything to do with pkg-config, and I don't feel like banging my head against that particular wall.
I don't really understand that, but okay. I have fewer reservations about relying on in-tree code than a module distributed by someone else which is likely to change depending on what version happens to be installed. Furthermore, a local copy means there is no need to bump the cmake dependency to get support for it, and any bugs can be fixed immediately instead of bumping the dependency again. IIRC when I put together Brotli's CMake stuff it worked all the way back to 2.8.6 (which is the version shipped with Solaris 11). A lot of CI providers (including Travis and Drone.io) use Ubuntu 12.04 which only has CMake 2.8.7, so supporting older versions of CMake is important.
Using the module is a simple option, too; writing it wasn't, but that's already done. It may not be pretty but AFAIK it works perfectly and IMHO the benefit of shipping a correct pkg-config file vastly outweighs any concerns about copying the module, but since Brotli isn't my project that's not my decision to make. Note that it would also be possible to copy some of the contents of the file into Brotli's CMakeLists.txt to avoid polluting the source tree.
@fred-wang commented on GitHub (Oct 20, 2016):
@nemequ
Well, my goal for the present GitHub issue is to have packages for Brotli/woff2 shared libraries that can be used by several programs, instead of having multiple copies of Brotli/woff2 bundled in their source code (in my case WebKit, Gecko and maybe fontforge). Achieving this by copying a cmake module seems a bit contradictory to that principle: IMHO the cmake module should really be upstreamed so that it can be used by all the projects that want pkg-config support without having to copy it everywhere. Now, if cmake devs don't care that's a bit problematic...
Note that Brotli is not my project either so I don't decide either. As I said my main goal is to have packages for shared libraries in order to get rid of multiple copies in projects I work on. I don't really feel like submitting a cmake module I did not wrote, that I can not test (the windows part I mean) and whose author indicates it needs some clean up and review. However, if you want to open a PR and if the windows stuff is very important for your then please do so :-) Otherwise, I think I'll just try the configure_file way when I have time...
@nemequ commented on GitHub (Oct 20, 2016):
It's not really related; you don't have to copy the module into projects which use brotli, it's just something for brotli's build system so it can output a correct pkg-config file. Besides, even if it was included in CMake's standard modules tomorrow, it would be many years before brotli could really take advantage of it; cmake 2.8.6 is just over 5 years old, and we're still stuck with it if we want to support Solaris. cmake 2.8.7 is slightly under 5 years old (by about 2 months), and we're still stuck with it for Ubuntu 12.04 and, by extension, Travis, Drone.io, etc.
Anyways, I'll try to throw together a PR for all this stuff by next week. I need to check a few of the changes which have been made to the CMake support anyways, since I think they'll break Squash when I pull them in.
@fred-wang commented on GitHub (Oct 20, 2016):
Indeed, it's worse than just "projects which use brotli": What you are suggesting is that any cmake project that generates pkg-config files (BTW, I plan to do the same work for woff2...) should not use a simple configure_file cmake command but copy your custom cmake module or at least the logic it contains. I'm just stating the obvious rule that if some pattern appears in several projects it should be shared as much as possible rather than duplicated in each place. IMHO, if generating pkg-config files is something important for many cmake projects, then a cmake command or module should be provided by the cmake developers.
True, but I'm not sure it's a strong reason against trying it. If I continue the parallel with libbrotli, it will take time before the next version of brotli is released and before shared libraries are packaged in the various distros so that they can really be used by the projects that need brotli. But that does not prevent me from trying ;-)
Thank you!
@nemequ commented on GitHub (Oct 20, 2016):
Ah, okay I misunderstood your point. Looking at the issue globally instead of just what Brotli needs right now you're right, it would be good to get support added to CMake eventually. I'll try to take a look at the module in the near future, clean it up if need be, and offer it up to the CMake people, but I'm not very confident about them accepting it.
@eustas, do you have an opinion on having the module in a separate file vs. merging it into Brotli's CMakeLists.txt? I don't really care.
@eustas commented on GitHub (Oct 24, 2016):
Hello. I prefer self-contained build files until they grow insanely large.
Currently CMakeLists is 264 lines, so it sounds OK.
@nemequ commented on GitHub (Oct 24, 2016):
I put together some patches for this over the weekend (https://github.com/nemequ/brotli). I want to do a bit more testing before I submit a PR, but in the meantime: comments welcome.
@nemequ commented on GitHub (Oct 31, 2016):
Okay, I rebased my patches and did some more testing, AFAICT everything works as expected. PR filed as #464.
I believe that, once merged, this issue should be resolved. Am I missing anything?
@fred-wang commented on GitHub (Nov 1, 2016):
Great, thank you @nemequ !
Can we close #332 too now?
@nemequ commented on GitHub (Nov 1, 2016):
Maybe. I'll respond there with details.
@fred-wang commented on GitHub (Dec 3, 2016):
For people interested in having separate pc files, I opened a follow issue #473