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

Update Gradle Compatibility #31

Merged
merged 15 commits into from
Oct 22, 2023
Merged

Update Gradle Compatibility #31

merged 15 commits into from
Oct 22, 2023

Conversation

milkyway0308
Copy link
Contributor

Gradle Version Support Updates

1. Added Support for Gradle 8

  • Problem: Gradle 8 removed some methods that were deprecated in previous versions, leading to incompatibility issues.
  • Solution: Necessary code changes have been made to ensure compatibility with Gradle 8.

2. Introduced Backward Compatibility with Gradle 6

  • Problem: Gradle 6 introduced some changes in the constructors of specific classes, causing incompatibility with Gradle 6 and newer versions.
  • Solution: Given the critical nature of this issue, support has been implemented using reflection to maintain compatibility with these versions.

3. Dropped Support for Gradle 5

  • Reason: Gradle 5 is an older version with fundamental methods not being compatible with the latest Gradle versions.
  • Solution: Instead of maintaining backward compatibility through reflection for Gradle 5, changes have been made to utilize methods introduced in Gradle 6 and newer. As a result, once this commit is merged, the project will no longer support Gradle 5.

Please review the changes and provide feedback.

This pull request is affected from Issue #30.

@saudet
Copy link
Member

saudet commented Aug 17, 2023

Instead of breaking compatibility with Gradle 5.x, let's keep the deprecated calls until they become unavailable in Gradle 9.x? I guess we could wait to merge this until Gradle 9.x is released...

@milkyway0308
Copy link
Contributor Author

Looks like they fully removed deprecated method calls in gradle 8, so we have to remove it or replace it.

We have to change it to reflection, or drop support to gradle 5.

gradle-javacpp is not working in Gradle 8.0.2.

@saudet
Copy link
Member

saudet commented Aug 17, 2023

@estigma88 says it's working fine with Gradle 8.1, see issue #28 (comment). What's not working for you exactly?

@milkyway0308
Copy link
Contributor Author

milkyway0308 commented Aug 17, 2023

I'm checking for Gradle users, and found some user from Gradle 8 not works.

As you can see, author of #30 cannot run gradle-javacpp in Gradle 8.0.

In my environment, Gradle 8.0 / Gradle 8.0.2 / Gradle 8.1 cannot run in my environment.

Here's some screenshot from my IDE.

image
image

P.S. Found that works in Mac user. Maybe Window version Gradle issue?

@milkyway0308
Copy link
Contributor Author

First is my mistake. Found gradle-javacpp-platform works, but gradle-javacpp-build not.

I checked build test in multiple environment.

Here's some test result from main branch:

  • Gradle 8.x wrapper not allows compile due to missing method / fields. (getOutputDir(), setBaseName(String)...)
  • Gradle 7.6.2 wrapper can compile main branch.
  • Some gradle wrapper not supports direct java access to property, but groovy access.

Sorry for taking your time.

@milkyway0308
Copy link
Contributor Author

P.S. I wrote code that changes proprty access to reflection to bridge some code changes.

If you want, I'll commit modified code to this branch.

@saudet
Copy link
Member

saudet commented Aug 17, 2023

Sure, let's see what that looks like.

- reverted indent affected from IntelliJ auto sort
@milkyway0308
Copy link
Contributor Author

milkyway0308 commented Aug 17, 2023

Here is it!

Sorry for late, I was fixing auto sorted indents affected from IntelliJ auto sort.

All indent reverted as original.

Code is not clean as well, but at least, it works.

@milkyway0308
Copy link
Contributor Author

Sorry again, it not works on 7.6.2.

Fixing code again.

@milkyway0308
Copy link
Contributor Author

milkyway0308 commented Aug 17, 2023

Test completed on next environment:

  • Gradle 8.2.1
  • Gradle 8.0.2
  • Gradle 8.0
  • Gradle 7.6.1
  • Gradle 6.3

@irnbrux
Copy link

irnbrux commented Aug 17, 2023

Works for me in
Gradle 8.3
Gradle 7.4.2

@saudet
Copy link
Member

saudet commented Aug 17, 2023

Sorry for late, I was fixing auto sorted indents affected from IntelliJ auto sort.

All indent reverted as original.

There's still a lot of unnecessary formatting changes. Please revert all that, it makes it hard to review.

@milkyway0308
Copy link
Contributor Author

I apologize for unintentionally altering the code format.

My IntelliJ kept trying to auto-format, so I made adjustments using Notepad.

@saudet
Copy link
Member

saudet commented Aug 18, 2023

Thank you, but please continue reverting necessary changes

@milkyway0308
Copy link
Contributor Author

milkyway0308 commented Aug 18, 2023

Didn't realize that IntelliJ had also broken code format of other file.

I changed them back to the original format.

@saudet saudet requested a review from HGuillemet August 18, 2023 23:02
Copy link
Collaborator

@HGuillemet HGuillemet left a comment

Choose a reason for hiding this comment

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

Sorry for the delay of this review.

I don't use the build plugin for my own uses, so I tried to make the sample zlib work with Gradle 8.2.1.
Here are my comments:

  1. build.sh must be updated with the correct sha256. We can update to zlib 1.3 too.
  2. Exception are now thrown when Gradle detects a task that uses an output of another task without declaring an implicit or explicit dependency. This is the case of javacppBuildCommand having workingDirectory as input, while this directory is populated by, eg, javacppPomProperties. So gradle would like javacppBuildCommand to depend on javacppPomProperties, which doesn't make sense. To solve this, @Optional @InputDirectory annotations on workingDirectory can be replaced by @Internal. There might be cleaner way to solve this.
  3. Same problem for javaDoc using a directory populated by javacppPomProperties. For this one, I guess we can add an explicit dependency;
project.getTasks().getByName("javadoc").dependsOn("javacppPomProperties");
  1. Similar problem with sourcesJar. The normal way to make sourcesJar always run after javacppParser is to add the output directory of javacppParser (or, better, the task itself) to the source set. But javacppParser depends on javacppCompile, which depends on the source set. So there would be a circular dependency. The way this currently can work is to not use the normal:
java {
    withSourcesJar()
}

but:

task sourcesJar(type: Jar, dependsOn: classes) {
    archiveClassifier = 'sources'
    from sourceSets.main.allSource
}

With archiveClassifier replaced by classifier for Gradle < 6.
I believe a better way to work around this would be to create another source set, generated, containing the directory of generated sources, and use it instead of main for compileJava, but not javacppCompileJava. This would allow to use the standard withSourcesJar and to not mess with classifier/archiveClassifier. But this is not a priority, since we have a workaround.

@saudet
Copy link
Member

saudet commented Sep 27, 2023

@milkyway0308 Please make the corrections pointed out by @HGuillemet

@HGuillemet Let's move the unrelated discussion to an issue? If you'd like to work on that, along with other things to support modules and what not, feel free to do it. I've given you admin access to this repository already before!

@milkyway0308
Copy link
Contributor Author

Apologies for the delay, I was tied up with other tasks.

I've made some fixes based on your review.

If there are any more errors, please let me know.

@saudet saudet requested a review from HGuillemet October 4, 2023 04:08
Copy link
Collaborator

@HGuillemet HGuillemet left a comment

Choose a reason for hiding this comment

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

Ok for me.
We can fix the sample and other problems I mentioned later on.

@saudet saudet merged commit 4ddf762 into bytedeco:master Oct 22, 2023
1 check passed
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