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

Fix protobuf gradle protobuf dependencies #133

Merged

Conversation

berksean
Copy link
Contributor

Addresses issue 132

Fix buf tasks behavior with more recent versions of the protobuf-gradle-plugin. With version 0.9.3 (and possibly all 0.9.* versions), the buf plugin tasks no longer work when protobuf dependencies are used (as described in the issue). This change filters the hard-coded "extracted" directories from the list of proto source directories set by the protobuf gradle plugin.

Tested by:

  • Updating the test protobuf gradle plugin version to 0.9.3.
  • Verifying tests fail when using protobuf dependencies.
  • Adding the extracted directory filter and verifying tests begin to pass.

@CLAassistant
Copy link

CLAassistant commented May 24, 2023

CLA assistant check
All committers have signed the CLA.

@berksean berksean force-pushed the fix-protobuf-gradle-protobuf-dependencies branch from 8a2d430 to 37f73de Compare May 24, 2023 19:38
@berksean
Copy link
Contributor Author

Note - Several android specific unit tests failed (suffixed with android library plugin and kotlin android plugin) indicating that the ANDROID_HOME environment variable was unset. I imagine this is because I don't have android studio installed/set up. If there some instructions on what tools are necessary somewhere, I can verify those tests are passing as well.

berksean added 6 commits May 25, 2023 16:31
- Standardize on method naming and nomenclature for source set directory lists vs. general directory lists.
- Add some comments to make extra clear what helpers return and why.
Copy link
Collaborator

@andrewparmet andrewparmet left a comment

Choose a reason for hiding this comment

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

Looks great. A few small nits.

@berksean
Copy link
Contributor Author

Random question that is unrelated -- If there are other potential issues/feature requests, do you/buf prefer starting a conversation in slack, as an issue, or some other means?

Two things that are in my queue:

  • There appears to be some misbehavior is some (not yet reproduce-able) situation where gradle's stale outputs detection will delete the contents of the bufbuild directory prior to running tasks like bufBuild (for bufBreaking -- deletion happens as part of the copy config task). I'm still fairly new to gradle, but it appears to be related to not declaring outputs of certain tasks.
  • For bufBreaking, how to handle reverts in CICD, since theoretically a revert might be incompatible with the previously published artifact.

@andrewparmet
Copy link
Collaborator

do you/buf prefer starting a conversation in slack

I'm happy with an issue since that's what I get notified about and it's a better public record. However I am not on the Buf team and they may have a different preference. cc @buildbreaker

gradle's stale outputs detection will delete the contents of the bufbuild directory

Sounds nasty. I started some work a while ago on output and input declarations but never gave it a ton of attention. It's effectively lost, so there's no need to worry about duplicated work if you decide to take it on. If you achieve reproduction with a minimal project, please file an issue!

handle reverts in CICD

I have set up a "workflow" in my organization where you disable the Buf check with checkSchemaAgainstLatestRelease = false for one minimal PR with just the breaking change, and then re-enable it in the very next PR. We have tooling that looks for that line in the buildscript and if it sees checkSchemaAgainstLatestRelease = false (or if it is absent), the only PR it allows to follow is one that re-enables the check so that no other breaking changes can sneak through. This check is tightly coupled to our build system, so I didn't write it into the plugin. I'm open to exploring a solution here.

Copy link
Contributor

@buildbreaker buildbreaker left a comment

Choose a reason for hiding this comment

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

do you/buf prefer starting a conversation in slack

Just like what @andrewparmet said, you're welcome to create an issue here to start a conversation if that is your preference :). If you want to join our slack and have a conversation there, that works as well!

Thank you @berksean for your contribution!

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

Successfully merging this pull request may close these issues.

Lint/Format fails with protobuf-gradle-plugin protobuf dependencies
4 participants