-
Notifications
You must be signed in to change notification settings - Fork 108
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Upgrade to Gradle 5.0 #900
Upgrade to Gradle 5.0 #900
Conversation
Remove errorprone Remove findbugs Remove buildSrc project Move generated code into core Give each project its own buildscript Remove AutoValue, replace with lombok
Revert lombok change
Fix new a problem identified by spotbugs
Codecov Report
@@ Coverage Diff @@
## master #900 +/- ##
============================================
- Coverage 51.71% 50.37% -1.35%
============================================
Files 247 304 +57
Lines 7812 8318 +506
Branches 533 535 +2
============================================
+ Hits 4040 4190 +150
- Misses 3590 3951 +361
+ Partials 182 177 -5 |
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.
* Where:
Build file '/Users/austinshalit/git/GRIP/build.gradle.kts' line: 50
* What went wrong:
No .git directory found!
And yes... there is a git directory. |
@@ -36,7 +36,7 @@ install: | |||
script: | |||
# Only run code generation tests on OSX -- linux requires sudo to install OpenCV dependencies, and that environment | |||
# will not be able to run MainWindowTest.testDragOperationFromPaletteToPipeline | |||
- ./gradlew check jacocoTestReport jacocoRootReport --stacktrace -Pheadless=true -PlogTests --scan; |
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.
Why the removal of the --scan
?
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.
It was causing Travis to fail from having to accept the license agreement. Since Travis is getting removed anyway, I'm not worried about temporarily reducing its workload to keep the important stuff working until then
repositories { | ||
jcenter() | ||
maven { | ||
setUrl("https://github.com/WPIRoboticsProjects/opencv-maven/raw/mvn-repo") |
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.
HTTPS support. Wonderful!
Github might get pissy though if we keep using them as a maven repo.
build.gradle.kts
Outdated
@@ -0,0 +1,201 @@ | |||
import org.ajoberstar.grgit.Grgit | |||
import com.github.spotbugs.* |
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 we avoid wildcard imports?
tasks.withType<Wrapper>().configureEach { | ||
gradleVersion = "5.0" | ||
distributionType = Wrapper.DistributionType.ALL | ||
} |
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 usually keep this at the bottom. Why 5.0
instead of 5.1
? The memory leak issue?
plugin("org.ajoberstar.grgit") | ||
} | ||
grgit = Grgit.open() | ||
} |
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.
You could do this as:
val grgit: Grgit? by lazy {
// blah....
}
jcenter() | ||
maven { | ||
name = "WPILib Maven Release" | ||
setUrl("http://first.wpi.edu/FRC/roborio/maven/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.
Booo. Not a fan of http
here.
Can you use the new Gradle repository filers so that only artifacts that are actually on these servers get searched in these repos?
https://docs.gradle.org/current/userguide/declaring_repositories.html#declaring_a_repository_filter
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.
first.wpi.edu unfortunately does not have an HTTPS cert
The repository filter looks nice, though
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.
Darn, the filtering isn't available in 5.0
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.
Is there any sort of hash validation we can do before we just start arbitrarily running code that was downloaded insecurely?
Is there some way we can validate the SHA 256 hash of it first or something?
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 think there's anything we can do beyond what I assume Gradle does already with using the .md5 or .sha hash files in the Maven repo.
This is also out of scope for this PR
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.
Fair enough.
reports { | ||
xml.isEnabled = false | ||
emacs.isEnabled = 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.
You can get spotbugs to produce an html
report if we don't want to deal with the emacs one anymore.
} | ||
|
||
tasks.withType<Javadoc> { | ||
source(tasks.named<JavaCompile>("compileJava").map { it.source }) |
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.
👍
additionalSourceDirs(srcFiles) | ||
sourceDirectories.from(srcFiles) | ||
classDirectories.from(files(sourceSets["main"].output)) | ||
executionData.from(tasks.named<JacocoReport>("jacocoTestReport").map { it.executionData }) |
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.
Are these from
s cumulative or last one wins?
This is the logic I've been using for creating a root report recently.
https://github.com/JLLeitschuh/kotlin-guiced/blob/master/kotlin-guiced.gradle.kts#L148-L178
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 have no idea. This is just a port of the old task (which wasn't ever running AFAICT) to Kotlin.
} | ||
assert project.buildFile.isFile(): "File named $groovyName or $kotlinName must exist." | ||
project.children.each { setUpChildProject(it) } | ||
} |
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.
This file can be Kotlin based now if you want it to be. Up to you though.
tasks.named<JavaExec>("run") { | ||
classpath = sourceSets["main"].runtimeClasspath | ||
main = application.mainClassName | ||
args = listOf("windowed") |
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.
Windowed?
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.
For launching the preloader by itself. See the preloader application class
compile(group = "com.hierynomus", name = "sshj", version = "0.16.0") | ||
compile(group = "org.apache.velocity", name = "velocity", version = "1.7") | ||
|
||
val coreTestOutput = project(":core").dependencyProject.sourceSets["test"].output |
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.
Eventually, we should fix this better.
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.
Overall really nice work. Just a few comments on things to potentially clean up.
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.
LGTM!
:core
Supersedes and closes #860