Restrict getOutputSourceDirectorySet() to directories only #523

cilki · 2021-09-18T04:11:30Z

#480 allows outputPath to be a zip/jar file, but it's not excluded from the SourceDirectorySet in that case. Therefore, when the source set gets evaluated, it leads to this exception in Gradle 7.2:

Caused by: org.gradle.api.InvalidUserDataException: Source directory '<redacted>/build/generated-proto/main/cpp.zip' is not a directory.
	at org.gradle.api.internal.file.DefaultSourceDirectorySet.getSourceTrees(DefaultSourceDirectorySet.java:273)
	at org.gradle.api.internal.file.DefaultSourceDirectorySet.getSrcDirTrees(DefaultSourceDirectorySet.java:256)
	at org.gradle.api.internal.file.DefaultSourceDirectorySet.getSrcDirs(DefaultSourceDirectorySet.java:114)
	at jdk.internal.reflect.GeneratedMethodAccessor2253.invoke(Unknown Source)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at com.google.protobuf.gradle.ProtobufPlugin$_addSourcesToIde_closure31.doCall(ProtobufPlugin.groovy:554)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at com.google.protobuf.gradle.ProtobufPlugin.addSourcesToIde(ProtobufPlugin.groovy:553)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at com.google.protobuf.gradle.ProtobufPlugin$_doApply_closure5.doCall(ProtobufPlugin.groovy:159)
...

The fix is to just ignore files when creating the source directory for generated output.

google-cla · 2021-09-18T04:11:36Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

cilki · 2021-09-18T04:13:53Z

@googlebot I signed it!

ejona86

I wonder if it was a mistake to do #480, like it was a mistake to have generatedFilesDir. But it looks like this is just for the IDE, and yeah, that shouldn't break.

cilki · 2021-09-20T23:40:19Z

I like the idea of outputting directly to a Zip file, but it seems strange to do it with this method:

protobuf-gradle-plugin/src/main/groovy/com/google/protobuf/gradle/GenerateProtoTask.groovy

Line 509 in 8a35d46

