Skip to content
This repository has been archived by the owner on Feb 11, 2022. It is now read-only.

Upgrade Gradle to 5.1.1 #168

Closed
zegnus opened this issue Feb 2, 2019 · 16 comments
Closed

Upgrade Gradle to 5.1.1 #168

zegnus opened this issue Feb 2, 2019 · 16 comments

Comments

@zegnus
Copy link
Contributor

zegnus commented Feb 2, 2019

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
Copy link
Contributor

@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
Copy link
Contributor Author

zegnus commented Feb 3, 2019

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
Copy link
Contributor

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
Copy link
Contributor Author

zegnus commented Feb 15, 2019

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
Copy link
Contributor

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

@zegnus
Copy link
Contributor Author

zegnus commented Feb 19, 2019

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

@pablisco
Copy link
Contributor

pablisco commented Mar 4, 2019

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
Copy link
Contributor

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
Copy link
Contributor

pablisco commented Mar 4, 2019

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
Copy link
Contributor

pablisco commented Mar 4, 2019

@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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

mr-archano commented Mar 9, 2019

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
Copy link
Contributor

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
Copy link

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

@tasomaniac
Copy link
Contributor

tasomaniac commented Nov 8, 2019

This is finally done as part of #203

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants