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

Improved Gradle Support #165

Closed
breskeby opened this issue May 7, 2020 · 1 comment
Closed

Improved Gradle Support #165

breskeby opened this issue May 7, 2020 · 1 comment
Assignees

Comments

@breskeby
Copy link

breskeby commented May 7, 2020

This is a general ticket about some limitations I've seen in the existing gradle support that I would like to discuss.

  • The bootstrapping of compiling a groovy script within the plugin is very ineffective and adds a significant overhead to builds using a cold daemon.
  • The task CheckForbiddenApis has deprecated behaviour:
    • classesDir is not annotated with an input or output annotation
    • patternSet is not annotated with an input or output annotation

The bootstrapping issue is problematic in my opinion. It is quite costly , especially with cold Gradle daemons Can you elaborate why you've done it like this?
As what I can see you build the plugin against a certain set of Gradle artifacts that do not include the whole set of classes you would require to compile everything statically here. Usually when writing a Gradle plugin, plugin authors would rely on the gradleApi jar that is generated by Gradle at runtime.
Since you're using ant you do not have access to that jar. One option could be to ship the generated binary as part of the build and reference it from the build. That way we could relatively easily imo move this bootstrapping groovy script into the static typed plugin. What do you think.

As far as I understand you're concerned about backwards compatibility right? which Gradle versions do you want / need to keep supporting here? Gradle 3.2 is almost 4 years old already. Not sure how much sense it makes to keep supporting this version in newer plugins.

Would you be generally open to split out the Gradle specific logic into its own project / subproject? I'm happy to help with that. This would make working on the plugin way easier and we could detangle the version of the Gradle plugin from the general different forbidden-apis version.

@uschindler
Copy link
Member

Hi,

sorry, but most of the stuff you mentioned here are contray what was done before release of 3.0. Have you looked at version 3.0?

The bootstrapping issue is problematic in my opinion. It is quite costly , especially with cold Gradle daemons Can you elaborate why you've done it like this?

Any numbers about this.?The bootstrap cost ist not even measureable, you can compile that script multiple times and still it takes only milliseconds. And: If you look at it: It's a static member in the Plugin class. There is no overhead, not even for a huge multi-module project with many submodules.

About your annotation problems:

The task CheckForbiddenApis has deprecated behaviour:
classesDir is not annotated with an input or output annotation
patternSet is not annotated with an input or output annotation

Annotating those with Input is not needed and was explicitely removed (see #159), because the patternset is indirectly changing the main @InputFiles attached to getClassFiles() (read-only property), which refers to every single class file. If you change the patternset the classFiles readonly property changes and uptodate detection works as expected. For comparison, please have a look at all the plugin using patternsets in the Gradle Source code. As far as I rember you worked once for Gradle, so you should have an idea how the tasks are done. All Gradle-internal tasks don't declare those "duplicates" as input, just @internal (as replacement to get rid of the warning about misisng annotations).

classesDir and the same for classesDirs is the same issue, because the actual value does not matter, the input files change the main classesFiles readonly property automatically. In fact adding @input would cache the file status 2 times and costs time when Gradle recalculates hashes twice. I would also recommend to check the Gradle source code, it's handled the same way.

So this won't be changed, I disagree about that.

As what I can see you build the plugin against a certain set of Gradle artifacts that do not include the whole set of classes you would require to compile everything statically here. Usually when writing a Gradle plugin, plugin authors would rely on the gradleApi jar that is generated by Gradle at runtime.

Tell Gradle people to publish their stuff on Maven Central. Sorry I have to say this!

Since you're using ant you do not have access to that jar. One option could be to ship the generated binary as part of the build and reference it from the build. That way we could relatively easily imo move this bootstrapping groovy script into the static typed plugin. What do you think.

I don't want to change this because especially gradleApi() has a huge problem: It includes classes in antique old versions that are required for complilation (shaded afterwards). forbiddenapis needs the latest ASM version in exactly the version used here available, and gradle has old shit bundled and shipped and visible in gradleApi(). I was once trying to convert the build to Gradle and gave up. For the same reason ASM is shaded inside the forbiddenapis JAR file.

As far as I understand you're concerned about backwards compatibility right? which Gradle versions do you want / need to keep supporting here? Gradle 3.2 is almost 4 years old already.

forbiddenapis has Java 7 as minimum requirement and many projects especially around Apache with a huge userbase are still using this. So Gradle version cannot be upgraded. This one just uses the one which is required to still have Java 7 and on the other hand support @Classpath annotation and we can detect Task avoidance support.

Sorry, I disagree with all here. If you want support close for this in Gradle, add it to Gradle itsself and implement it there, then the maintenance burden is no longer on my side (you have the contacts). I could remove the Gradle stuff from the plugin and Gradle just uses the public API, like it does for Checkstyle & Co. I have to say that Gradle is a desaster for plugin authors, all the time something changes and you can't see any stable APIs. With Maven you can use a plugin from Maven 2 (forbiddenapis compiles against it) and it runs still without any warnings in Maven 3.6.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants