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

Gradle build: Upgrade protobuf Gradle plugin (for Gradle 8, Java 20) #3059

Conversation

msgilligan
Copy link
Member

@msgilligan msgilligan commented Apr 22, 2023

  • Upgrade protobuf-javalite to version 3.22.3
  • Upgrade Gradle protobuf plugin to version 0.9.4 (if Gradle 7 or later)
  • Remove generated ProtoBuf files, they're now produced in build/generated/...

@schildbach
Copy link
Member

Can we keep the version upgrade separate from the rest?

There were good reasons against generating the protobufs on the fly, if I only could remember.

@schildbach
Copy link
Member

Related: 002fe81 and 6e85ea3

@msgilligan
Copy link
Member Author

Can we keep the version upgrade separate from the rest?

I think so. Let's merge #3058 and then I'll try breaking this one down to the minimum functionality to get Gradle 8 working.

There were good reasons against generating the protobufs on the fly, if I only could remember.

Something seemed to break when I upgraded either protobuf itself or the plugin (I started getting duplicate Proto.java files) -- so I just deleted the build.gradle line that set the directory and that seemed to resolve the issue. I can investigate further...

@msgilligan
Copy link
Member Author

There were good reasons against generating the protobufs on the fly, if I only could remember.

6e85ea3 says "too much support hassle." Does that ring a bell?

@schildbach
Copy link
Member

I picked one of the version upgrades.

@schildbach
Copy link
Member

I've updated the plugin to 0.9.1. 0.9.2 seems to have some regression regarding generatedFilesBaseDir:

/home/aschildbach/dev/workspace/bitcoinj/core/build/generated/source/proto/main/java/org/bitcoin/protocols/payments/Protos.java:6: error: duplicate class: org.bitcoin.protocols.payments.Protos
public final class Protos {
             ^
/home/aschildbach/dev/workspace/bitcoinj/core/build/generated/source/proto/main/java/org/bitcoinj/wallet/Protos.java:6: error: duplicate class: org.bitcoinj.wallet.Protos
public final class Protos {
             ^

@msgilligan
Copy link
Member Author

Can we keep the version upgrade separate from the rest?

Apparently not. It looks like the Gradle Plug-in has dropped support for specifying the (complete) output directory. (and without the courtesy of giving an error when the removed option is used)

@msgilligan
Copy link
Member Author

I've updated the plugin to 0.9.1. 0.9.2 seems to have some regression regarding generatedFilesBaseDir:


/home/aschildbach/dev/workspace/bitcoinj/core/build/generated/source/proto/main/java/org/bitcoin/protocols/payments/Protos.java:6: error: duplicate class: org.bitcoin.protocols.payments.Protos

public final class Protos {

             ^

/home/aschildbach/dev/workspace/bitcoinj/core/build/generated/source/proto/main/java/org/bitcoinj/wallet/Protos.java:6: error: duplicate class: org.bitcoinj.wallet.Protos

public final class Protos {

             ^

See google/protobuf-gradle-plugin#636

@schildbach
Copy link
Member

schildbach commented Apr 23, 2023

So to clarify: there is no way with 0.8.2 to keep the existing behaviour to generate the protobuf files within src/?

If that is true, is there a way to use a new special directory, which is not part of src/ but not within build/ either? Somewhere where it can remain under version control.

And also, what are the downsides of staying with 0.8.1 for as long as this issue is resolved by the plugin (which I assume should be resolved; our configuration used to be very common years ago)?

@msgilligan
Copy link
Member Author

So to clarify: there is no way with 0.8.2 to keep the existing behaviour to generate the protobuf files within src/?

I don't know if there is no way. I have very little knowledge of protobuf and the protobuf Gradle plugin. I only tinker with it when I'm trying to upgrade Gradle. I also assume you mean 0.9.2 above, right?

@schildbach
Copy link
Member

I also assume you mean 0.9.2 above, right?

Yes, sorry.

@msgilligan msgilligan force-pushed the msgilligan-protobuf-javalite-3.22.3 branch from adcb5de to dedd20e Compare April 24, 2023 19:17
@msgilligan
Copy link
Member Author

Rebased and force-pushed. (This still combines upgrading the plugin, removing the generated files from Git, and removing the deprecated generatedFilesBaseDir property, but now reflects that we've upgraded the protobuf library itself.)

@ejona86
Copy link

ejona86 commented May 5, 2023

So to clarify: there is no way with 0.8.2 to keep the existing behaviour to generate the protobuf files within src/?

In 0.9.2 the plugin will always use the default directory. But if you specify generatedFilesBaseDir, then it will copy the generated files into that directory afterward. generatedFilesBaseDir is deprecated because it doesn't handle deletes properly, because it can't distinguish between "old generated code" and "your artisan source files."

@msgilligan msgilligan force-pushed the msgilligan-protobuf-javalite-3.22.3 branch from dedd20e to 703e06b Compare May 26, 2023 00:14
@msgilligan
Copy link
Member Author

...is there a way to use a new special directory, which is not part of src/ but not within build/ either? Somewhere where it can remain under version control.

I think we could make it a "source set" in Gradle and put it in core/src/protobufGenerated or something similar. The first step in doing that would be to move the generated files to a dedicated package (or packages) -- see PR #3101.

@msgilligan
Copy link
Member Author

@schildbach I have rebased and force-pushed.

I think we should merge #3101 to move the generated Java classes to org.bitcoinj.protobuf (and not create a new source set) and then rebase and merge this PR.

Given that you don't remember the "good reason" for checking in the generated source, and that I recall you saying elsewhere that you "weren't opposed" to removing it, I think we should go ahead and remove it. If we find a problem we can revert the change or try a different approach (maybe a source-set)

@msgilligan
Copy link
Member Author

@ejona86 I forgot to thank you for your comment until now. Thank you!

…d Protos files

* Upgrade Gradle protobuf plugin to version 0.9.4 (if Gradle 7 or later)
* Remove deprecated `generatedBaseFileDir` setting, default to build/generated/...
@msgilligan msgilligan force-pushed the msgilligan-protobuf-javalite-3.22.3 branch from 703e06b to 1d245bb Compare September 2, 2023 00:15
@msgilligan
Copy link
Member Author

@schildbach OK, this is rebased to upgrade to the latest Protobuf plugin and to remove the deprecated generateFilesBaseDir setting.

@msgilligan msgilligan changed the title Gradle build: Upgrade protobuf and protobuf Gradle plugin (for Gradle 8, Java 20) Gradle build: Upgrade protobuf Gradle plugin (for Gradle 8, Java 20) Sep 2, 2023
@schildbach
Copy link
Member

Merged.

@schildbach schildbach closed this Sep 2, 2023
@msgilligan msgilligan mentioned this pull request Sep 16, 2023
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.

3 participants