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

Include Brotli and Zstd deps - 🎄 #705

Merged
merged 1 commit into from
Jan 3, 2024
Merged

Conversation

KingMob
Copy link
Collaborator

@KingMob KingMob commented Dec 25, 2023

They're now included by default, and all the code to check whether they're present has been removed, and replaced with a static check that will blow up if they're somehow missing.

@KingMob
Copy link
Collaborator Author

KingMob commented Dec 25, 2023

Fixes #703

@KingMob
Copy link
Collaborator Author

KingMob commented Jan 2, 2024

@DerGuteMoritz @arnaudgeiser Any thoughts, or shall we merge it?

Copy link
Collaborator

@arnaudgeiser arnaudgeiser left a comment

Choose a reason for hiding this comment

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

Sorry, I was on holiday, and I just came back.
It voted for integrating brotli4j as dependencies, so all good to me!
Thanks!

@KingMob KingMob merged commit 544561d into master Jan 3, 2024
1 check passed
@KingMob KingMob deleted the add-brotli-and-zstd-deps branch January 3, 2024 08:18
@David-Ongaro
Copy link

Why does it have to be a required dependency? Using a baseimage with the native libs included increases our image size by about 40 MB, and since we use aleph within a service-mesh we probably will never use brotli compression.

@KingMob
Copy link
Collaborator Author

KingMob commented Mar 8, 2024

@David-Ongaro I actually meant to make it optional to begin with. But the Clojure compiler itself had some bugs in supporting Netty's optional codec-checking code.

Here's a little context on it from Slack:

alexyakushev
I understand. Unfortunately, we have limited resources, and supporting modern codecs without requiring the deps started consuming more effort than I'd like, and I'm concerned about future maintenance burden. I originally designed it all to keep the codec deps optional, but that's trickier than it looks, it turns out.

At its root, the issue is the Clojure language uses the same syntax for static member access and static function calls, and the compiler requires reflection to distinguish between the two. Twice this led to tricky ClassNotFoundExceptions compiling codec interop code (that Java itself has no problem with). I ended up writing a whole Java class just to provide a Clojure-safe facade for the problem. Despite that, the problem cropped up again in a new form. While I think I can still make the codecs optional, both choices started to seem equal overall, so I thought it worth asking the users if they had a preference.

Other consequences: for Brotli, it's effectively supported everywhere, and I have no problem making it a default, and if optional, fewer people would enable it. Zstd's future is less clear, but including the Brotli deps and not the Zstd one would just be the worst of both worlds.

Dependency conflicts are always a possibility, but that seems unlikely in this case. People with pre-existing Br/zstd deps are probably trying to modernize Aleph, if I had to guess.

@David-Ongaro
Copy link

@KingMob Thanks for posting this context for the benefit of those who are not members of this Slack channel. That makes the reasoning much more understandable. I couldn't imagine that it's so complicated to support an optional dependency. It seems in Netty itself even HTTP/2 support is optional, but that is pure Java...

Still, the argument of browser support makes me wonder if wider use cases are taken into consideration during Aleph development. This point is irrelevant when using Aleph within a service-mesh. I just hope this doesn't lead to a future impedance mismatch.

@KingMob
Copy link
Collaborator Author

KingMob commented Mar 11, 2024

I couldn't imagine that it's so complicated to support an optional dependency.

We would have had to maintain a complete Java facade for the relevant classes, ensure that the Clojure compiler never reflects on that code or we'd get an NPE, maintain a separate testing regime, etc. It's doable, but keeping it optional would have added to our burden, and Aleph is nobody's full-time job. Heh, if I compared the hours worked last year to the grant I got from Clojurists Together, I probably made below minimum wage working on Aleph.

Still, the argument of browser support makes me wonder if wider use cases are taken into consideration during Aleph development.

Well, we almost never hear from people, so I don't know how others are using Aleph. That makes it hard to account for their needs when making decisions. You said yourself, you don't follow the slack channel. And unlike a lot of project maintainers, I do try to solicit feedback. This very question was put to a vote in the #aleph slack channel.

If artifact size is that crucial to your company, I'd suggest maintaining a fork.

@David-Ongaro
Copy link

Heh, if I compared the hours worked last year to the grant I got from Clojurists Together, I probably made below minimum wage working on Aleph.

I understand, and I'll be the first to suggest Aleph if we get a budget available to support open source Clojure projects.

Well, we almost never hear from people

I guess that's the curse of open source, as long as it works no one says a single word...

If artifact size is that crucial to your company, I'd suggest maintaining a fork.

It's not, it's a nice to have. And we almost recouped that increase by just taking a closer look at our Dockerfile and avoid unnecessary layers... The wider concern is avoiding unnecessary dependencies and complexity. But the SAP Concur Clojure, group is not big (sadly) so we surely won't maintain our own fork, and we probably won't get resources contributing.

@KingMob
Copy link
Collaborator Author

KingMob commented Mar 12, 2024

@David-Ongaro Another possibility occurred to me that should be lower maintenance than a fork. What if you add a build step to strip out the architectures you don't need from the jar file?

@David-Ongaro
Copy link

What if you add a build step to strip out the architectures you don't need from the jar file?

I figured Leiningen actually has an option for that: :uberjar-exclusions. So I tried that out, and it indeed reduces the jar size by about 11 MB, so it reclaims about a fourth of the 40 MB (the rest being due to having to use a different base image in order to support brotli, libstdc++ etc.). But as indicated above, the image size is just a symptom, with the real concern being additional dependencies which may imply future CVEs etc.

In any case, we can live with that. Much more relevant is that Aleph is actively maintained. In that sense, we're happy with the new 0.7 releases.

DerGuteMoritz added a commit that referenced this pull request Apr 6, 2024
…deps"

This reverts commit 544561d, reversing
changes made to b5794d4.
@DerGuteMoritz
Copy link
Collaborator

@David-Ongaro Took antoher stab at it: #723 -- since the workaround is pretty simple, I think we could justify making the dependencies optional again. But let's see how the others feel about it 🙂

@David-Ongaro
Copy link

That's looks promising! Let's hope it works out.

DerGuteMoritz added a commit that referenced this pull request Apr 9, 2024
…deps"

This reverts commit 544561d, reversing
changes made to b5794d4.
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.

4 participants