Should deprecate usage of configuring generated file directory

[voidzcy]

Allowing configuring output directory for generated files is problematic, as it makes managing generated files harder.

Each run of generateProtoTask should always produce the just-right set of generated files for the given proto files. That is, unwanted files in generated files directory from previous build should be deleted (see #331 ).

However, if we allow configuring output directory for generated files, we cannot easily wipe out the output directory before running generateProtoTask. So this usage should be deprecated. We would either ask users to do a Sync/Copy task to bring generated files to their desired directory or we may support that by adding a Copy task to do so for users.

[ejona86]

Nit: Users may want a Copy task instead. Sync task wipes out the destination before copying.

[devinrsmith]

I don't think that configurability is actually the problem - I think the internal plugin logic should be able to handle it just fine - it should just run a delete on the target directory before it invokes the code generation.

Note: I've run into this issue and worked around it by hacking in a manual delete. See #331 (comment)

[ejona86]

it should just run a delete on the target directory before it invokes the code generation.

We know some users were dumping the generated code into directories containing other things. We don't want to go around deleting directories that we didn't create.

[devinrsmith]

Ah - okay. That seems like it's abusing the nature of the gradle build system / caching - but it's also not ideal to make a change that would delete unsuspecting users' files.

Maybe the best way to go about it is to force the user to specify if they want the output directory deleted only if they have changed the default output directory.

So:

protobuf {
  generatedFilesBaseDir = "$projectDir/src/my-custom-dir"
}

would cause a build error with a helpful message.

The correct configuration would be:

protobuf {
  generatedFilesBaseDir = "$projectDir/src/my-custom-dir"
  deleteGeneratedFilesBeforeCompile = (true|false)
}

[voidzcy]

This raises an issue:

If the user does not specify generatedFilesBaseDir in protobuf closure, we will use the default directory. Then we will do a clean compile to wipe off pre-existing files. This is what we want to do for not allowing users to configure generated file directory. But for the case generatedFilesBaseDir is overridden while deleteGeneratedFilesBeforeCompile is not specified. We probably want to default it to false to avoid surprising users by deleting their own thing. But this may still surprise users as they will see different behaviors with only the change of generatedFilesBaseDir.

[devinrsmith]

IMO, having no default and throwing an error when generatedFilesBaseDir is set and deleteGeneratedFilesBeforeCompile is unset seems like the most explicit way to be safe - it forces the users to be more explicit about their intentions when they are doing things in a non-standard way. Yes it will break existing non-standard configurations, but 1) it won't delete anything it's not supposed to, and 2) it allows non-standard configurations to choose the "correct" way of doing things instead of silently defaulting to false.

Edit: 3) If a new user uses the default configurations (w/ default true), and then switches to a non-standard configuration (w/ default false), they won't be aware that their build might now be incorrect.

[voidzcy]

After offline discussion with @zhangkun83, we think we should keep generatedFilesBaseDir internal and users are not expected to touch things under this directory. For easier maintainability of generated files, users should not mix their own files into the directory for generated files.

If users want to add some extra files into compilation, they can already do that with sourceSet closure without the necessity to put files together. For other reasons such as packaging, users can add a Copy task to copy generated files from the output directory of generateProtoTask.

Is there any specific reason/use case you want to change the default generatedFilesBaseDir and mix with your own files?

[devinrsmith]

I don't have any particular use-case for changing generatedFilesBaseDir atm, so it does seem like keeping it internal is also a good solution (w/ an appropriate error when users try to set it, mainly useful for those upgrading plugin versions running w/ custom base dir). Sorry for the extra noise on this issue!

[devinrsmith]

Likely will be able to fix #180 after fixing this issue.

[oehme]

We know some users were dumping the generated code into directories containing other things. We don't want to go around deleting directories that we didn't create.

