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

#4923: access to gradle internal APIs is protected from failing the project load, just logs a notification. #4936

Merged
merged 1 commit into from
Nov 8, 2022

Conversation

sdedic
Copy link
Member

@sdedic sdedic commented Nov 7, 2022

The culprit for the reported error (see #4923) is a LinkageError as the gradle version configured for the build changed its internal APIs (a change needed to adopt Groovy 4.0). It's true that now the gradle project loader accesses even gradle internal APIs that can be less stable than the proper APIs.

This PR centralizes the gradle internal access to GradleInternalAdapter class. Each call to an internal API should be now guarded with safeCall that blocks thrown Errors and RuntimeExceptions and logs them as problems, but continue the loading process.

The main NbProjectInfoBuilder should be now free of references to Gradle internals. To accommodate different API versions, the Adapter can be subclassed with alternative implementations for both newer or older versions that the one the gradle tooling is compiled againts.

… the project load, just logs a notification.
@sdedic sdedic added kind:bug Bug report or fix Gradle [ci] enable "build tools" tests labels Nov 7, 2022
@sdedic sdedic added this to the NB16 milestone Nov 7, 2022
@sdedic sdedic requested a review from lkishalmi November 7, 2022 17:35
@sdedic sdedic self-assigned this Nov 7, 2022
@sdedic sdedic linked an issue Nov 7, 2022 that may be closed by this pull request
Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

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

I like this implementation.

@lkishalmi
Copy link
Contributor

Well, better than none, though I feel we have cut ourselves by using Gradle internal stuff. I fear this is just a bandaid.

Copy link
Contributor

@lkishalmi lkishalmi left a comment

Choose a reason for hiding this comment

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

I've created a branch for Gradle 7.6-rc-1 upgrade. This one cannot compile with Gradle 7.6 as of:

build-tooling-lib:
> Task :clean

> Task :compileJava FAILED
/home/lkishalmi/NetBeansProjects/netbeans/extide/gradle/netbeans-gradle-tooling/src/main/java/org/netbeans/modules/gradle/tooling/GradleInternalAdapter.java:95: error: cannot find symbol
        return etv.isFixedValue();
                  ^
  symbol:   method isFixedValue()
  location: variable etv of type ExecutionTimeValue
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.
Note: Some input files use unchecked or unsafe operations.
Note: Recompile with -Xlint:unchecked for details.
1 error

@sdedic
Copy link
Member Author

sdedic commented Nov 8, 2022

I've created a branch for Gradle 7.6-rc-1 upgrade. This one cannot compile with Gradle 7.6 as of:

The same reason as hasFixedValue could not compile on pre-7.6-rc-1. The PR uses a regular call for the bundled gradle, and reflection for 'the other versions'. I thought the code will change to compile against 7.6 when the bundled APIs are upgraded (and will use reflection to work with earlier versions). This pattern is usually used elsewhere in JDK-specific stuff.

@sdedic
Copy link
Member Author

sdedic commented Nov 8, 2022

re.: using internal APIs -- currently the usage is rather minimalistic and the benefit of getting default + all conventions applied is enormous (configured values), compared to 'hand-made' extraction and approximation - given all the flexibility an imperative buildscript gives to the user. It's still an API, internal - but still with some degree of maintenance, as it provides interface between Gradle subprojects.

So let me disagree with cutting it out. Maybe @swpalmer / @errael suggestion would be ideal: ask to export a minimalistic introspection API (see discussion in #4923)

@sdedic
Copy link
Member Author

sdedic commented Nov 8, 2022

@lkishalmi if you point me to the branch for 7.6 update, I can contribute apdated reflection code there.

Additional note: if the code will not directly link against at least some Gradle version, we won't notice that the called method was refactored out at all, maybe not even during an upgrade - a possible change will not fail the build, but will fail at runtime only.

@sdedic sdedic requested a review from dbalek November 8, 2022 09:27
@sdedic
Copy link
Member Author

sdedic commented Nov 8, 2022

@neilcsmith-net please decide on the solution with respect to the release schedule:

  1. leave unattended for NB16
  2. go with the PR as it is, then upgrade to Gradle 7.6-rc1 to cover JDK19
  3. make calls for both versions reflective for NB16, then discuss whether that is appropriate
  4. other solution

@neilcsmith-net
Copy link
Member

@sdedic I'd rather stay out of making that call if possible. That's one for you and other reviewers to make primarily - this is covered at https://lists.apache.org/thread/hyjbsz55zb9xfcnccghkrsvqsnt83nwf

Maybe @MartinBalin has a view from releases side? If this was in Maven support I'd feel more comfortable taking a view. @ebarboni ?

@sdedic
Copy link
Member Author

sdedic commented Nov 8, 2022

@sdedic I'd rather stay out of making that call if possible.

OK, let's wait (even with RC4 ?) on how the consensus evolves.

@MartinBalin
Copy link
Contributor

I would like to proceed and merge this PR for RC4 NB16 don't wait for next release.

@neilcsmith-net neilcsmith-net added the priority:high High priority issue that should, if possible, be fixed in next release label Nov 8, 2022
@neilcsmith-net
Copy link
Member

OK, @MartinBalin made the call - we're having RC4! 😄

Let's hold off merge until @lkishalmi is happy with what's going in.

Copy link
Contributor

@lkishalmi lkishalmi left a comment

Choose a reason for hiding this comment

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

Well, re-checked the implementation. Changing my vote.
I'm more on favor to have this one on NB16, then probably have the Gradle upgrade on NB17.

@MartinBalin MartinBalin merged commit 31c6618 into apache:delivery Nov 8, 2022
@MartinBalin
Copy link
Contributor

Thank's all, merged to delivery to be in NB16.

@swpalmer
Copy link
Contributor

swpalmer commented Nov 8, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Gradle [ci] enable "build tools" tests kind:bug Bug report or fix priority:high High priority issue that should, if possible, be fixed in next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gradle Project Loading issues with NetBeans 16 RC-3
6 participants