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

fix: include_file should handle proto without package #1002

Merged
merged 16 commits into from May 5, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 25 additions & 0 deletions prost-build/src/lib.rs
Expand Up @@ -1168,6 +1168,31 @@ impl Config {
let mut written = 0;
entries.sort();

if depth == 0 {
caspermeijn marked this conversation as resolved.
Show resolved Hide resolved
let (empty, remaining): (Vec<&Module>, Vec<&Module>) =
entries.into_iter().partition(|m| m.is_empty());
for _ in empty {
if basepath.is_some() {
self.write_line(
outfile,
depth,
&format!("include!(\"{}.rs\");", self.default_package_filename),
)?;
} else {
self.write_line(
outfile,
depth,
&format!(
"include!(concat!(env!(\"OUT_DIR\"), \"/{}.rs\"));",
self.default_package_filename
),
)?;
}
written += 1;
}
entries = remaining;
}

while !entries.is_empty() {
let modident = entries[0].part(depth);
let matching: Vec<&Module> = entries
Expand Down
30 changes: 30 additions & 0 deletions tests/src/build.rs
Expand Up @@ -146,6 +146,36 @@ fn main() {
.compile_protos(&[src.join("well_known_types.proto")], includes)
.unwrap();

{
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();
}

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

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

prost_build::Config::new()
.out_dir(out_path)
.include_file("__.rs")
caspermeijn marked this conversation as resolved.
Show resolved Hide resolved
.default_package_filename("__.default")
.compile_protos(
&[src.join("complex_package_structure/proto/post/post.proto")],
&[src.join("complex_package_structure/proto")],
)
.unwrap();
}

config
.compile_protos(
&[src.join("packages/widget_factory.proto")],
Expand Down
63 changes: 63 additions & 0 deletions tests/src/complex_package_structure/mod.rs
@@ -0,0 +1,63 @@
use alloc::{string::ToString, vec};

use self::proto::image::Image;
use self::proto::post::content::post_content_fragment;
use self::proto::post::content::PostContentFragment;
use self::proto::post::Post;
use self::proto::user::User;
use self::proto::Timestamp;

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

#[test]
fn test_complex_package_structure() {
let user = User {
id: "69a4cd96-b956-4fb1-9a97-b222eac33b8a".to_string(),
name: "Test User".to_string(),
created_at: Some(Timestamp {
seconds: 1710366135,
nanos: 0,
}),
..User::default()
};
let posts = vec![
Post::default(),
Post {
id: "aa1e751f-e287-4c6e-aa0f-f838f96a1a60".to_string(),
author: Some(user),
content: vec![
PostContentFragment {
content: Some(post_content_fragment::Content::Text(
"Hello, world!".to_string(),
)),
},
PostContentFragment {
content: Some(post_content_fragment::Content::Image(Image {
name: "doggo.jpg".to_string(),
description: Some("A dog".to_string()),
data: vec![0, 1, 2, 3],
})),
},
PostContentFragment {
content: Some(post_content_fragment::Content::Link(
"https://example.com".to_string(),
)),
},
],
..Post::default()
},
Post::default(),
];
assert_eq!(posts.len(), 3);
assert_eq!(posts[1].content.len(), 3);
if let PostContentFragment {
content: Some(post_content_fragment::Content::Image(Image { name, .. })),
} = &posts[1].content[1]
{
assert_eq!(name, "doggo.jpg");
} else {
assert!(false, "Expected an image")
}
}
3 changes: 3 additions & 0 deletions tests/src/complex_package_structure/proto/.editorconfig
@@ -0,0 +1,3 @@
[*]
ij_protobuf_keep_blank_lines_in_code = 0
insert_final_newline = true
13 changes: 13 additions & 0 deletions tests/src/complex_package_structure/proto/post/content.proto
@@ -0,0 +1,13 @@
syntax = "proto3";

package post.content;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this complex example testing?

  • It seems you do set the package names.
  • Please add more documentation to the example to specify that is tested.
  • Can the example the reduced and still test the important parts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This example has multiple files, some with packages, and some without. It also contains nested packages, and imports between everything. I wanted to make sure that the generated include file is correct for a combination of these edge cases.
I can remove it or simplify it once I know the solution works

Copy link
Collaborator

Choose a reason for hiding this comment

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

My intention is not to remove the test. All functionality should have a test. You solve a bug, so the bugged behavior should be tested.

My comment is in two parts:

  • Please add documentation to the test so that it is clear why the test exists and what is tested.
  • Is each part of the example needed to test the behavior? Or is some of it redundant with other tested examples?


import "wrong/image.proto";

message PostContentFragment {
oneof content {
string text = 1;
image.Image image = 2;
string link = 3;
}
}
17 changes: 17 additions & 0 deletions tests/src/complex_package_structure/proto/post/post.proto
@@ -0,0 +1,17 @@
syntax = "proto3";

package post;

import "std/time.proto";
import "user/user.proto";
import "post/content.proto";

message Post {
string id = 1;
string title = 2;
reserved 3;
user.User author = 4;
Timestamp created_at = 5;
Timestamp updated_at = 6;
repeated content.PostContentFragment content = 7;
}
6 changes: 6 additions & 0 deletions tests/src/complex_package_structure/proto/std/time.proto
@@ -0,0 +1,6 @@
syntax = "proto3";

message Timestamp {
int64 seconds = 1;
int32 nanos = 2;
}
13 changes: 13 additions & 0 deletions tests/src/complex_package_structure/proto/user/user.proto
@@ -0,0 +1,13 @@
syntax = "proto3";

package user;

import "std/time.proto";

message User {
string id = 1;
Timestamp created_at = 2;
Timestamp updated_at = 3;
string name = 4;
string email = 5;
}
12 changes: 12 additions & 0 deletions tests/src/complex_package_structure/proto/wrong/image.proto
@@ -0,0 +1,12 @@
// This file makes sure that the file path of the proto file has nothing to do with the package name,
// therefore testing the validity of write_includes.

syntax = "proto3";

package image;

message Image {
string name = 1;
optional string description = 2;
bytes data = 3;
}
4 changes: 4 additions & 0 deletions tests/src/lib.rs
Expand Up @@ -33,6 +33,8 @@ pub mod unittest;
#[cfg(test)]
mod bootstrap;
#[cfg(test)]
mod complex_package_structure;
#[cfg(test)]
mod debug;
#[cfg(test)]
mod deprecated_field;
Expand All @@ -41,6 +43,8 @@ mod generic_derive;
#[cfg(test)]
mod message_encoding;
#[cfg(test)]
mod no_package_with_message;
#[cfg(test)]
mod no_unused_results;
#[cfg(test)]
#[cfg(feature = "std")]
Expand Down
5 changes: 5 additions & 0 deletions tests/src/no_package_with_message.proto
@@ -0,0 +1,5 @@
syntax = "proto3";

message NoPackageWithMessageExampleMsg {
string some_field = 1;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this test already exists: no_root_packages. Can you extend the test to also test your usecase?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What I meant is: the no_root_packages test seems to do the same as the test you add. If so, please remove your test. If not, please explain why this is different.

11 changes: 11 additions & 0 deletions tests/src/no_package_with_message.rs
@@ -0,0 +1,11 @@
mod proto {
include!(concat!(env!("OUT_DIR"), "/no_package/_includes.rs"));
}

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