void setOutputSubDir(String outputSubDir) {

Although it does work perfectly after the change in this PR.

ejona86 · 2021-09-22T16:05:15Z

Arg. We're bitten by the travis-ci.org migration. I'm a bit surprised that it only now started failing, but it was still working in August, even though it is claimed to be shut off in June.

voidzcy · 2021-09-23T00:49:09Z

I wonder if it was a mistake to do #480, like it was a mistake to have generatedFilesDir. But it looks like this is just for the IDE, and yeah, that shouldn't break.

It is a bit awkward to support such a usage, as packaging generated code into JAR/ZIP doesn't seem to be something that should/can be done together with source compilation (the primary goal of this plugin is to automate compilation). It would be best if users do this by themselves separately.

Many users like in #478, they use this plugin simply as a management tool for proto and generated code (mostly for languages other than Java family). As supporting this doesn't do harm to existing functionalities, I accepted it.

voidzcy · 2021-09-23T00:54:45Z

@ejona86 Could you help fix Travis? Looks it requires permission from google organization.

ejona86 · 2021-09-23T17:23:50Z

@voidzcy, I can't fix it right now. Instead of migrating to travis-ci.com we've generally been migrating off of travis, like to github actions. That isn't that hard, but isn't "do it in 5 minutes."

ejona86 · 2021-09-24T21:24:13Z

Sent out #525 for CI migration.

cilki · 2021-11-04T05:32:16Z

Anything else I can do to push this one through?

ejona86 · 2021-11-05T18:38:03Z

@cilki, there were test failures. The failures are in ProtobufJavaPluginTest, so shouldn't be too hard for you to reproduce and take a look. If the failure doesn't make much sense I can help take a look, but it doesn't seem like a flake at first glance.

cilki · 2021-11-05T18:40:43Z

@ejona86 I'll give it a go. The tests were failing for me before I made any changes, so I'll investigate that first.

ejona86 · 2021-11-05T18:49:49Z

@cilki, the Android tests and such can be more annoying, but I thought that one failing test should be easier. If no progress, just report back.

travis-ci.org is dead and travis-ci.com has no free tier, which means it isn't great for users to test on their own. GH Actions seems a better fit.

This improves the 'testing' GitHub actions workflow in a number of ways: - Use 'gradle-build-action' to invoke Gradle, to optimize caching of Gradle User Home between jobs - Run each test build and the codenarc builds in parallel

- Set a 'CI' env var when running on GitHub actions, and use this to create txt codenarc reports. - Remove some special handling for Travis that is not required on GitHub actions. - Remove the Travis status flag from README.

Upgradle to Gradle 6.9.1 for new Kotlin DSL capabilities. Use better dependency declaration between Groovy/Java. Remove environment variables for action: - CI is set to true in Github action runs anyway. - The memory settings shouldn't be necessary any more.

Empty directories should not matter for source files, so let's ignore them for up-to-date checks as well. This avoids two issues: * Right now, the task will be out-of-date when you add an empty directory to the source folder. There will also be a build cache miss if the only difference between the sources is an empty directory somewhere. * In Gradle 8.0, the task won't skip any more if there are only empty directories in the source folder and no actual files. That is because the behaviour for skipping when empty and up-to-date checking will be more consistent.

google-cla · 2021-11-06T15:51:23Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2021-11-06T15:53:25Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

cilki · 2021-11-06T16:04:06Z

Indeed some of the test failures were my fault for using Java 11 instead of Java 8. I fixed the actual issue, but rebased in the meantime which seems to have messed some things up. I'll make a new PR.

Restrict getOutputSourceDirectorySet() to directories only

49d25f8

ejona86 approved these changes Sep 20, 2021

View reviewed changes

voidzcy approved these changes Sep 23, 2021

View reviewed changes

voidzcy closed this Sep 23, 2021

voidzcy reopened this Sep 23, 2021

voidzcy closed this Sep 28, 2021

voidzcy reopened this Sep 28, 2021

ejona86 mentioned this pull request Nov 5, 2021

sourceFiles should ignore empty dirs #530

Merged

fml2 and others added 7 commits November 6, 2021 10:51

doc: fix typo in README

1733624

Replace Travis with GH Actions (#525)

9677d8f

travis-ci.org is dead and travis-ci.com has no free tier, which means it isn't great for users to test on their own. GH Actions seems a better fit.

Switch to use gradle-build-action

072edb2

This improves the 'testing' GitHub actions workflow in a number of ways: - Use 'gradle-build-action' to invoke Gradle, to optimize caching of Gradle User Home between jobs - Run each test build and the codenarc builds in parallel

Remove last vestige of Travis CI

23155ac

- Set a 'CI' env var when running on GitHub actions, and use this to create txt codenarc reports. - Remove some special handling for Travis that is not required on GitHub actions. - Remove the Travis status flag from README.

Check filename instead of type since the dir may not exist yet

9011d43

Merge branch 'google:master' into fix_get_output_source_directory_set

64d02fb

cilki closed this Nov 6, 2021

cilki deleted the fix_get_output_source_directory_set branch November 6, 2021 16:00

cilki mentioned this pull request Nov 6, 2021

Restrict getOutputSourceDirectorySet() to directories only #533

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restrict getOutputSourceDirectorySet() to directories only #523

Restrict getOutputSourceDirectorySet() to directories only #523

cilki commented Sep 18, 2021

google-cla bot commented Sep 18, 2021

cilki commented Sep 18, 2021

ejona86 left a comment •

edited

Loading

cilki commented Sep 20, 2021

ejona86 commented Sep 22, 2021

voidzcy commented Sep 23, 2021

voidzcy commented Sep 23, 2021 •

edited

Loading

ejona86 commented Sep 23, 2021

ejona86 commented Sep 24, 2021

cilki commented Nov 4, 2021

ejona86 commented Nov 5, 2021

cilki commented Nov 5, 2021

ejona86 commented Nov 5, 2021

google-cla bot commented Nov 6, 2021

google-cla bot commented Nov 6, 2021

cilki commented Nov 6, 2021

Restrict getOutputSourceDirectorySet() to directories only #523

Restrict getOutputSourceDirectorySet() to directories only #523

Conversation

cilki commented Sep 18, 2021

google-cla bot commented Sep 18, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

cilki commented Sep 18, 2021

ejona86 left a comment • edited Loading

Choose a reason for hiding this comment

cilki commented Sep 20, 2021

ejona86 commented Sep 22, 2021

voidzcy commented Sep 23, 2021

voidzcy commented Sep 23, 2021 • edited Loading

ejona86 commented Sep 23, 2021

ejona86 commented Sep 24, 2021

cilki commented Nov 4, 2021

ejona86 commented Nov 5, 2021

cilki commented Nov 5, 2021

ejona86 commented Nov 5, 2021

google-cla bot commented Nov 6, 2021

google-cla bot commented Nov 6, 2021

cilki commented Nov 6, 2021

ejona86 left a comment •

edited

Loading

voidzcy commented Sep 23, 2021 •

edited

Loading