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

Yet another Gradle update issue: 7.4 deprecated FileTrees using @SkipWhenEmpty #189

Closed
reta opened this issue Feb 9, 2022 · 19 comments · Fixed by #193
Closed

Yet another Gradle update issue: 7.4 deprecated FileTrees using @SkipWhenEmpty #189

reta opened this issue Feb 9, 2022 · 19 comments · Fixed by #193
Assignees
Milestone

Comments

@reta
Copy link

reta commented Feb 9, 2022

Gradle 7.4 (release notes [1]) deprecated @SkipWhenEmpty [2], builds (with strict deprecation check) fail with:

> Task <project>:forbiddenApisTest NO-SOURCE
Relying on FileTrees for ignoring empty directories when using @SkipWhenEmpty has been deprecated. This is scheduled to be removed in Gradle 8.0. Annotate the property classFiles with @IgnoreEmptyDirectories or remove @SkipWhenEmpty. Consult the upgrading guide for further information: https://docs.gradle.org/7.4/userguide/upgrading_version_7.html#empty_directories_file_tree

[1] https://docs.gradle.org/7.4/release-notes.html
[2] https://docs.gradle.org/current/userguide/upgrading_version_7.html#empty_directories_file_tree

@uschindler
Copy link
Member

uschindler commented Feb 9, 2022

