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

Multiple protobufs with same filename are not compiled #248

Closed
stetra opened this issue Jul 19, 2018 · 4 comments
Closed

Multiple protobufs with same filename are not compiled #248

stetra opened this issue Jul 19, 2018 · 4 comments
Assignees

Comments

@stetra
Copy link

stetra commented Jul 19, 2018

It seems that if there are multiple .proto files with the same filename, only the last added one will get compiled.

Suppose I have two protobuf files, both named foo.proto:

protobuf
└── util
    ├── v1
    │   └── foo.proto
    └── v2
        └── foo.proto

If my build.gradle is configured like so:

dependencies {
    protobuf fileTree("protobuf/util/v1")
    protobuf fileTree("protobuf/util/v2")
}

Then when running gradle build only protobuf/util/v2/foo.proto is compiled and sources are generated under build/generated/source/proto/. However if I reverse the order of the dependencies:

dependencies {
    protobuf fileTree("protobuf/util/v2")
    protobuf fileTree("protobuf/util/v1")
}

Then only protobuf/util/v1/foo.proto is compiled. I have created an example repo at https://github.com/stetra/protobuf-gradle-plugin-example

Is this the expected behavior? How can I ensure both foo.proto protobufs are compiled without having to rename them?

@zpencer
Copy link
Contributor

zpencer commented Jul 19, 2018

In your repo, if you move your proto directory to src/main/proto then everything works as expected.
Looking at the output of gradlew build --debug:

13:20:21.418 [DEBUG] [org.gradle.internal.operations.DefaultBuildOperationExecutor] Build operation 'Resolve files of :protobuf' completed
13:20:21.418 [DEBUG] [org.gradle.api.Task] Extracting protos from /tmp/protobuf-gradle-plugin-example/protobuf/util/v1/foo.proto to /tmp/protobuf-gradle-plugin-example/build/extracted-protos/main
13:20:21.419 [DEBUG] [org.gradle.api.Task] Extracting protos from /tmp/protobuf-gradle-plugin-example/protobuf/util/v2/foo.proto to /tmp/protobuf-gradle-plugin-example/build/extracted-protos/main

Each .proto file was added individually, and they get copied to the destination overwriting each other. `

Alternatively you can use this config if you must use the non default location:

plugins {
    id "com.google.protobuf" version "0.8.6"
    id "java"
}

sourceCompatibility = 1.8
targetCompatibility = 1.8

repositories {
    mavenCentral()
}

dependencies {
    compile 'com.google.protobuf:protobuf-java:3.6.0'
    
}

protobuf {
    protoc {
        // The protobuf compiler, should be compatible with protobuf-java
        artifact = 'com.google.protobuf:protoc:3.6.0'
    }
}


sourceSets {
  main {
    proto {
      srcDir 'protobuf/'
    }
  }
}

@zhangkun83
Copy link
Collaborator

Having just protobuf fileTree("protobuf/util") still copies individual files into the destination directory. It should have copied the directory tree. This is indeed a bug.

The problem is in ProtobufExtract.groovy:

  @TaskAction
  void extract() {
    inputs.files.each { file ->
      logger.debug "Extracting protos from ${file} to ${destDir}"
      if (file.isDirectory()) {
        project.copy {
          includeEmptyDirs(false)
          from(file.path) {
            include '**/*.proto'
          }
          into(destDir)
        }

It appears when a directory is given in the build script, it's expanded into individual files when given to inputs. The code above never sees the original directory.

@zhangkun83
Copy link
Collaborator

I think we incorrectly recommended the use of:

protobuf fileTree("ext/")

With this syntax Gradle will collect all files under the given directory and emit them as individual files, leaving the plugin unable to tell what the root directory is, thus dumping the files under the same directory.

If we just pass the directory to the configuration, the current implementation will correctly handle the copying of a whole directory, preserving its structure:

protobuf files("ext/")

I am going to make change the tests to have the latter syntax, and add warnings if the former syntax is detected.

/cc @ejona86

@ejona86
Copy link
Collaborator

ejona86 commented Jan 17, 2019

SGTM

zhangkun83 added a commit to zhangkun83/protobuf-gradle-plugin-1 that referenced this issue Jan 17, 2019
@zhangkun83 zhangkun83 self-assigned this Jan 17, 2019
zhangkun83 added a commit that referenced this issue Jan 18, 2019
We **incorrectly** recommended the use of:
```gradle
protobuf fileTree("ext/")
```
With this syntax Gradle will collect all files under the given directory and emit them as individual files, leaving the plugin unable to tell what the root directory is, thus dumping the files under the same directory.

If we just pass the directory to the configuration, the current implementation will correctly handle the copying of a whole directory, preserving its structure:
``` gradle
protobuf files("ext/")
```

This PR includes a test to reproduce the issue, and updates tests and examples to use the latter syntax. A warning will be printed if the former syntax is detected.

Resolves #248
@pvegh pvegh mentioned this issue Aug 5, 2021
chemag added a commit to chemag/encapp that referenced this issue Jun 10, 2022
Upgraded gradle version to latest (7.4.2) and added some gradle option.

Tested:

Build the app.

```
$ java --version
openjdk 17.0.3 2022-04-19
OpenJDK Runtime Environment 21.9 (build 17.0.3+7)
OpenJDK 64-Bit Server VM 21.9 (build 17.0.3+7, mixed mode, sharing)
```

```
$ ./gradlew clean
$ ./gradlew build
Warning: Mapping new ns http://schemas.android.com/repository/android/common/02 to old ns http://schemas.android.com/repository/android/common/01
Warning: Mapping new ns http://schemas.android.com/repository/android/generic/02 to old ns http://schemas.android.com/repository/android/generic/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/addon2/02 to old ns http://schemas.android.com/sdk/android/repo/addon2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/addon2/03 to old ns http://schemas.android.com/sdk/android/repo/addon2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/repository2/02 to old ns http://schemas.android.com/sdk/android/repo/repository2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/repository2/03 to old ns http://schemas.android.com/sdk/android/repo/repository2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/sys-img2/03 to old ns http://schemas.android.com/sdk/android/repo/sys-img2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/sys-img2/02 to old ns http://schemas.android.com/sdk/android/repo/sys-img2/01
Warning: Mapping new ns http://schemas.android.com/repository/android/common/02 to old ns http://schemas.android.com/repository/android/common/01
Warning: Mapping new ns http://schemas.android.com/repository/android/generic/02 to old ns http://schemas.android.com/repository/android/generic/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/addon2/02 to old ns http://schemas.android.com/sdk/android/repo/addon2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/addon2/03 to old ns http://schemas.android.com/sdk/android/repo/addon2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/repository2/02 to old ns http://schemas.android.com/sdk/android/repo/repository2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/repository2/03 to old ns http://schemas.android.com/sdk/android/repo/repository2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/sys-img2/03 to old ns http://schemas.android.com/sdk/android/repo/sys-img2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/sys-img2/02 to old ns http://schemas.android.com/sdk/android/repo/sys-img2/01

