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 byte-streams #51

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

add byte-streams #51

wants to merge 2 commits into from

Conversation

piotr-yuxuan
Copy link

@piotr-yuxuan piotr-yuxuan commented Dec 22, 2021

I've been able to compile lacinia in these environments:

OS Host compiler GraalVM version
macOS (version 12.0.1, build 21A559) ? GraalVM CE 21.3.0 (build 17.0.1+12-jvmci-21.3-b05)

Also, I suggest to add a PR template. Feel free to amend or remove it.

In addition to this, what would the maintainers think about a GitHub action to test that everything compiles correctly when a PR is open, and periodically tries to update library versions? Parallel jobs and build matrix could come handy here. If there were any interest, happy to think more about it.

@FieryCod
Copy link
Member

Cool. Now we know byte-streams work with GraalVM, but the usage of initialize-at-build-time without arguments should be replaced with the correct version (meaning with the list of namespaces, classes that should be initialized-at-build-time.

@FieryCod
Copy link
Member

The tricky thing with this library is the fact it uses the single segment package name which makes it extremely hard to native-compile with GraalVM without --initialize-at-build-time with no arguments. The additional patch to the library is required with similar changes @borkdude did for clj-digest clj-commons/digest@a230d0d

@piotr-yuxuan
Copy link
Author

Will suggest a patch against byte-stream upstream.

@FieryCod
Copy link
Member

FieryCod commented Dec 22, 2021

Cool! Thanks for that. Please make sure your changes are minimal, so that we can ensure the backward compatibility for byte-streams :)

@BrunoBonacci
Copy link
Collaborator

Hi @piotr-yuxuan,

thx for the PR.

Please remove the template, versions are changing so fast that I don't think there is much values on capturing these.

The idea of testing the builds with the different GraalVM versions and building a build matrix already came up in some earlier discussions, if you are keen we are open to a PR for that. If you intend to work on it, please open a separate issue/PR for the related discussions so that we keep this about byte-stream.

@piotr-yuxuan piotr-yuxuan marked this pull request as draft December 22, 2021 12:23
@piotr-yuxuan
Copy link
Author

I will exercice code with some more test cases to have more confidence, then I'll mark the PR ready for review.

@FieryCod
Copy link
Member

@piotr-yuxuan I can see the primitive-math is also single namespaced.

@borkdude
Copy link
Member

primitive-math is a library that comes up more often! for dtype.next it was inlined for this reason: https://github.com/cnuernber/dtype-next/blob/master/third-party/com/github/ztellman/primitive_math.clj

@FieryCod
Copy link
Member

Wondering if the library could be donated to clj-commons?

@piotr-yuxuan piotr-yuxuan marked this pull request as ready for review December 22, 2021 20:24
@piotr-yuxuan
Copy link
Author

FYI @ztellman #51 (comment), would you consider donating primitive-math to Clj Commons? or would you accept to unarchive it?

  • Some tentative work moving from single-segment to multi-segment: https://github.com/piotr-yuxuan/primitive-math;
  • While the upstream repo is not updated, non-single-segment version may be available as [com.github.piotr-yuxuan/primitive-math "0.1.7"].

@KingMob
Copy link

KingMob commented Jan 27, 2022

Zach's not very active in clj these days, but he's been generally amenable to putting his code under clj-commons, so I doubt that would be an issue.

My bigger concern is breaking backwards-compatibility in byte-streams and primitive-math. Is there a pointer to the problem with GraalVM? I know single-segment namespaces aren't recommended, but being unable to handle them seems more like a bug in GraalVM, no?

@borkdude
Copy link
Member

@KingMob It's not an issue with GraalVM, but with https://github.com/clj-easy/graal-build-time.
It uses some logic to detect which compiled Java packages are created by compiled Clojure namespaces and then lists those for build-time initialization. But if there is no top level package, then it cannot list that namespace and you almost have no other choice than to initialize everything in the image at build time, something which should be prevented, since there may be Java classes in your image that do not behave correctly when initialized at built time.

The way I solved this for https://github.com/clj-commons/digest/blob/master/src/clj_commons/digest.clj is to copy the single segment namespace [digest] to [clj-commons.digest] and mark the old namespace as deprecated. If you are going with this solution, then do not require the single segment namespace in the library anywhere, since loading that will bring back the package-less classes in the image.

@KingMob
Copy link

KingMob commented Jan 27, 2022

@piotr-yuxuan @borkdude

Hmmmm. I'm not a fan of the duplication, but primitive-math and byte-streams are pretty small. Byte-streams isn't especially popular, but primitive-math has 4 million downloads. OTOH, they're both pretty stable, so I doubt people who fail to update their requires will miss much.

They each have a single top-level file; I should be able to copy those down a level to something like byte-streams.core or somesuch. OK, this shouldn't be too bad.

@borkdude
Copy link
Member

borkdude commented Jan 27, 2022

I would also consider clj-commons.byte-streams as the namespace name for consistency across other libs in this org.

@borkdude
Copy link
Member

Btw, yes, it's either duplication or breaking changes. I think it's better to have duplication + backwards compatibility.

@KingMob
Copy link

KingMob commented Mar 12, 2022

@piotr-yuxuan Did you see my DM on Clojurians Slack?

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.

5 participants