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

Get rid of unused transport feature. #1112

Merged
merged 9 commits into from
Nov 7, 2022
Merged

Get rid of unused transport feature. #1112

merged 9 commits into from
Nov 7, 2022

Conversation

BratSinot
Copy link
Contributor

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
Copy link
Member

I think we can merge this just need to fix CI

@BratSinot
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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

@BratSinot
Copy link
Contributor Author

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
Copy link
Contributor Author

BratSinot commented Oct 27, 2022

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
Copy link
Member

@BratSinot
Copy link
Contributor Author

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
Copy link
Contributor Author

BratSinot commented Oct 28, 2022

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
Copy link
Contributor Author

@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

- uses: Swatinem/rust-cache@v1
- uses: actions/checkout@master
- name: Run tests
run: cargo test --all --all-features
- name: Run ignored tonic-health bootstrap test
run: cargo test --all-features --package tonic-health --test bootstrap bootstrap -- --ignored --exact
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

2 participants