tokio-rs / prost

PROST! a Protocol Buffers implementation for the Rust Language
Apache License 2.0
3.69k stars 481 forks source link

fix: include_file should handle proto without package #1002

Closed MixusMinimax closed 2 months ago

MixusMinimax commented 4 months ago

Fixes #1001

The solution is to do the same thing that some other tests already do manually, we need to write the include for the root package at the root level of the include file.

MixusMinimax commented 4 months ago

I'll make the no-std tests work, sorry

caspermeijn commented 4 months ago

I don't understand what the problem is you are trying to fix.

Could you try to explain the problem you are encountering? Maybe you could add a minimal, reproducible example?

MixusMinimax commented 4 months ago

Hi! The included tests have an example:

tests/src/no_package_with_message.proto

syntax = "proto3";

message NoPackageWithMessageExampleMsg {
  string some_field = 1;
}

tests/src/no_package_with_message.rs

mod proto {
    include!(concat!(env!("OUT_DIR"), "/no_package/_includes.rs"));
}

#[test]
fn it_works() {
    assert_eq!(
        proto::NoPackageWithMessageExampleMsg::default(),
        proto::NoPackageWithMessageExampleMsg::default()
    );
}

tests/src/build.rs

{
    let out = std::env::var("OUT_DIR").unwrap();
    let out_path = PathBuf::from(out).join("no_package");

    std::fs::create_dir_all(&out_path).unwrap();

    prost_build::Config::new()
        .out_dir(out_path)
        .include_file("_includes.rs")
        .compile_protos(&[src.join("no_package_with_message.proto")], includes)
        .unwrap();
}
MixusMinimax commented 4 months ago

The function that generates _includes.rs panics if the proto files contain a file without a package statement (which is the case in this example).

MixusMinimax commented 4 months ago

This is the only change necessary: prost-build/src/lib.rs

the rest of the changes in this PR are just tests to validate that everything works.

The issue #1001 also has some more description about this

MixusMinimax commented 3 months ago

it seems that Module::to_partial_filename is no longer used. As it is a private function, should I remove it? It was used in the old implementation of write_includes.

MixusMinimax commented 3 months ago

I have been quite busy, sorry, I'll get around to finishing that up over the weekend. Those other tests were more like integration tests rather than unit tests, just testing all the features that my changes could have affected, mostly a combination of using the write_include_file feature and proto package structures that contain nested packages, and content in the root package. None of the other tests tested all of that in combination, which I needed because the features could affect each other. But I agree, just altering the existing tests makes more sense.

TL;DR combination of no-package files and write_includes was not tested before in combination with files that DO have package names

MixusMinimax commented 3 months ago

The no_root_packages test does not generate an includes file, and I did not want to mess with existing tests, but I think in this case it makes sense to just add that to no_root_packages so that no_package_with_message is not needed anymore.

MixusMinimax commented 3 months ago

Ok this should be it

caspermeijn commented 2 months ago

Thank you for your contribution. I appreciate the patience during the review.