Closed barakmich closed 2 years ago
Hmm. I'm not sure if we should forward all of tonic
's feature flags...instead, an application can just add its own tonic
dependency and enable that feature flag, and it will be enabled for console-api
as well.
I agree that the "compression" feature is often useful and should typically be enabled, but if we start adding feature flags to enable specific tonic
features, the question is...where do we stop? In general, I think it only makes sense for a crate to have feature flags for a feature of a dependency when additional code in that crate is required to support the dependency's feature. Otherwise, it makes more sense for users who need a feature enabled on a transitive dependency to enable those crates features via their own dependency.
On the other hand, the console-subscriber
crate maybe should enable the "compression" feature by default, because it actually just spins up an entire server on its own...and tokio-console
, the client, should definitely enable "compression", so that it can talk to servers which enable compression. But, I'm unconvinced that it makes sense for the console-api
crate to also have a feature for enabling "compression" — any crate that depends on console-api
will generally have its own tonic
dependency, and can just enable whatever tonic
feature flags it wants to directly.
That's a completely reasonable argument, and I agree that it should really be up to the application's choice of tonic
feature flags.
But therein lies the problem -- right now the application can't choose to use the compression
flag, because then console-api
fails to compile (its generated protos not expecting the fields to be other than ()
)
Specifically, errors like:
Error: --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/console-api-0.2.0/src/generated/rs.tokio.console.instrument.rs:338:33
|
338 | ... accept_compression_encodings,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `EnabledCompressionEncodings`, found `()`
error[E0308]: mismatched types
Error: --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/console-api-0.2.0/src/generated/rs.tokio.console.instrument.rs:339:33
|
339 | ... send_compression_encodings,
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ expected struct `EnabledCompressionEncodings`, found `()`
error[E0308]: mismatched types
when the application has compression
turned on
I'd be more than happy if the application can choose 👍
But therein lies the problem -- right now the application can't choose to use the
compression
flag, because thenconsole-api
fails to compile (its generated protos not expecting the fields to be other than()
)
Ah, hmm...so it seems like the issue here is that we've checked in the generated Rust code for the protos, rather than generating them every time the crate is built. This was changed in #318 in order to remove the build-time dependency on protoc
every time console-api
is built.
However, it seems like an unfortunate consequence of that change is that now, the compression
feature (and presumably, any other feature flags which affect the generated Rust code are now no longer additive, and enabling a feature flag causes build failures. This is not kosher for how cargo feature flags are expected to work. I think this might mean that we should stop checking in generated Rust code, and instead switch back to generating the Rust code at build-time. cc @LucioFranco, what do you think?
Yeah, exactly! Ironically I found this issue because I wanted to generate our own protos and remove the build-time dependency. Generating them turned on compression, and, here we are.
I'm good with any solution, but yeah, non-kosher feature flags are rough.
I think I may have an angle that can allow us to remove the compression
flag from tonic-build
. This would allow for checking in the generated code while still leaving the decision whether to include the compression machinery to the end application (via the compression
flag on tonic
).
I just opened a PR on tonic
that, if accepted, should allow generated code to be agnostic to the eventual compression features enabled while building an application.
Hi, is there any solution for this? How could I compile if I rely on compression flag
Hi, is there any solution for this? How could I compile if I rely on compression flag
It looks like @neoeinstein's upstream PR hyperium/tonic#1004 has merged, but hasn't been released (as it's a breaking change). When tonic
0.8 is released, that should resolve this issue.
I think the upstream fix is almost certainly the best solution for this issue. In the meantime, I would consider accepting a PR that changes the console-api
build process to not check the generated code in to the repo, and builds it in a build script instead (the way we did prior to https://github.com/tokio-rs/console/pull/318. That way, a downstream dependent enabling the tonic-build/compression
feature would result in console-api
generating code that supports compression.
This would re-introduce the build-time dependency on protoc
for compiling crates that depend on console-api
, which is somewhat unfortunate. We should check with the Tonic maintainers to see if a release with hyperium/tonic#1004 will happen in the near future --- if it's going to happen soon, it would be preferable to not have to do a workaround that re-introduces the build-time protoc
dep.
I am no longer experiencing this issue now that tonic
0.8
has been released (using the gzip
feature in tonic
and no added features in tonic-build
). Is there anything left to track in this issue?
We need to update our tonic dependency, I believe. But, it seems that the problem has, in fact, been fixed upstream.
Think we can close this, right? :D
Looks like I forgot to publish a release. :sweat:
What problem are you trying to solve?
Compiling
console-subscriber
into a package that already has some protos and tonic compiled in.If these protos are using tonic's
compression
feature, they will enable a couple of fields (accept_compression_encodings
,send_compression_encodings
) that appear inconsole-api
's generated protobufs that currently expect a unit.How should the problem be solved?
Regenerate
console-api
's protos with tonic's compression turned on (it ought to be backward compatible if it's on)Any alternatives you've considered?
Allow a feature, either through
console-subscriber
through toconsole-api
to optionally enable the compression featureHow would users interact with this feature?
No response
Would you like to work on this feature?
maybe