We rely on "no files available", so we need to add the additional annotation also mentioned. Problem may be the minimum gradle version that does not have the annotation :-(

This is not urgent, but I will find a solution before the next release (coming with Java 18). For now ignore the warning.

@uschindler
Copy link
Member

uschindler commented Feb 9, 2022

Hi,
the @SkipWhenEmpty is not deprecated (see https://docs.gradle.org/7.4/javadoc/org/gradle/api/tasks/SkipWhenEmpty.html), it is only when used alone. You need to add @IgnoreEmptyDirectories then it is "correct use". But unfortunately this new annotation only exists since Gradle 6.8, so we can't add it for backwards compatibility: https://docs.gradle.org/7.4/javadoc/org/gradle/api/tasks/IgnoreEmptyDirectories.html

Sorry this can't be fixed easily. Please ignore the warning for now and fix it later. Disable with strict deprecation check, sorry for this!

@uschindler uschindler self-assigned this Feb 9, 2022
@reta
Copy link
Author

reta commented Feb 9, 2022

Thanks a lot for quick reply @uschindler !

@reta reta changed the title Yet another Gradle update issue: 7.4 deprecated @SkipWhenEmpty Yet another Gradle update issue: 7.4 deprecated FileTrees using @SkipWhenEmpty Feb 9, 2022
@uschindler
Copy link
Member

I think the only way to fix this for forbiddenapis while maintaining backwards compatibility with earlier versions (before Gradle 6.8, Minimum is 3.4), is to remove the annotation and exit in the executeTask method if the FileTree is empty (like @reta did in a first version of the PR opensearch-project/OpenSearch#2078). Unfortunately this causes Gradle to no longer print "NO-SOURCE" behind task name. I am also not sure if this slows additionally.

@reta
Copy link
Author

reta commented Feb 9, 2022

@uschindler thank you for the update, the "NO-SOURCE" indeed won't be there, using lifecycle() logger could compensate that a bit (but this is not the same certainly) with a message.

I am also not sure if this slows additionally.

I would suspect it may be slower since more evaluations may potentially be run before the the task is skipped manually. There is a hacky way to make it work with annotation (but I don't even want to suggest it for widely used plugin like forbidden-apis). Looks like manual check is the price to pay for wider compatibility.

@uschindler
Copy link
Member

There is a hacky way to make it work with annotation

which one? 🤔

@reta
Copy link
Author

reta commented Feb 10, 2022

There is a hacky way to make it work with annotation

which one? 🤔

There are 2 hacks to try (but I have not explored these routes):

  • add annotation at runtime (hardcore reflection / bytecode manipulation) [1]
  • shade the @IgnoreEmptyDirectories by adding the same package + class to the plugin (to compensate < Gradle 6.8) - not sure it will work, depends on how Gradle does the checks

[1] https://stackoverflow.com/questions/1635108/adding-java-annotations-at-runtime

@uschindler
Copy link
Member

Hi,

There are 2 hacks to try (but I have not explored these routes):

  • add annotation at runtime (hardcore reflection / bytecode manipulation) [1]

OK, this is not a good idea :-)

  • shade the @IgnoreEmptyDirectories by adding the same package + class to the plugin (to compensate < Gradle 6.8) - not sure it will work, depends on how Gradle does the checks

This may work, but that's not a good idea, and it is much more complex than adding an if-statement in the execute method.

I was hoping that like with Inputs, you can add them in the configuration phase. For inputs you can add the annotation, but on the other hand you can also add them using task.inputs. I was hoping you can maybe configure that setting. As the configuration phase of the plugin is handled by Groovy code (not compiled), this is the way how to make it compatible with different Gradle versions (see TASK_AVOIDANCE_AVAILABLE in https://github.com/policeman-tools/forbidden-apis/blob/main/src/main/resources/de/thetaphi/forbiddenapis/gradle/plugin-init.groovy).

@uschindler
Copy link
Member

I will check at weekend and implement some workaround.

This is why I hate Gradle's development model: They change their APIs and workflows all the time and then have no idea how to help to maintain plugins for various versions. It is OK to do this and also remove deprecated stuff in the next major version, but Gradle releases a new major version like every half a year!

@reta
Copy link
Author

reta commented Feb 10, 2022

I will check at weekend and implement some workaround.

This is why I hate Gradle's development model: They change their APIs and workflows all the time and then have no idea how to help to maintain plugins for various versions. It is OK to do this and also remove deprecated stuff in the next major version, but Gradle releases a new major version like every half a year!

I agree with you, the upgrade from 6.x to 7.x wiped out a week of my life but I really expected the update from 7.3.3 to 7.4 to be a no-brainer but ... :-\

@uschindler
Copy link
Member

Hi,
there might be an elegant solution: I can make the ForbiddenApis task extend SourceTask, which has everything needed:

  • PatternFilterable (so it applies patterns like the current task, so I can remove most of the code. I just need to set the "default pattern **/*.class
  • the getSource() method has the rquired annotations. As those are inherited there's nothing to be done. The task-specific public FileTree getClassFiles() can go away and the execute only calls getSource().

P.S.: I am about to relase v3.3 with JDK 18 support and the other cool features added recently.

@uschindler
Copy link
Member

I will provide a PR later today as this is a bit of tricky change. I am fighting convention properties at the moment. I have it working, but it has some problems.

@reta
Copy link
Author

reta commented Mar 23, 2022

Thanks a lot @uschindler !

@uschindler
Copy link
Member

uschindler commented Mar 24, 2022

Hi,
I played around with it but at end I gave up. The problem with SourceTask is that you cannot get the list of directories anymore once somebody set it. You would need to maintain the parallel FileCollection classesDirs. This works, but fails with task conventions. I got it running, but then I still have to force people to use setClassesDirs, but the other method to set setSource is still public. So at end in brings more problems than it solves.

shade the @IgnoreEmptyDirectories by adding the same package + class to the plugin (to compensate < Gradle 6.8) - not sure it will work, depends on how Gradle does the checks

This solution was very easy. I just package a copy of the annotation with original package name (not shaded) inside the JAR file. Due to the way classloading works (parent first), Gradle won't load it after Gradle 6.8. Before Gradle 6.8 it is loaded from the forbiddenapis JAR file but ignored by Gradle on reflective access to them (it is just an arbitrary annotation).

This is working fine and could also be done with other annotations. Other plugin authors have done the same in the meantime, so the idea was not new. This is mainly because this special annotation is too new (6.8) and brings major hassles. Most plugins in plugin store support a very large range of version numbers of Gradle, so their chance is only to ship with annotation.

Uwe

uschindler added a commit that referenced this issue Mar 24, 2022
… forbiddenapis task with it. This works around problems with Grdale 7.4+. Closes #189
@uschindler
Copy link
Member

uschindler commented Mar 24, 2022

Here is the pull request: #193; could you have a look and maybe review it?

@uschindler
Copy link
Member

This solution was very easy. I just package a copy of the annotation with original package name (not shaded) inside the JAR file. Due to the way classloading works (parent first), Gradle won't load it after Gradle 6.8. Before Gradle 6.8 it is loaded from the forbiddenapis JAR file but ignored by Gradle on reflective access to them (it is just an arbitrary annotation).

We don't even need to ship with the annotation, so I excluded it from JAR file. This is really the clEanest solution:

  • add a copy of annotation to source tree, just to be able to compile against it
  • exclude it from JAR file because at Runtime, Java does not need it (see JLS spec, notes about that in issue), although they are runtime. RUNTIME just means it is discoverable at runtime, but it does not require it to be there when class is linked. Class#getAnnotation() would just not return it.

@reta
Copy link
Author

reta commented Mar 24, 2022

Thank you @uschindler !

@uschindler
Copy link
Member

uschindler commented Mar 24, 2022

I only fixed another issue with forbiddens's own annotation processing (it wanted runtime annotations to be on classpath). This was a long standing issue, now also fixed: #194

@uschindler
Copy link
Member

Looks like all is working fine without any warnings in Gradle 7.4.1: https://jenkins.thetaphi.de/job/Forbidden-APIs-testonly-GradleLatest-Linux/82/console

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants