Brotli4j icon indicating copy to clipboard operation
Brotli4j copied to clipboard

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

Open Kale-Ko opened this issue 1 year ago • 15 comments

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

  • Linux x86
  • Windows x86
  • Windows ArmV7

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

  • For Linux compilation it now uses GCC cross compilation in a Ubuntu 18.04 Docker image to build and QEMU user mode emulation to test. x86_64 and aarch64 are also tested in an Ubuntu 18.04 Docker image to be sure it works.
  • For Windows it uses MSVC cross compilation to build. Testing is done for x86_64 and x86 on a standard runner while aarch64 is done using the custom runner previously setup for this. Unfortunately it is not possible to test armv7 on Windows but it should work. (I also didn't get a chance to test aarch64 since I just couldn't get a VM running)
  • For Mac is uses the same build and test system as before.
  • It now also produces a single "Complete" build of the project that contains all native modules for easy publishing

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/

Kale-Ko avatar Jun 01 '24 21:06 Kale-Ko

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.

slandelle avatar Jun 01 '24 21:06 slandelle

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

Kale-Ko avatar Jun 01 '24 23:06 Kale-Ko

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 avatar Jun 02 '24 09:06 slandelle

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

Feel free to join.

slandelle avatar Jun 02 '24 09:06 slandelle

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 avatar Jun 02 '24 15:06 Kale-Ko

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.

Kale-Ko avatar Jun 05 '24 19:06 Kale-Ko

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:

  • this work can only be merged once Netty 4.2 is around the corner, assuming the CentOS 6 support is indeed dropped (I'll send PRs).
  • merging it would mean dropping support for Netty 4.1. I cast my vote for this, but @hyperxpro is the sole owner of this project.

@hyperxpro your call.

slandelle avatar Jun 05 '24 19:06 slandelle

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)

Kale-Ko avatar Jun 05 '24 19:06 Kale-Ko

would be a different gcc or libc version

Yes, it was the case.

slandelle avatar Jun 05 '24 19:06 slandelle

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?

hyperxpro avatar Jun 09 '24 14:06 hyperxpro

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.

Kale-Ko avatar Jun 10 '24 17:06 Kale-Ko

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.

hyperxpro avatar Jun 10 '24 18:06 hyperxpro

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

Kale-Ko avatar Jun 11 '24 03:06 Kale-Ko

image

Kale-Ko avatar Jun 11 '24 03:06 Kale-Ko

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.

hyperxpro avatar Jun 18 '24 12:06 hyperxpro