Get rid of unused transport feature.

[BratSinot]

Greetings!

Motivation

I don't use transport feature and I don't need it dependencies.

Solution

Disable unused transport feature in tonic-reflections and tonic-health.

[LucioFranco]

I think we can merge this just need to fix CI

[BratSinot]

I think we can merge this just need to fix CI

I also can't catch how it should work.
tonic-health repo have already generated rs-proto files, and it doesn't re-gen.
So, tonic-health = { version = "*", default-features = false } doesn't work.

[LucioFranco]

Yeah, looks like you need to regen, this should be done by running cargo test. Though I wonder if some weird feature stuff is going on where the transport portion is enabled by accident.

[LucioFranco]

https://github.com/hyperium/tonic/blob/master/tonic-health/tests/bootstrap.rs this is the test that regens the codegen that you then have to commit after running the test.

[BratSinot]

https://github.com/hyperium/tonic/blob/master/tonic-health/tests/bootstrap.rs this is the test that regens the codegen that you then have to commit after running the test.

Yeah, but if the user of library will enable transport feature, crate doesn't rebuild. So or we regenerate and add flag check to generated code, or generate 2 version and use one of them based on that flag.

[LucioFranco]

Thats a good point, I would say we want to generate it without the transport feature since in reality it doesn't really add that much for codegen and people can just pass things into the normal new fn.

[BratSinot]

Thats a good point, I would say we want to generate it without the transport feature since in reality it doesn't really add that much for codegen and people can just pass things into the normal new fn.

Okey. Done.

[LucioFranco]

Looks like what is being generated on CI isn't the same as what is committed and that is causing CI failures.

[BratSinot]

Looks like what is being generated on CI isn't the same as what is committed and that is causing CI failures.

Ow, yeah, it because I always enable rustfmt on save and it add additional spaces to commentary. I will fix it tomorrow.

[BratSinot]

Looks like what is being generated on CI isn't the same as what is committed and that is causing CI failures.

Yeah, okey, your tests running as --all --all-features, so bootstrap in tonic-health starts with enabled transport feature, and it gives error, because files in source-tree are generated without transport features.

Aaa, okey, I got it. You mean remove tonic/transport.
I also add small example in tonic-health/README.md.

[LucioFranco]

Seems like another CI failure https://github.com/hyperium/tonic/actions/runs/3337161875/jobs/5527464629

[BratSinot]

Seems like another CI failure https://github.com/hyperium/tonic/actions/runs/3337161875/jobs/5527464629

Yeah, because tokio::spawn required rt feature and tonic-reflection doesn't enable it.

Okey, I just found that I can run pipelines in my fork, so I can settle down all errors without troubling you =)

[BratSinot]

Seems like another CI failure https://github.com/hyperium/tonic/actions/runs/3337161875/jobs/5527464629

Arr, I can't get it who enable transport -_-"
test --all --all-features -- error.
test -p tonic-health --test bootstrap -- OK!!

Seems that --all-features enables transport in tonic-build and tonic-health use it.

[BratSinot]

@LucioFranco
Okey, I have an idea. I've marked tonic-health bootstrap "test" as #[ignored] and add additional step in workflow specially for that case:

    - name: Run ignored tonic-health bootstrap test
      run: test --all-features --package tonic-health --test bootstrap bootstrap -- --ignored --exact

[LucioFranco]

I don't think this is supposed to be ignored? We want the test to run and it should generate the exact code that was committed? Does it not work like that?

[BratSinot]

cargo test --ignored run tests which are marked as #[ignored].
So scheme is next:

1. running test --all --all-features in root directory with all packages causes to transport feature on for tonic-builds, which lead to overriding dev-dependencies in tonic-health, which lead to rerun bootstrap.rs with transport feature, which lead to generating transport code;
2. so we ignore that bootstrap in test --all, but run this test separately;

[LucioFranco]

I see what you're saying, I think the better approach here is to add a build_transport feature that when transport is enabled for build can disable it. So in the bootstrap test we can just tell it even if transport is enabled don't generate it. Here is the PR #1130 so once that is merged you can just add the build_transport(false) setting to tonic-health's bootstrap.rs and then remove the transport feature. Or if it works you can make it so that when health's transport feature is enabled it sets that to true etc.

[LucioFranco]

Ok I finally got the PR merged after 3 days of trying to fix a CI bug with it! So if you update this PR with that then we can move forward. We should back out the CI changes etc the test should pass now.

[BratSinot]

Ok I finally got the PR merged after 3 days of trying to fix a CI bug with it! So if you update this PR with that then we can move forward. We should back out the CI changes etc the test should pass now.

Okey, now it have:

Run cargo test --all --all-features
    Updating git repository `[https://github.com/tokio-rs/prost/`](https://github.com/tokio-rs/prost/%60)
error: failed to load source for dependency `prost-build`

Caused by:
  Unable to update https://github.com/tokio-rs/prost/?branch=lucio/format

[LucioFranco]

The code changes seem fine to me but the ci changes I am not sure about.