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

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

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

Kale-Ko
Copy link

@Kale-Ko Kale-Ko commented Jun 1, 2024

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/

@slandelle
Copy link
Contributor

slandelle commented Jun 1, 2024

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
Copy link
Author

Kale-Ko commented Jun 1, 2024

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

@slandelle
Copy link
Contributor

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
Copy link
Contributor

I've started a discussion here: netty/netty#14091

Feel free to join.

@Kale-Ko
Copy link
Author

Kale-Ko commented Jun 2, 2024

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
Copy link
Author

Kale-Ko commented Jun 5, 2024

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
Copy link
Contributor

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.

@Kale-Ko
Copy link
Author

Kale-Ko commented Jun 5, 2024

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
Copy link
Contributor

would be a different gcc or libc version

Yes, it was the case.

@hyperxpro
Copy link
Owner

hyperxpro commented Jun 9, 2024

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
Copy link
Author

Kale-Ko commented Jun 10, 2024

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
Copy link
Owner

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
Copy link
Author

Kale-Ko commented Jun 11, 2024

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

@Kale-Ko
Copy link
Author

Kale-Ko commented Jun 11, 2024

image

Comment on lines +80 to +86
docker build \
--platform linux/amd64 \
--build-arg "DOCKER_IMAGE=amd64/ubuntu:${{ matrix.architecture.ubuntu_version }}" \
--build-arg "TARGET_GCC_PACKAGE=${{ matrix.architecture.gcc_package_name }}" \
--build-arg "TARGET_GCC_EXEC=${{ matrix.architecture.gcc_exec_name }}" \
--tag linux-buildimage:${{ matrix.architecture.name }} \
docker/build/
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actions file is now giant. Can we dump these into a file?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not meant to be small, it's meant be easy to follow.

Putting the docker commands into extra files will just lead to a bunch of scripts with random parameters that are hard to follow.

@hyperxpro
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants