hyperxpro / Brotli4j

Brotli4j provides Brotli compression and decompression for Java.
Apache License 2.0
102 stars 35 forks source link

Add new architecture support (Linux x86, Windows x86, Windows ArmV7) and improve build system #141

Open Kale-Ko opened 3 months ago

Kale-Ko commented 3 months ago

Motivation:

I'm working on my own project that uses Netty's HTTP features and Brotli compression and thought it would be nice if I could support these architectures as well. I was originally just going to add them with a fairly simple PR but I noticed the build system could be greatly improved and got a bit carried away with it.

Modification:

I have added the following architectures to the project

Along with this I have basically rewrote the entirety of .github/workflows/maven.yml.

In the process I also moved the logic of checking if a module should be used to the module itself instead of the centralized Brotli4jLoader.java.

Result:

The mentioned architectures are now supported and the project is now slightly easier to work with.

I've put the artifacts from the latest build up on my maven if you would like to test them https://maven.kaleko.dev/public-snapshot/

slandelle commented 3 months ago

I don't remember where netty stands nowadays wrt Linux compat. IIRC, the current Linux build is complicated and not modern because of the CentOS 7 CentOS 6 compat requirement.

Kale-Ko commented 3 months ago

I've used it in a handful of projects and it's always worked wonderfully.

slandelle commented 3 months ago

I've used it in a handful of projects and it's always worked wonderfully.

Irrelevant. The main focus of this library is to be compatible with Netty. And currently, Netty still requires for its binary dependencies to be compatible with CentOS 6 (I corrected my previous comment).

We at Gatling had to go with overly complicated length to make the build here compatible instead of using the build from upstream (the actual brotli project). I'm pretty sure you're breaking this compatibility, eg when upgrading cmake.

Then, I'm completely sold that netty should drop this requirement. But that's the netty core team that has to be convinced. I'll try again as Netty 4.2 is underway.

slandelle commented 3 months ago

I've started a discussion here: https://github.com/netty/netty/discussions/14091

Feel free to join.

Kale-Ko commented 3 months ago

I've used it in a handful of projects and it's always worked wonderfully.

Irrelevant.

Sorry I did not understand your first comment

I'll set up a vm and see what I can do in terms of compatibility later today. I think it should be easy enough to build using the CentOS 6 docker image.

The Cmake upgrade was only because Cmake was complaining about deprecation and removal, even still it shouldn't be an issue if CentOS supports anything remotely recent.

Kale-Ko commented 3 months ago

Sorry this took so long, I got really busy the past few days.

All Linux images are now built in Ubuntu 18.04. As for testing have a look at the updated README

I've tested the latest build on CentOS 7 and all tests are passing. I would have tried 6 but I can't find an iso or working docker image anywhere.

slandelle commented 3 months ago

I would have tried 6 but I can't find an iso anywhere and I can't get it running in Docker either.

That's the whole thing. I'm 100% sure that what you've done doesn't work on CentOS 6, starting with the cmake ipgrade. As I explained, it was a huge headache to make it work for this reason. If not for this requirement, we would have done what you're doing here.

The good news is that the Netty core team seems to be willing to raise the compatibility baseline to CentOS 8 for the upcoming Netty 4.2 (I have no ETA on this, I'm just an external minor contributor).

This means that:

@hyperxpro your call.

Kale-Ko commented 3 months ago

I only upgrade the minimum Cmake version to 3.5.0, the previous build system was using 3.20. Either way the only thing that would cause incompatibility would be a different gcc or libc version (which is still entirely possible)

slandelle commented 3 months ago

would be a different gcc or libc version

Yes, it was the case.

hyperxpro commented 3 months ago

I'd stick to the old gcc version because there is no EOL date for Netty 4.1 and also I don't want to surprise people with broken gcc version when pushing update.

Also, the actions file looks very congested. Can you please offload the Dockerfile content into files?

Kale-Ko commented 3 months ago

I'm wondering if it's really worth supporting CentOS 6, how many systems could possibly be running it.

If you do want to support it though what would you think about static linking? It would in theory run on any version of Linux.

hyperxpro commented 3 months ago

It's not really about 'worth supporting' but about maintaining compatibility for systems that are already using it.

I know it's painful to deal this but breaking systems is the last thing I'd do in any situation.

Kale-Ko commented 3 months ago

Alright it should be compatible now (building on the old docker image), I'll test it once the build finishes.

Kale-Ko commented 3 months ago

image

hyperxpro commented 2 months ago

Also, we can drop Windows 32-bit. Microsoft has killed it and other products are also decommissioning their 32-bit variants.

Rest, I will work on this to keep the same compatibility.