-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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
Upgrade to Gradle 4.2.1, remove nebula plugin dependency #5633
Conversation
Codecov Report
@@ Coverage Diff @@
## 2.x #5633 +/- ##
============================================
+ Coverage 96.16% 96.29% +0.13%
- Complexity 5843 5852 +9
============================================
Files 634 634
Lines 41539 41539
Branches 5752 5752
============================================
+ Hits 39946 40002 +56
+ Misses 650 608 -42
+ Partials 943 929 -14
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM, I'd like to check Jar Manifest config more closely later when I have time if you don't mind
build.gradle
Outdated
classpath 'gradle.plugin.nl.javadude.gradle.plugins:license-gradle-plugin:0.13.1' | ||
classpath "me.champeau.gradle:jmh-gradle-plugin:0.4.4" | ||
classpath 'com.jfrog.bintray.gradle:gradle-bintray-plugin:1.7.3' | ||
classpath "org.jfrog.buildinfo:build-info-extractor-gradle:latest.release" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd strongly recommend to replace latest.release
with particular version like 4.5.2
, otherwise build won't be reproducible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but these are snapshot/release builds which are by definition non-reproducible as there can be only one version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, non-reproducible also means that RxJava release might fail unpredictably because this plugin was updated even if no one changed anything Gradle-related in RxJava itself…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay.
distributionBase=GRADLE_USER_HOME | ||
distributionPath=wrapper/dists | ||
zipStoreBase=GRADLE_USER_HOME | ||
zipStorePath=wrapper/dists | ||
distributionUrl=https\://services.gradle.org/distributions/gradle-2.14-bin.zip | ||
distributionUrl=https\://services.gradle.org/distributions/gradle-4.2-bin.zip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please run something like ./gradlew --version
on this branch?
It should update gradle/wrapper/gradle-wrapper.jar
and gradlew
and you need to commit them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did exactly that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like 4.2.1 came out just now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well yes, but it still should have updated wrapper jar and bash script hm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
build.gradle
Outdated
apply plugin: 'nebula.rxjava-project' | ||
apply plugin: 'maven' | ||
apply plugin: 'osgi' | ||
apply plugin: "me.champeau.gradle.jmh" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit: can you please use either single or double quotes for consistency?
(Separate PR was always overkill for that, but since you're modifying the files it feels like a great opportunity to finally do that)
build.gradle
Outdated
instruction 'Bundle-Vendor', 'RxJava Contributors' | ||
instruction 'Bundle-DocURL', 'https://github.com/ReactiveX/RxJava' | ||
instruction 'Import-Package', '!org.junit,!junit.framework,!org.mockito.*,*' | ||
instruction 'Eclipse-ExtensibleAPI', 'true' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this in manifest file of 2.1.4, do we need it?
build.gradle
Outdated
name = 'rxjava' | ||
instruction 'Bundle-Vendor', 'RxJava Contributors' | ||
instruction 'Bundle-DocURL', 'https://github.com/ReactiveX/RxJava' | ||
instruction 'Import-Package', '!org.junit,!junit.framework,!org.mockito.*,*' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about testNG?
As an alternative, you can explicitly declare only org.reactivestreams;version="[1.0,2)"
.
Or we can figure out how to dynamically collect compile
dependencies to put them here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these are needed by the OSGi plugin MANIFEST.MF. I don't know how relevant this is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's then also remove the findbugs
& pmd
apply plugins calls that are commented out?
Removed findbugs and pmd references. Squashed. |
Let's merge it then. Note the artifactory repo seems to deduplicate files. I referenced the snapshot in a separate project and it worked. |
This PR upgrades the Gradle runtime to 4.2 (which supports Java 9) and gets rid of the
rxjava-nebula
plugin. Its two features, publishing a snapshot and publishing a release has been manually implemented inbuild.gradle
based on some [online examples] and RxAndroid's variant.The snapshot publication works and to test the release, we may have to burn a couple of version tags from 2.1.x.
The PR also renamed the
perf
directory intojmh
because the replacement JMH plugin expects those files there.