libbrotli

[bagder]

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]

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]

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]

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]

Please go ahead with transforming libbrotli to a pull request that merges libbrotli into brotli.

[fred-wang]

With the new CMake build system it's now easy to create such brotli libraries. FYI I've opened #421.

[fholzer]

@fred-wang but that doesn't mean that @bagder's repo is no longer required, right? Built brotli with -DBUILD_SHARED_LIBS=1 but nginx's ./configure couldn'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]

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

BTW, the above mentioned brotli nginx module looks for libbrotlienc while cmake here creates libbrotli_enc.

[fholzer]

@rockdaboot hi, yes, though i found the issue, which is that the header files won't be copied to $PREFIX/include when running make 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]

and yes, that is in addition to the fact that the naming (w/ underscore) is incorrect

[fred-wang]

hi, yes, though i found the issue, which is that the header files won't be copied to $PREFIX/include when running make install. Is this expected behavior?

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]

@fholzer Can you please try with #437 ?

[fholzer]

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 config file so it finds the library. Not sure if this is the "wanted" solution, but if so, I can create PRs for both.

[fred-wang]

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

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

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]

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.

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]

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

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 #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 google/woff2#63 and google/woff2#61).

[dvershinin]

For the sake of consistency with the google / ngx_brotli, the underscores should not be present...

[fred-wang]

For the sake of consistency with the google / ngx_brotli, the underscores should not be present...

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 #439

[eustas]

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]

@eustas thanks for merging my pull requests.

So now that the library names match google/ngx_brotli, is there anything else needed here?

[bagder]

Shared libraries and proper SONAME versioning?

[eustas]

I believe one last piece is missing: .pc files.
Not sure if we should have them generated, or manually written are ok.

[bagder]

We generate them in the libbrotli repo, although using autotools, but the same approach could be used.

[fred-wang]

@bagder

Share libraries and proper SONAME versioning?

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]

Oh, shared libraries are optional. I didn't expect that, sorry.

[fred-wang]

Oh, shared libraries are optional. I didn't expect that, sorry.

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]

I was thinking about injection this to CMakeLists.txt:

# Build shared libs by default in unbundled mode.
if(BUILD_SHARED_LIBS STREQUAL "")
  if(BROTLI_BUNDLED_MODE STREQUAL "OFF")
    set(BUILD_SHARED_LIBS ON)
  endif()
endif()

[bagder]

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]

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

i.e. https://cmake.org/cmake/help/v3.0/command/option.html

[fholzer]

Yes, AFAIK with cmake you're expected to use this option if you want shared libs.

[bagder]

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]

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.

I don't know if that remark was directed to me but just to clarify I was replying to #326 (comment) 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]

@eustas Regarding pc files, it seems we could use the configure_file command to generate them, see for example mysql/mysql-proxy@6d108a4 or madler/zlib@ca6e7a0.

[nemequ]

@eustas Regarding pc files, it seems we could use the configure_file command to generate them, see for example mysql/mysql-proxy@6d108a4 or madler/zlib@ca6e7a0.

Generating a pkg-config file from CMake using configure_file is 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 use configure_file directly; 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]

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

@nemequ That seems more complicated than the configure_file way and IIUC this would only be beneficial for Windows.

I'm not 100% on the "only" part (IIRC --dont-define-prefix and --prefix-variable are only available on Windows, but I could be wrong), but Windows is definitely the major problem.

Do you have any plan to clean this module up

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.

and submit it to the cmake maintainers?

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 would personally be more comfortable to rely on an official cmake module than to have to copy and maintain one in brotli...

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.

In any case, the configure_file way seems a reasonable and simple option, even if it's only for the short term.

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]

@nemequ

but AFAICT the CMake people are entirely disinterested in anything to do with pkg-config

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.

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

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.

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]

Achieving this by copying a cmake module seems a bit contradictory to that principle

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]

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.

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.

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

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

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.

Thank you!

[nemequ]

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]

Hello. I prefer self-contained build files until they grow insanely large.
Currently CMakeLists is 264 lines, so it sounds OK.

[nemequ]

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]

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]

Great, thank you @nemequ !
Can we close #332 too now?

[nemequ]

Can we close #332 too now?

Maybe. I'll respond there with details.

[fred-wang]

For people interested in having separate pc files, I opened a follow issue #473