Deprecate FlatPackage as a GeneratorOption.

[plaflamme]

This will trigger on imported files as well, which will break if a related runtime was not compiled with this option enabled. Since this is now supported as a package-level option, users can simply move to use that instead for compatibility.

[plaflamme]

@thesamet is there any additional thing that should be done to get this in?

[thesamet]

Before deprecating, I wanted to introduce a generator parameter that allows setting flat_package only under a certain package, so this option is not assumed for imported protos.

[plaflamme]

Ok, I understood that users can simply define this for their own packages using scalapb.options and this shouldn't be used anymore.

What's the rationale for also supporting this via a generator parameter? From my own experience, having compiler parameters that alter the generated package / class names creates more friction than is actually worth.

One problem this creates is for custom plugins that rely on the scalapb generated classes. You end up having to share the compiler options between them which isn't necessary if the package / naming information is self-contained in the .proto files (which is usually the case with option java_package = "..." and such).

[thesamet]

There have been user feedback on preferring generator options passed through SBT, as opposed to adding a file that depends on scalapb.proto. I would anticipate more of this feedback when deprecating flat package since it's fairly popular option. The rationale you provided is spot on - that has been my reasoning for not continuing to add generator options. I haven't fully landed on how to proceed with this. A more drastic change could be to set flat_package to be on by default, which many users prefer it this way. That of course would also lead to inconvenience for others.

[plaflamme]

Sounds like the end state should be one where flat_package is true by default and there are no generator-level option that allows changing the naming / structure of generated code.

Perhaps this deprecation cycle could work (each top-level item is a scalapb release):

