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

protobuf packaging doesnt retain directory structure in Android aar libraries #435

Closed
nydrani opened this issue Sep 21, 2020 · 7 comments · Fixed by #440
Closed

protobuf packaging doesnt retain directory structure in Android aar libraries #435

nydrani opened this issue Sep 21, 2020 · 7 comments · Fixed by #440
Labels

Comments

@nydrani
Copy link

nydrani commented Sep 21, 2020

Generating a protobuf library on android doesn't retain the directory structure from the base "src/main/proto" folder.

e.g. A folder structure of:

src/main/proto
├── a.proto
├── firstfolder
│   └── b.proto
└── secondfolder
    └── c.proto

will package all corresponding .protobuf files into the root directory of the packaged .aar file instead of in the corresponding folders (firstfolder and secondfolder).

This means that applications consuming the protobuf library and trying to import the b.proto file using
import "firstfolder/b.proto" will fail.

I assume the inclusion of proto files is here: https://github.com/google/protobuf-gradle-plugin/blob/master/src/main/groovy/com/google/protobuf/gradle/ProtobufPlugin.groovy#L273 or https://github.com/google/protobuf-gradle-plugin/blob/master/src/main/groovy/com/google/protobuf/gradle/ProtobufPlugin.groovy#L293. But I'm not quite sure why the directory structure is not retained.

@voidzcy voidzcy added the bug label Sep 22, 2020
@voidzcy
Copy link
Collaborator

voidzcy commented Oct 2, 2020

I am able to reproduce the bug locally. There are two issues involved here:

The way proto files being packaged does not preserve the directory structure.

// Add proto files to java resources, and package them into aar/classes.jar.
it.resources.srcDirs += generateProtoTask.sourceFiles

In order to preserve the original directory hierarchy, this should be achieved by the variant's ProcessResourceTask (which is a Copy task that preserves the directory hierarchy). This should be similar to what's being done for Java projects:

Task processResourcesTask = variant.getProcessJavaResources()
processResourcesTask.from(generateProtoTask.sourceFiles) {
    include '**/*.proto'
}

variant.sourceSets.each {
    Task extractTask = setupExtractProtosTask(generateProtoTask, it.name)
    processResourcesTask.dependsOn(extractTask)
}

The more critical problem is in the proto extraction side. The plugin is not able to extract proto files from .aar:

} else if (file.path.endsWith('.jar') || file.path.endsWith('.zip')) {
FileTree zipTree = archiveActionFacade.zipTree(file.path)
copyActionFacade.copy { spec ->
spec.includeEmptyDirs = false
spec.from(zipTree) {
include '**/*.proto'
}
spec.into(destDir)
}
} else if (file.path.endsWith('.tar')
|| file.path.endsWith('.tar.gz')
|| file.path.endsWith('.tar.bz2')
|| file.path.endsWith('.tgz')) {
FileTree tarTree = archiveActionFacade.tarTree(file.path)
copyActionFacade.copy { spec ->
spec.includeEmptyDirs = false
spec.from(tarTree) {
include '**/*.proto'
}
spec.into(destDir)
}
} else {
logger.debug "Skipping unsupported file type (${file.path}); handles only jar, tar, tar.gz, tar.bz2 & tgz"
}

It turns out the test added by #414 does not test the real thing. Depending on a subproject does not go through the packaging process, dependent classes and resources are pulled directly from the subproject. Also, the test is weak as it only covers the consumption of generated classes. The application's protos are not consuming protos in the library project, which is the more important behavior we want to see.

Some other existing tests (e.g., the library-application test for Java projects) may also not test the desired behavior, and the test itself is also weak.

I am probably going to revert #414 and fix this bug.

@robcos
Copy link

robcos commented Oct 7, 2020

I'm piggybacking on this issue as I think what I see is related.
With 0.8.13 I can normally see the .protos and, as said in this bug, they are stripped from the path, which is wrong.

However, sometimes (and it seems to be consistent), when I build a full app, the res.jar with the protos is not generated at all. I can't share full logs but maybe the attached screenshot can help understand what is going on.

Build A is a good one, Build B is a bad one. During Build B, there is no res.jar in the build folder.
As you can see the :libs:proto:processDebugJavaRes has different value properties.
The next step bundleLibResDebug has no value for the resources and resourcesAsJars in build B:

Screenshot 2020-10-07 at 15 38 12

@robcos
Copy link

robcos commented Oct 16, 2020

Is there anything I can do to help this bug moving forward? It is preventing my company (and anyone else using gradle I guess!) to really enjoy the power of protos. You have no idea of the hacks we have to go through otherwise :-)

@nydrani
Copy link
Author

nydrani commented Oct 16, 2020

Is there anything I can do to help this bug moving forward? It is preventing my company (and anyone else using gradle I guess!) to really enjoy the power of protos. You have no idea of the hacks we have to go through otherwise :-)

The workaround I used was to build a java library (.jar) instead of an android library (.aar).

@robcos
Copy link

robcos commented Oct 16, 2020

I'm actually seeing this issue with jars. Am I doing something wrong?

@voidzcy
Copy link
Collaborator

voidzcy commented Oct 16, 2020

I'm actually seeing this issue with jars. Am I doing something wrong?

Non-Android jars shouldn't have problems. But it will not work if you are building from an android project. Packaging side seems to be easy to solve as I mentioned above, we just need to use the ProcessJavaResources task. However, I am stuck on extracting protos from Aar dependencies. Gradle/AGP has some dependency transform process that transforms artifacts containing Aar to something else, and that cannot be seen from the configuration we used to get proto files. Probably missing some attributes for that configuration. Currently I don't have much time digging into it, so the fix is slowed down. I may get to it in next a few weeks.


Update: seems I tweaked the configuration in a wrong way in my local experiment such that no dependency variant can be matched. The existing attribute setting should still work. I've fixed the aar proto packaging and extracting issue in #440 and tested with my local installation (use grpc-android with some proto definition and consume it in a helloworld project).

@voidzcy
Copy link
Collaborator

voidzcy commented Nov 18, 2020

The fix is in 0.8.14 release now.

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

Successfully merging a pull request may close this issue.

3 participants