> Task :app:lint
Execution optimizations have been disabled for task ':app:lint' to ensure correctness due to the following reasons:
  - In plugin 'com.android.internal.version-check' type 'com.android.build.gradle.tasks.LintGlobalTask' property 'allInputs' cannot be resolved:  Cannot convert the provided notation to a File or URI: appcompat-1.0.0.aar (androidx.appcompat:appcompat:1.0.0). The following types/formats are supported:  - A String or CharSequence path, for example 'src/main/java' or '/usr/include'. - A String or CharSequence URI, for example 'file:/usr/include'. - A File instance. - A Path instance. - A Directory instance. - A RegularFile instance. - A URI or URL instance. - A TextResource instance. Reason: An input file collection couldn't be resolved, making it impossible to determine task inputs. Please refer to https://docs.gradle.org/7.4.2/userguide/validation_problems.html#unresolvable_input for more details about this problem.
Ran lint on variant debug: 10 issues found
Ran lint on variant release: 10 issues found
Wrote HTML report to file:///.../encapp/app/build/reports/lint-results.html
Wrote XML report to file:///.../encapp/app/build/reports/lint-results.xml

> Task :app:extractIncludeDebugUnitTestProto UP-TO-DATE
proto file '/.../encapp/proto/tests.proto' directly specified in configuration. It's likely you specified files('path/to/foo.proto') or fileTree('path/to/directory') in protobuf or compile configuration. This makes you vulnerable to google/protobuf-gradle-plugin#248. Please use files('path/to/directory') instead.

> Task :app:extractIncludeReleaseUnitTestProto UP-TO-DATE
proto file '/.../encapp/proto/tests.proto' directly specified in configuration. It's likely you specified files('path/to/foo.proto') or fileTree('path/to/directory') in protobuf or compile configuration. This makes you vulnerable to google/protobuf-gradle-plugin#248. Please use files('path/to/directory') instead.

Deprecated Gradle features were used in this build, making it incompatible with Gradle 8.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

See https://docs.gradle.org/7.4.2/userguide/command_line_interface.html#sec:command_line_warnings

Execution optimizations have been disabled for 1 invalid unit(s) of work during this build to ensure correctness.
Please consult deprecation warnings for more details.