1. Make "flat package" style the default
   1. introduce LegacyPackageStyle generator option and legacy_package_style proto option to get the original behaviour back
   2. deprecate FlatPackage and flat_package (doesn't do anything anyway)
2. Deprecate LegacyPackageStyle
   1. remove FlatPackage and flat_package
3. Remove LegacyPackageStyle generator option (desired end-state is attained here)
4. Remove legacy_package_style proto option (optional step, but the option doesn't seem useful)

The reason for introducing a new generator option and then removing it is to make the transition easier for people that have a large codebase: they would simply have to turn on the generator option once to get the previous behaviour back. Then using the new proto option would allow to move over to the new style on a per-file / package basis. For example, a user could go through these steps:

1. Bump scalapb add LegacyPackageStyle option
2. Add legacy_package_style: true to all files
3. Remove LegacyPackageStyle option
4. for each proto file
   1. remove legacy_package_style
   2. fix compiler / naming issues

Obviously, users could go directly to adding the proto-level option, but that seems like a lot to ask given that the user was previously using the default behaviour.

[EDIT]: to fix markdown rendering

[bjaglin]

Bumping this interesting topic to see if/how we can move on. As a user of akka-grpc, which enables flat_package by default, I feel the pain of not being able to use https://github.com/scalapb/common-protos out of the box (although enabling it selectively at the package level helped for a smooth transition).

1. I have tried packaging https://github.com/scalapb/common-protos with both flat_package=true and flat_package=false (with https://github.com/scalapb/protoc-bridge/pull/158) in the same JAR as a POC, and it compiles (except for proto-google-common-protos where Java classes seem to conflict when flat_package=true alone is set). That would not work everywhere obviously, as they might be conflicts. Still, I was wondering if we could have a ScalaPB generator flag flat_package_aliases that would generate classes with flat_package=false but creating type aliases corresponding to the types expected with flat_package=true. I haven't gone so far as trying so there might be challenges along the way...
2. If we were to change the default to flat_package=true, I believe it would be possible to write a Scalafix rule that would detect references to flat_package=false stubs and update them to respect the flat_package=true signatures. Obviously, that assumes that third party stubs are also available, so it would need (1). But given that scalafix has a good integration with the major build tools (and scala-steward), that would greatly reduce the cost of that breaking change.

[bjaglin]

Before deprecating, I wanted to introduce a generator parameter that allows setting flat_package only under a certain package, so this option is not assumed for imported protos.

Could flat_package (the generator option) apply only to sources (no matter what package they are in)?

[thesamet]

Could flat_package (the generator option) applies only to sources (no matter what package they are in)?

If we do that, some projects may be importing third-party protos and internal protos from other sub-projects, with the expectation that internal ones are flat-packaged but externals aren't. With this suggestion, how would they control how imported protos are interpreted?

[bjaglin]

With this suggestion, how would they control how imported protos are interpreted?

Indeed, package-wide settings are much easier to handle than artifact/project-wide ones...

Giving it a try, although it looks quite complex: local projects or JAR exposing protos with already compiled ScalaPB stubs could expose the ScalaPB generator options that were used for compilation, so that downstream projects can themselves pass them as generator options, using include paths (as opposed to packages).

I think Ivy extra attributes could be used to store this information in remote JARs, using the version-less generator artifact (as captured in SandboxedJvmGenerator) as key so that clients like sbt-protoc don't need to do adhoc handling of ScalaPB. By changing the unpack mechanism to extract each dependency in their own directory and copying local project sources (to have a stable, machine-independant identifier), it could look like this with sbt-protoc for example:

protoc
--plugin=protoc-gen-jvm_0=/tmp/protocbridge866750902096298528
--jvm_0_out=/path/to/app/target/scala-2.13/src_managed
--jvm_0_opt=no_lenses,flat_package,include_options=XXXXXXXX
-I/localartifact1/target/protobuf_external/remoteartifact1
-I/localartifact1/target/protobuf_external_src
-I/localartifact2/target/protobuf_external/remoteartifact2
-I/localartifact2/target/protobuf_external_src
-I/app/src/main/protobuf
-I/app/src/main/protobuf/target/protobuf_external/artifact3
-I/app/src/main/protobuf/target/protobuf_external_src
-I/app/src/main/protobuf/target/protobuf_internal/localartifact1
-I/app/src/main/protobuf/target/protobuf_internal/localartifact2
...

with XXXX being a proper encoding of, for example:

remoteartifact1=
remoteartifact2=flat_package
remoteartifact3=no_lenses
localartifact1=no_lenses
localartifact2=flat_package

ScalaPB plugins could get the same treatment, provided sbt-protoc detects the ScalaPB compiler transitively included in their SandboxedJvmGenerator (and thus lookup the right attributes from Ivy or options from the relevant target in upstream local projects).

[thesamet]

A simpler approach could if the external jars just throw in a proto file with package-scoped options for what they provide, and the end-user's project that ensures those protos are imported. Maybe the auto-import can be automated by sbt-protoc through convention on that proto file name within the external jar.

[plaflamme]

The approach we take internally is to put a pkg.proto file in each package which contains the package-wide options. This works well locally for that project because the compiler pulls in those files, but it somewhat breaks down when importing from another project since it only needs to import files it actually uses.

For example, say project A uses this layout:

src/main/protobuf
  `- my_package
      |- pkg.proto
      `- model.proto

Then if project B does import my_package.model.proto then pkg.proto isn't visible to scalapb and thus the package-level options aren't visible.

I suppose a solution to this is to always import pkg.proto, but then you get warnings about unused imports which is pretty annoying.

The alternative we've been using in some cases is to use file-level options, but this requires copy-pasting which is suboptimal as well...

So if we want to inject the scalapb options, it'd to be in every file to deal with imports like shown here.

[bjaglin]

A simpler approach could if the external jars just throw in a proto file with package-scoped options for what they provide, and the end-user's project that ensures those protos are imported. Maybe the auto-import can be automated by sbt-protoc through convention on that proto file name within the external jar.

I saw that you implemented this on commons-proto & sbt-protoc :christmas_tree:

I took the liberty to try it out from master and I think there is a corner case to handle: when the same JAR (pgv-proto-scalapb below) is unpacked both by a module (app below) and one of its dependency (lib below):

/app/target/protobuf_external/validate/scalapb-options.proto: Input is shadowed in the --proto_path by "/lib/target/protobuf_external/validate/scalapb-options.proto".  Either use the latter file as your input or reorder the --proto_path so that the former file's location comes first.

I guess the recommendation would work as a simple fix?