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

Add explicit DuplicatesStrategy as required by gradle 7+ (#470) #487

Merged
merged 7 commits into from
Apr 16, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import com.google.common.base.Preconditions
import groovy.transform.CompileDynamic
import org.gradle.api.DefaultTask
import org.gradle.api.file.ConfigurableFileCollection
import org.gradle.api.file.DuplicatesStrategy
import org.gradle.api.file.FileCollection
import org.gradle.api.file.FileSystemLocation
import org.gradle.api.file.FileTree
Expand Down Expand Up @@ -113,6 +114,8 @@ abstract class ProtobufExtract extends DefaultTask {
spec.from(allProtoFiles)
spec.include('**/*.proto')
spec.into(destDir)
// gradle 7+ requires a duplicate strategy to be explicitly defined
spec.duplicatesStrategy = DuplicatesStrategy.INCLUDE
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
package com.google.protobuf.gradle

import groovy.transform.CompileDynamic
import org.gradle.testkit.runner.BuildResult
import org.gradle.testkit.runner.GradleRunner
import org.gradle.testkit.runner.TaskOutcome
import spock.lang.Specification
import spock.lang.Unroll

/**
* Unit test confirming copy spec is explicitly defined for gradle7+ compliance.
*/
@CompileDynamic
class ProtobufKotlinDslCopySpecTest extends Specification {
private static final List<String> GRADLE_VERSIONS = ["5.6", "6.0", "6.7.1", "7.0"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added tests against full gradle suite


@Unroll
void "testProjectKotlinDslCopySpec should declare explicit copy spec [gradle #gradleVersion]"() {
given: "project from testProjectKotlinDslCopySpec"
File projectDir = ProtobufPluginTestHelper.projectBuilder('testProjectKotlinDslCopySpec')
.copyDirs('testProjectKotlinDslCopySpec')
.build()

when: "build is invoked"
BuildResult result = GradleRunner.create()
.withProjectDir(projectDir)
.withArguments('test', 'build', '--stacktrace')
.withPluginClasspath()
.withGradleVersion(gradleVersion)
.forwardStdOutput(new OutputStreamWriter(System.out))
.forwardStdError(new OutputStreamWriter(System.err))
.build()

then: "it succeed"

result.task(":test").outcome == TaskOutcome.SUCCESS

verifyProjectDir(projectDir)

where:
gradleVersion << GRADLE_VERSIONS
}

private static void verifyProjectDir(File projectDir) {
File generatedSrcDir = new File(projectDir.path, "build/generated/source/proto/main/java")
List<File> fileList = []
generatedSrcDir.eachFileRecurse { file ->
if (file.path.endsWith('.java')) {
fileList.add (file)
}
}
assert fileList.size > 0
}

}
6 changes: 3 additions & 3 deletions testProjectBase/build_base.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,10 @@ dependencies {
protobuf files("ext/")
testProtobuf files("lib/protos-test.tar.gz")

compile protobufDep
testCompile 'junit:junit:4.12'
implementation protobufDep
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change allows ProtobufJavaPluginTest suite to run under gradle 7, but issues relating to copy dirs are causing gradle 7 :jar task to fail (failure is in test itself, not connected to plugin). It requires some refactoring to enable 7.0 testing for ProtobufJavaPluginTest

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But ProtobufJavaPluginTest doesn't include Gradle 7 now, and ProtobufKotlinDslCopySpecTest is independent. So this change doesn't seem to be necessary now, does it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not necessary, no. It just means when someone does look at gradle 7, the tests compile, so thought it was better to make the fix preemptively as it's backwards compatible to gradle 5.6.
Should I revert it?

testImplementation 'junit:junit:4.12'
// KotlinFooTest.kt requires reflection utilities
testCompile "org.jetbrains.kotlin:kotlin-reflect:1.2.0"
testImplementation "org.jetbrains.kotlin:kotlin-reflect:1.2.0"
grpcCompile protobufDep
grpcCompile 'io.grpc:grpc-stub:1.0.0-pre2'
grpcCompile 'io.grpc:grpc-protobuf:1.0.0-pre2'
Expand Down
6 changes: 3 additions & 3 deletions testProjectDependent/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ repositories {
}

dependencies {
compile 'com.google.protobuf:protobuf-java:3.0.0'
compile project(':testProject')
implementation 'com.google.protobuf:protobuf-java:3.0.0'
implementation project(':testProject')

testCompile 'junit:junit:4.12'
testImplementation 'junit:junit:4.12'
}

protobuf.protoc {
Expand Down
59 changes: 59 additions & 0 deletions testProjectKotlinDslCopySpec/build.gradle.kts
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import com.google.protobuf.gradle.generateProtoTasks
import com.google.protobuf.gradle.id
import com.google.protobuf.gradle.ofSourceSet
import com.google.protobuf.gradle.plugins
import com.google.protobuf.gradle.protobuf
import com.google.protobuf.gradle.protoc

plugins {
java
id("java-library")
id("com.google.protobuf").version("0.8.15")
Copy link
Collaborator

@voidzcy voidzcy Apr 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem right. You shouldn't specify the 0.8.15 version. Otherwise, it's not actually testing the plugin built from the project (tests still pass, which means the change isn't really get covered. Or the Gradle TestKit just ignores the version while still injecting the plugin built from code to the classpath of the test build? I am not sure, try delete this version setting and confirm you are really testing the code instead of the plugin pulled from Maven).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting. This is an interesting one, I will investigate it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have pushed a sanity check to confirm that the test fails under 0.8.15 - pushed here:
https://github.com/tom-haines/protobuf-gradle-plugin/tree/example-project/prove_test_fail

}

repositories {
maven("https://plugins.gradle.org/m2/")
}

java {
sourceCompatibility = JavaVersion.VERSION_1_8
targetCompatibility = JavaVersion.VERSION_1_8
}

val protobufDepVersion = "3.0.0"
val grpcVersion = "1.37.0"

dependencies {
implementation("io.grpc:grpc-protobuf:$grpcVersion")
implementation("io.grpc:grpc-stub:$grpcVersion")
implementation("com.google.protobuf:protobuf-java:$protobufDepVersion")
testImplementation("org.junit.jupiter:junit-jupiter-api:5.7.1")
testRuntimeOnly("org.junit.jupiter:junit-jupiter-engine:5.7.1")
}


protobuf {
protoc {
artifact = "com.google.protobuf:protoc:$protobufDepVersion"
}
plugins {
id("grpc") {
artifact = "io.grpc:protoc-gen-grpc-java:$grpcVersion"
}
}
generateProtoTasks {
ofSourceSet("main").forEach {
it.plugins {
id("grpc") {
outputSubDir = "grpc"
}
}
}
}
}
tasks {
test {
useJUnitPlatform {
}
}
}
8 changes: 8 additions & 0 deletions testProjectKotlinDslCopySpec/settings.gradle.kts
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
pluginManagement {

repositories {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be put directly into the repositories{} closure in the buildscript.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok will do

gradlePluginPortal()
google()
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// Copyright 2020 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

syntax = "proto3";

package google.type;

option cc_enable_arenas = true;
option go_package = "google.golang.org/genproto/googleapis/type/money;money";
option java_multiple_files = true;
option java_outer_classname = "MoneyProto";
option java_package = "com.google.type";
option objc_class_prefix = "GTP";

// Represents an amount of money with its currency type.
message Money {
// The three-letter currency code defined in ISO 4217.
string currency_code = 1;

// The whole units of the amount.
// For example if `currencyCode` is `"USD"`, then 1 unit is one US dollar.
int64 units = 2;

// Number of nano (10^-9) units of the amount.
// The value must be between -999,999,999 and +999,999,999 inclusive.
// If `units` is positive, `nanos` must be positive or zero.
// If `units` is zero, `nanos` can be positive, zero, or negative.
// If `units` is negative, `nanos` must be negative or zero.
// For example $-1.75 is represented as `units`=-1 and `nanos`=-750,000,000.
int32 nanos = 3;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: leave an empty line at the end of file.

Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package com.myapp;

import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;

public class ExampleUnitTest {

@Test
public void addition_isCorrect() {
Assertions.assertEquals(4, 2 + 2);
}
}