BUILD SUCCESSFUL in 2s
69 actionable tasks: 1 executed, 68 up-to-date
```
JohanBlome pushed a commit to JohanBlome/encapp that referenced this issue Jun 11, 2022
Upgraded gradle version to latest (7.4.2) and added some gradle option.

Tested:

Build the app.

```
$ java --version
openjdk 17.0.3 2022-04-19
OpenJDK Runtime Environment 21.9 (build 17.0.3+7)
OpenJDK 64-Bit Server VM 21.9 (build 17.0.3+7, mixed mode, sharing)
```

```
$ ./gradlew clean
$ ./gradlew build
Warning: Mapping new ns http://schemas.android.com/repository/android/common/02 to old ns http://schemas.android.com/repository/android/common/01
Warning: Mapping new ns http://schemas.android.com/repository/android/generic/02 to old ns http://schemas.android.com/repository/android/generic/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/addon2/02 to old ns http://schemas.android.com/sdk/android/repo/addon2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/addon2/03 to old ns http://schemas.android.com/sdk/android/repo/addon2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/repository2/02 to old ns http://schemas.android.com/sdk/android/repo/repository2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/repository2/03 to old ns http://schemas.android.com/sdk/android/repo/repository2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/sys-img2/03 to old ns http://schemas.android.com/sdk/android/repo/sys-img2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/sys-img2/02 to old ns http://schemas.android.com/sdk/android/repo/sys-img2/01
Warning: Mapping new ns http://schemas.android.com/repository/android/common/02 to old ns http://schemas.android.com/repository/android/common/01
Warning: Mapping new ns http://schemas.android.com/repository/android/generic/02 to old ns http://schemas.android.com/repository/android/generic/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/addon2/02 to old ns http://schemas.android.com/sdk/android/repo/addon2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/addon2/03 to old ns http://schemas.android.com/sdk/android/repo/addon2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/repository2/02 to old ns http://schemas.android.com/sdk/android/repo/repository2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/repository2/03 to old ns http://schemas.android.com/sdk/android/repo/repository2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/sys-img2/03 to old ns http://schemas.android.com/sdk/android/repo/sys-img2/01
Warning: Mapping new ns http://schemas.android.com/sdk/android/repo/sys-img2/02 to old ns http://schemas.android.com/sdk/android/repo/sys-img2/01

> Task :app:lint
Execution optimizations have been disabled for task ':app:lint' to ensure correctness due to the following reasons:
  - In plugin 'com.android.internal.version-check' type 'com.android.build.gradle.tasks.LintGlobalTask' property 'allInputs' cannot be resolved:  Cannot convert the provided notation to a File or URI: appcompat-1.0.0.aar (androidx.appcompat:appcompat:1.0.0). The following types/formats are supported:  - A String or CharSequence path, for example 'src/main/java' or '/usr/include'. - A String or CharSequence URI, for example 'file:/usr/include'. - A File instance. - A Path instance. - A Directory instance. - A RegularFile instance. - A URI or URL instance. - A TextResource instance. Reason: An input file collection couldn't be resolved, making it impossible to determine task inputs. Please refer to https://docs.gradle.org/7.4.2/userguide/validation_problems.html#unresolvable_input for more details about this problem.
Ran lint on variant debug: 10 issues found
Ran lint on variant release: 10 issues found
Wrote HTML report to file:///.../encapp/app/build/reports/lint-results.html
Wrote XML report to file:///.../encapp/app/build/reports/lint-results.xml

> Task :app:extractIncludeDebugUnitTestProto UP-TO-DATE
proto file '/.../encapp/proto/tests.proto' directly specified in configuration. It's likely you specified files('path/to/foo.proto') or fileTree('path/to/directory') in protobuf or compile configuration. This makes you vulnerable to google/protobuf-gradle-plugin#248. Please use files('path/to/directory') instead.

> Task :app:extractIncludeReleaseUnitTestProto UP-TO-DATE
proto file '/.../encapp/proto/tests.proto' directly specified in configuration. It's likely you specified files('path/to/foo.proto') or fileTree('path/to/directory') in protobuf or compile configuration. This makes you vulnerable to google/protobuf-gradle-plugin#248. Please use files('path/to/directory') instead.

Deprecated Gradle features were used in this build, making it incompatible with Gradle 8.0.

You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.

See https://docs.gradle.org/7.4.2/userguide/command_line_interface.html#sec:command_line_warnings

Execution optimizations have been disabled for 1 invalid unit(s) of work during this build to ensure correctness.
Please consult deprecation warnings for more details.

BUILD SUCCESSFUL in 2s
69 actionable tasks: 1 executed, 68 up-to-date
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants