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

Pushed Jena version to 5.0.0 and updated Caching library #177

Merged
merged 6 commits into from
Dec 17, 2024

Conversation

jerdebsp
Copy link

@jerdebsp jerdebsp commented May 26, 2024

Hi,

I was having trouble using 1.4.3 due to Jena classes deprecated or removed - more specifically org.apache.jena.graph.Factory. In my pull request, i've updated Jena to the latest version and also updated the Caching library to use benmanes (https://github.com/ben-manes/caffeine) which is compatible with the previously used library. I believe that the library used in the main fork is not available anymore.

Thanks,
Jeremy

PS. I couldn't assign anyone as reviewer. I guess Holger or Ashley can do this.

@@ -72,7 +72,7 @@ public MultiUnion createMultiUnion(Graph[] graphs) {
* @return the default Graph
*/
public Graph createDefaultGraph() {
return Factory.createDefaultGraph();
return GraphFactory.createDefaultGraph();
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: Should this be a call to `JenaUtil.createDefaultGraph()?

Copy link
Author

Choose a reason for hiding this comment

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

For some reason JenaUtil.createDefaultGraph() is calling the helper class, causing a stack overflow cycle

This is what there is in JenaUtil.
public static Graph createDefaultGraph() { return helper.createDefaultGraph(); }
Maybe this needs to be changed?

src/main/java/org/topbraid/jenax/util/JenaUtil.java Outdated Show resolved Hide resolved
src/main/java/org/topbraid/shacl/arq/SHACLARQFunction.java Outdated Show resolved Hide resolved
@costas80
Copy link

costas80 commented Jul 5, 2024

Hey @jerdebsp . Is this PR blocked? I see discussion about a stack overflow from the review by @afs but no clear resolution.

@jerdebsp
Copy link
Author

jerdebsp commented Jul 5, 2024

That comment was for why I did not use JenaUtil.createDefaultGraph(). I think the PR is still blocked for now until Java (on the CI) is updated from 17 to 21 or 22.

If you are using java 21 or 22, you can download my branch and use it. I am using it for project work and training and so far it seems to work well

@afs
Copy link
Contributor

afs commented Oct 26, 2024

The latest Jena release is 5.2.0.

@costas80
Copy link

@jerdebsp, if you update Jena to the latest version, maybe @ashleycaselli, or @HolgerKnublauch could have a look at the CI issue and merge this?

@jerdebsp
Copy link
Author

I will try to update to Jena 5.2.0 by end of next week

@ashleycaselli ashleycaselli added the dependencies Pull requests that update a dependency file label Dec 9, 2024
@jerdebsp
Copy link
Author

jerdebsp commented Dec 9, 2024

I tested 5.2.0 on my machine and seems to work, however, it only makes sense to update once this PR is merged.

@ashleycaselli ashleycaselli marked this pull request as draft December 11, 2024 14:36
@ashleycaselli
Copy link
Collaborator

@jerdebsp can you please update the JDK used in the maven-test-pr.yml to Java 17 (Jena5 requires that)? Also, before merging the PR we would need to update the JRE versions used as base images for building the docker image (docker-image.yml).

@jerdebsp jerdebsp marked this pull request as ready for review December 12, 2024 11:08
@jerdebsp
Copy link
Author

@ashleycaselli done. when this is merged i'll move with updating to 5.2.0

@ashleycaselli
Copy link
Collaborator

@ashleycaselli done. when this is merged i'll move with updating to 5.2.0

Thanks!

Lastly, the maven-compiler-plugin is building the code against Java 11. That means that even if Maven/Docker image now run on JDK 17, when the package is released it's compiled following Java 11 specifications. To avoid any compatibility issues at this stage I would keep that Java version in sync with the JDK version used. However, before making any changes I'd wait for @HolgerKnublauch 's point on this.

@HolgerKnublauch
Copy link
Collaborator

Don't wait for my feedback here as I don't really have time to follow the details of this repo anymore. We have completely moved in our code base to Java 17 last year and didn't encounter problems.

@afs
Copy link
Contributor

afs commented Dec 13, 2024

For docker, the image can jump to Java21 runtime, even if the code only requires Java17.

Jena already builds with Java21 (targeting Java17) and runs on Java21.

Java25 is LTS in September 2025. Sometime after, when it's clear Java25 is established, Jena will highly likely move to supporting Java21 and Java25, dropping Java17.

Apache Lucene 10 requires Java21.

@ashleycaselli ashleycaselli changed the base branch from master to 177-jena5 December 17, 2024 14:51
@ashleycaselli ashleycaselli merged commit 5d64c56 into TopQuadrant:177-jena5 Dec 17, 2024
1 check passed
@ashleycaselli
Copy link
Collaborator

I merged this PR into the 177-jena5 branch.

@jerdebsp do you want to bump Jena to v5.2 into that branch and then I merge into master when it's ready to be released?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants