novoda/gradle-static-analysis-plugin

Upgrade Gradle to 5.1.1

zegnus opened this issue · 16 comments

zegnus commented

The current used version is 4.10.2 https://github.com/novoda/gradle-static-analysis-plugin/blob/master/sample-multi-module/gradle/wrapper/gradle-wrapper.properties#L3

It would be great to ensure that the plugin supports the latest 5.x available Gradle version, including the sample applications.

tasomaniac commented

@zegnus, I can verify that it works fine with Gradle 5.x.

Since we don't use any of internal APIs, it should always be backward compatible.

I have an open PR #165 where I try to integrate lazy task configuration from Gradle 5 in a backward compatible fashion. We will definitely upgrade to 5.1.1 together with that.

There is no practicle reason to update now. Having said that, feel free to open a PR for the upgrade. 🙌

zegnus commented

I cannot open a PR for this upgrade because the sample applications fail:

In sample/app/gradle/wrapper/gradle-wrapper.properties use gradle-5.1.1-all.zip

then, inside the sample directory, run:

./gradlew checkstyleMain

It fails with the following:

Execution failed for task ':app:checkstyleMain'.
> Cannot convert org.gradle.api.internal.file.collections.DefaultSingletonFileTree@3e0a3cdc to local file system directories.
tasomaniac commented

I thought I've been using it with 5.1.1 but I don't have checkstyle integrated. So something is wrong with checkstyle then.

I will have a look.

zegnus commented

This is the line that seems to break:
https://github.com/novoda/gradle-static-analysis-plugin/blob/master/plugin/src/main/groovy/com/novoda/staticanalysis/internal/SourceFilter.groovy#L36

task.source = task.source.findAll { !excludedFiles.contains(it) }

tasomaniac commented

I've seen that Gradle team had a fix for checkstyle on 5.2. Can you double check with that version? Thanks

zegnus commented

Checked with distributionUrl=https\://services.gradle.org/distributions/gradle-5.2-all.zip, it also fails

pablisco commented

Tried with 5.2.1 and Java 8 (below 8 not supported) and it assembles correctly.
It fails on tests.
The first failure was regarding the lack of settings.gradle (as it was using the one from the runner project instead of the test project). That one is easy to fix.
After fixing that, we get the following error thrown on every test:

* What went wrong:
Metaspace

It seems to be having a memory issues as that error comes after an OOM Exception.

The sample test directory runs correctly when manually run from command line (after adding the version to the dependencies), so we think it may be related to GradleRunner and/or the testKit.

tasomaniac commented

Thanks @pablisco We can update the Gradle version in a PR and see what CI says.

You are describing how you try to build the project. What about using the latest stable version with Gradle 5.2.1 in a project?

pablisco commented

Looking at updating this project to work with 5.x first.
@mr-archano After adding more memory to the test project I get this error:

The file lock is held by a different Gradle process (pid: 88765, lockId: 5612053480214148201). Pinged owner at port 64160
pablisco commented

@tasomaniac These are the changes I've tried so far if you want to try them locally:
https://github.com/novoda/gradle-static-analysis-plugin/compare/upgrade-gradle-to-5

mr-archano commented

Ok, I started to take a look at this myself. I have pushed some changes to develop in order to focus on just the changes that are needed. I will post here all my updates.

mr-archano commented

I quickly tried to use the latest snapshot on a project using Gradle 5.2.1 and found an issue. A PR with a fix is open: #179.

mr-archano commented

With #180 we should have a snapshot that works correctly with projects running with Gradle 5.x

I have tried the snapshot on the samples and on another app using Gradle 5.x with success. This means that we will not have any urgent need to migrate the plugin to compile against Gradle 5.x as we are not using any more deprecated API or similar.

mr-archano commented

The issue with Gradle 5.x and test-kit is more complicated than expected, and we will need help from the Gradle team to fix it.
The gradle-static-analysis-plugin doesn't need to compile against newer versions of Gradle, given we don't need to use any of the new APIs, and test-kit is used just to run functional tests. This means we'll stick with Gradle 4.10.2 for the time being.

I'll kip this issue open to track the conversation around the OutOfMemoryError in test-kit.

worstkiller commented

Hi All,
It seems this has been fixed in 5.6 release, updating to 5.6 makes the error gone.

tasomaniac commented

This is finally done as part of #203