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

#726 breaks build #747

Closed
matze opened this issue Nov 7, 2022 · 9 comments
Closed

#726 breaks build #747

matze opened this issue Nov 7, 2022 · 9 comments

Comments

@matze
Copy link

matze commented Nov 7, 2022

We have a build.rs to add some additional methods to services. Upgrading from 0.11.1 to 0.11.2 caused this (actual binary names and paths redacted)

error: failed to run custom build command for `…`

Caused by:
  process didn't exit successfully: `/…/build-script-build` (exit status: 101)
  --- stderr
  thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', /…/prost/prost-build/src/lib.rs:1069:52
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I could pinpoint the regressing behaviour to the change commited with #726 however I do not see yet how this causes the unwrap to fail. The crash traces from

    prost_build::Config::new()
        .file_descriptor_set_path(desc_path.clone())
        .skip_protoc_run()
        .extern_path(".foo", "foo_proto")
        .include_file("prost_includes.rs")
        .service_generator(Box::new(gen))
        .compile_protos(&Vec::<&str>::default(), &Vec::<&str>::default())?;

in build.rs to

        if let Some(ref mut service_generator) = self.service_generator {
            for (module, package) in packages {
                let buf = modules.get_mut(&module).unwrap();
                service_generator.finalize_package(&package, buf);
            }
        }

in the prost-build/src/lib.rs.

@LucioFranco
Copy link
Member

Would it be possible to provide a reproduction proto file I can play around with?

@matze
Copy link
Author

matze commented Nov 7, 2022

I will try to come up with a repro build.rs and proto file(s).

@LucioFranco
Copy link
Member

I just need the proto file really, I can create a test to reproduce and look at it. We may need to yank 0.11.2 then and either fix or remove this commit. Thanks!

@matze
Copy link
Author

matze commented Nov 8, 2022

Hmm, it's not so easy to come up with a repro from scratch, it will take a while I guess :-(

@LucioFranco
Copy link
Member

All good! This has been a really tricky feature that I already had to revert once so getting that edge case test case would be super valuable!

matze added a commit to matze/prost that referenced this issue Nov 9, 2022
This happens for protos referencing a different package from their
name, i.e. something like foo/v1/bar.proto where bar.proto defines
`package foo.v1`.

Fixes tokio-rs#747
@matze
Copy link
Author

matze commented Nov 9, 2022

I could pin it down. As written in the PR, it happens for protos that specify a parent package.

@matze
Copy link
Author

matze commented Nov 10, 2022

@LucioFranco could you reproduce this issue?

@LucioFranco
Copy link
Member

I have not had the time to try it out yet but I think for your PR adding a test would be very helpful.

@matze
Copy link
Author

matze commented Nov 15, 2022

The issue is actually a combination of the mentioned path hierarchy as well as the fact that we skip generating one particular service causing the output to be empty and hence the module to be removed. So, I will close this issue as a very peculiar case.

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 a pull request may close this issue.

2 participants