That's the user's fault and they simply shouldn't do this. Please don't take away configurability just "because people might abuse it". Pretty much every Gradle task allows configuring its output folder and those tasks also delete their output (with a few exceptions like the Copy task). The few users who set the output folder to something that shouldn't be deleted will notice this once, do a git reset and then change their build. No big deal.

Configurability is important because you don't know how this plugin might be integrated into others. For example, the Java plugin and Android plugin each use very different folder structures. I'm sure there are more layouts out there, e.g. for Javascript projects etc. The protobuf plugin should not prescribe how they set up their output structure.

[doctorpangloss]

Just solve the caching problem.

In retrospect this is very hard due to limitations of protoc.

[StefanHISE]

As far as I understand there's basically kind of an agreement that it's fine to dump the outdir to solve the caching issue. Any chance for this to get out in one of the next builds? We just stumbled about it ourselves and now use a custom build, applying #347 to solve the issue, which really cost us quite some time for multiple developers already, which weren't aware of this plugin's behavior with keeping outdated generated files behind after switching branches.

[rougsig]

IMO: The user is not interested in where the generated files are. If everything compiles, that's all the user needs. For kotlin/java we can easily remove generateFilesBaseDir. To work correctly with gradle, the user should not overlap the output between tasks. Learn more about overlapping outputs.

Open questions:

1. What about generatedFilesBaseDir configuration option for python/dart/js? For java/kotlin/android/kmm we have official gradle plugins and we can support sourcesets from them. But for python/dart/js we don't have them.
2. Why do we have support for platform ​​other than java/kotlin/android?

---

My plan is, two PRs.

1. For minor release.
   Deprecate generateFilesBaseDir configuration option.
   That configure option will be removed in major release
2. For major release.
   Remove generateFilesBaseDir configuration option.
   Move generate output location to other directory.
   Make generateProtoTask always generate the correct content in the output directory.
   Make Gradle UP-TO-DATE checks work correctly. (generateProto always up-to-date #180)

[ejona86]

Why do we have support for platform ​​other than java/kotlin/android?

We've seen a surprising number of people using the plugin for other languages. I think they mainly do Java or the like, but still interact with other languages so having everything in one build system is convenient.

[doctorpangloss]

Directories are valid outputs for protoc and plugin generators, file lists are not.

Make generateProtoTask always generate the correct content in the output directory.

You will have to read the output files, in a language appropriate way, to know which were generated by protoc. They are annotated. But so much hassle.

deleteGeneratedFilesBeforeCompile = (true|false)

Only knowable by parsing the output files in the destination directory.

Ultimately protoc output is not cacheable when targeting a directory with pre-existing files. No guarantees its output is annotated, so you'd never know what is user versus protoc generated content. Underlying flaw with checking in generated code. That said if you raised the issue with protobufs team they'll say you shouldn't check in generated code anyway. I appreciate the attention to this issue.

[rougsig]

@doctorpangloss

You will have to read the output files, in a language appropriate way, to know which were generated by protoc. They are annotated. But so much hassle.

We should use different output directories for different gradle tasks. If more than one gradle task write to one directory we will have overlapping outputs, as result not cacheable status for tasks. (more details)

If we have different output directories, we can easily check the input and output of the task if they have changed - just run to generate new code. Gradle provides a great API for this.

[doctorpangloss]

We should use different output directories for different gradle tasks.

This is a good idea, but I am not sure how to prevent the user from choosing a directory with pre-existing files.

It seems appealing to let users still specify the directory, because it is really high value to let the project check in generated files. Imagine something like jitpack running on arm and there's a missing protoc artifact - you'd be trading one pain for another.

[StefanHISE]

How about to make it configurable it two modes:
Mode 1: keep as is, single output directory with the documented caveats of this particular issue with that
Mode 2: use distinct output directories with the note that the output directory will also be pruned by gradle (to solve this and related particular issues)

At least for us this would be a feasible way to go and get this issue which has been bothering us for the pasts months resolved.