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

Upgrade Gradle Tooling API to 8.10 #7690

Merged
merged 1 commit into from
Sep 6, 2024
Merged

Conversation

mbien
Copy link
Member

@mbien mbien commented Aug 22, 2024

@mbien mbien added Gradle [ci] enable "build tools" tests Upgrade Library Library (Dependency) Upgrade ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Aug 22, 2024
@mbien mbien requested a review from lkishalmi August 22, 2024 06:31
@sid-srini
Copy link
Contributor

Hi @mbien. I was wondering if this PR is ready for review or do you expect more changes? Would this be planned for NB23 delivery? Thanks.

@mbien
Copy link
Member Author

mbien commented Sep 5, 2024

Hi @mbien. I was wondering if this PR is ready for review or do you expect more changes? Would this be planned for NB23 delivery? Thanks.

@sid-srini this PR wasn't picked up for NB 23 so it is likely going to end up in NB 24 or closed.

I posted two mails to the dev list when I opened the maven/gradle PRs:
https://lists.apache.org/thread/4pswdxf89szssxjsck95jgj0dj3ydhf4 (maven, #7683)
https://lists.apache.org/thread/w53t2240ny5lzpfypm6bc5bm6gw833zm (gradle, this PR)

the way I understand this is that the gradle tooling upgrade is not technically needed since the gradle project init script is already using "newest", which should init with the latest available wrapper. However I never could get this to work (it always picked 8.9), others reported that it does work though.

@sid-srini
Copy link
Contributor

sid-srini commented Sep 5, 2024

Thanks @mbien for the delivery references.

the way I understand this is that the gradle tooling upgrade is not technically needed since the gradle project init script is already using "newest", which should init with the latest available wrapper. However I never could get this to work (it always picked 8.9), others reported that it does work though.

  • Your PR does work for me.
    • When I created a new project with this in place, the gradle wrapper set up contains the distributionUrl for gradle-8.10-bin in the gradle-wrapper.properties; instead of the previous version.
    • An older existing project, on the other hand, continued to use the previous version of gradle that was configured in it.
    • In terms of Java toolchain support for version 23, gradle 8.10 was able to understand the version number, and auto-download some version of JDK 23 (in concert with the foojay plugin).
    • On the other hand, gradle 8.9 was unable to do so and gave an error.
  • I expect that it would work for others too in the same way.
  • So, I urge you to submit this PR for review and not close it. 😃

Thank you.

@mbien
Copy link
Member Author

mbien commented Sep 5, 2024

When I created a new project with this in place, the gradle wrapper set up contains the distributionUrl for gradle-8.10-bin in the gradle-wrapper.properties; instead of the previous version.

yep. but in theory this should already happen with NB 22. (it didn't work for me either, but others claimed that it does work)

it tells gradle to use the latest available wrapper

edit: if you test this, make sure you clean your ~/.gradle folder which caches a lot of things

 - full JDK 23 support
@mbien mbien changed the base branch from master to delivery September 5, 2024 23:00
@mbien mbien marked this pull request as ready for review September 5, 2024 23:06
@mbien mbien requested a review from sdedic September 5, 2024 23:12
@mbien
Copy link
Member Author

mbien commented Sep 5, 2024

due to popular demand I rebased this on top of delivery in case there is spare capacity to review this for NB 23 - if not I can move it back to master/NB24 - no problem.

PR produces a dev build for convenient testing.

@mbien
Copy link
Member Author

mbien commented Sep 5, 2024

@sid-srini has any testing been done with the patch outside quick checks if new-project init works on JDK 23?

@mbien mbien added this to the NB23 milestone Sep 6, 2024
@sid-srini
Copy link
Contributor

sid-srini commented Sep 6, 2024

@sid-srini has any testing been done with the patch outside quick checks if new-project init works on JDK 23?

Yes, I have checked the following with NB running on both JDK 22 and 23:
- New project init
- Project loading
- Clean, Compile and Run the app
- Basic code editing, quick fixes and refactoring.
- Using some locally available external libraries in the project.

When I created a new project with this in place, the gradle wrapper set up contains the distributionUrl for gradle-8.10-bin in the gradle-wrapper.properties; instead of the previous version.

yep. but in theory this should already happen with NB 22. (it didn't work for me either, but others claimed that it does work)

This did not work for me either. I got the following results for the gradle versioned link that is stored in the wrapper on init:

  1. With NB 22: gradle-8.7-bin
  2. With the rc1 PR Upgrade Gradle Tooling API to 8.9 #7583: gradle-8.9-bin
  3. With this PR: gradle-8.10-bin

sid-srini added a commit to oracle/javavscode that referenced this pull request Sep 6, 2024
Backport NB patch apache/netbeans#7690 for Gradle tooling 8.10 upgrade
@ebarboni
Copy link
Contributor

ebarboni commented Sep 6, 2024

LGTM

@mbien
Copy link
Member Author

mbien commented Sep 6, 2024

@sid-srini thanks a lot for the info! It helps to know what has been tested in such last-minute PRs.

@ebarboni ebarboni merged commit adf4a0b into apache:delivery Sep 6, 2024
33 checks passed
@MartinBalin
Copy link
Contributor

sdedic is on vacation this afternoon. Seeing he did not objected and was on PR for a while I think it is OK to merge to NB23. So merge it

@neilcsmith-net
Copy link
Member

One (small?) problem with this update is that the wizard now defaults to JDK 23, which then takes a while downloading a JDK via Disco without much feedback. Probably not a showstopper for NB24, and mainly the behaviour that was already there, but the wizard should probably default to the running JDK if possible in NB24?

@mbien
Copy link
Member Author

mbien commented Sep 11, 2024

I think what it could do is to indicate if it will dl a JDK or not. User can decide what to do. The default of Java 23 (essentially latest known) is probably fine for NB 23 since it overlaps with JDK 23 GA but I wouldn't mind setting it to the NB runtime JDK. (also note that the draft of the NB download page does also mention JDK 23 as supported runtime JDK apache/netbeans-antora-site#29)

I vaguely remember pointing out that gradle simply downloading stuff without asking is probably not ideal on the PR which implemented this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) Gradle [ci] enable "build tools" tests Upgrade Library Library (Dependency) Upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants