fix: include_file should handle proto without package #1002

MixusMinimax · 2024-03-13T22:13:10Z

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 · 2024-03-13T22:47:19Z

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

caspermeijn · 2024-03-22T16:29:29Z

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 · 2024-03-24T22:10:42Z

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 · 2024-03-24T22:11:38Z

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 · 2024-03-24T22:13:48Z

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

caspermeijn

The PR title should be something like: "include_file should handle proto without package". Please change it.

prost-build/src/lib.rs

tests/src/build.rs

caspermeijn · 2024-04-05T08:37:07Z

tests/src/complex_package_structure/proto/post/content.proto

@@ -0,0 +1,13 @@
+syntax = "proto3";
+
+package post.content;


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?

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

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?

caspermeijn · 2024-04-05T08:38:38Z

tests/src/no_package_with_message.proto

+syntax = "proto3";
+
+message NoPackageWithMessageExampleMsg {
+  string some_field = 1;
+}


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

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.

caspermeijn

I very much like the new implementation. It simplifies the code and fixes your bug.

caspermeijn · 2024-04-12T08:06:59Z

prost-build/src/lib.rs

+            .default_package_filename("_.default")
+            .write_includes(modules.iter().collect(), &mut buf, None, &file_names)
+            .unwrap();
+        let expected = [


Other tests read the expected result from a file. I think that improves maintainalibity. For example:

prost/prost-build/src/lib.rs

Lines 1682 to 1684 in 04be604

let expected_content =

read_all_content("src/fixtures/helloworld/_expected_helloworld_formatted.rs")

.replace("\r\n", "\n");

caspermeijn · 2024-04-12T08:08:51Z

prost-build/src/lib.rs

@@ -1833,4 +1824,57 @@ mod tests {
        f.read_to_string(&mut content).unwrap();
        content
    }
+
+    #[test]
+    fn test_write_includes() {


I really like this test. It is simple and to the point. And it doesn't require build.rs.

tests/src/build.rs

caspermeijn · 2024-04-12T08:14:14Z

tests/src/complex_package_structure/proto/post/content.proto

@@ -0,0 +1,13 @@
+syntax = "proto3";
+
+package post.content;


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?

caspermeijn · 2024-04-12T08:15:56Z

tests/src/no_package_with_message.proto

+syntax = "proto3";
+
+message NoPackageWithMessageExampleMsg {
+  string some_field = 1;
+}


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.

MixusMinimax · 2024-04-12T18:30:48Z

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.

caspermeijn

This PR is almost ready. My only major open question is this:

What do no_package_with_message and complex_package_structure test, that is not already tested by the existing tests?

prost-build/src/lib.rs

MixusMinimax · 2024-04-26T09:07:47Z

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

…write_includes # Conflicts: # prost-build/src/lib.rs

MixusMinimax · 2024-04-26T10:33:09Z

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.

…ludes

MixusMinimax · 2024-04-26T11:13:08Z

Ok this should be it

caspermeijn · 2024-04-26T13:41:59Z

prost-build/src/module.rs

@@ -40,6 +39,15 @@ impl Module {
        self.components.iter().map(|s| s.as_str())
    }

+    #[must_use]
+    #[inline(always)]
+    pub fn starts_with(&self, needle: &[String]) -> bool


I believe this function should be pub(crate). Similar to the function you removed.

Done 👍🏽

caspermeijn · 2024-05-05T19:35:32Z

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

_PROST!_ is a [Protocol Buffers](https://developers.google.com/protocol-buffers/) implementation for the [Rust Language](https://www.rust-lang.org/). `prost` generates simple, idiomatic Rust code from `proto2` and `proto3` files. This patch update brings new fixes: - fix: include_file should handle proto without package (tokio-rs#1002) - Place Config::format behind the format feature flag - Handle keyword `Self` after stripping enum type prefix (tokio-rs#998) ## Documentation - fix(readme): fix the link and badge for CI (tokio-rs#1049) ## Internal - style(codegen): `Syntax` to a separate file (tokio-rs#1029) - chore(codegen): extract c string escaping to a separate file (tokio-rs#1028) - style(prost-build): `CodeGenerator::boxed` method (tokio-rs#1019) - style(prost-build): Consolidate field data into struct (tokio-rs#1017) - style(prost-build): `BytesType and MapType` into a `collections` module. (tokio-rs#1030) - style(prost-build): Split `Config` and `Module` into a separate module and files (tokio-rs#1020) - style(prost-build): prost_path helper (tokio-rs#1018) - style: Fix toml indent (tokio-rs#1048) - style: Fix clippy warnings and enable clippy in CI (tokio-rs#1008) - build: Use git submodule to download protobuf sources (tokio-rs#1014) - ci: Add TOML validation with `taplo` (tokio-rs#1034) - tests: Create a separate tempdir for each test (tokio-rs#1044) - tests: Remove GoogleMessage3 and GoogleMessage4 benchmarks (tokio-rs#1037) - chore: Update internal crates to Rust edition 2021 (tokio-rs#1039) - chore: Update crate descriptions (tokio-rs#1038) - chore: Fix clippy checks in CI (tokio-rs#1032) - chore: Add Casper Meijn as author (tokio-rs#1025)

_PROST!_ is a [Protocol Buffers](https://developers.google.com/protocol-buffers/) implementation for the [Rust Language](https://www.rust-lang.org/). `prost` generates simple, idiomatic Rust code from `proto2` and `proto3` files. This patch update brings new fixes: - fix: include_file should handle proto without package (#1002) - Place Config::format behind the format feature flag - Handle keyword `Self` after stripping enum type prefix (#998) ## Documentation - fix(readme): fix the link and badge for CI (#1049) ## Internal - style(codegen): `Syntax` to a separate file (#1029) - chore(codegen): extract c string escaping to a separate file (#1028) - style(prost-build): `CodeGenerator::boxed` method (#1019) - style(prost-build): Consolidate field data into struct (#1017) - style(prost-build): `BytesType and MapType` into a `collections` module. (#1030) - style(prost-build): Split `Config` and `Module` into a separate module and files (#1020) - style(prost-build): prost_path helper (#1018) - style: Fix toml indent (#1048) - style: Fix clippy warnings and enable clippy in CI (#1008) - build: Use git submodule to download protobuf sources (#1014) - ci: Add TOML validation with `taplo` (#1034) - tests: Create a separate tempdir for each test (#1044) - tests: Remove GoogleMessage3 and GoogleMessage4 benchmarks (#1037) - chore: Update internal crates to Rust edition 2021 (#1039) - chore: Update crate descriptions (#1038) - chore: Fix clippy checks in CI (#1032) - chore: Add Casper Meijn as author (#1025)

fix tokio-rs#1001 and add tests

3ea9f40

MixusMinimax mentioned this pull request Mar 13, 2024

include_file does not work if any of the modules does not declare a package #1001

Closed

MixusMinimax added 2 commits March 13, 2024 23:52

add alloc:: imports

ae360ef

Merge branch 'master' into fix/write_includes

9aea9b7

MixusMinimax added 2 commits March 24, 2024 23:15

Merge branch 'master' into fix/write_includes

21056cd

Merge branch 'master' into fix/write_includes

ba9e558

caspermeijn reviewed Apr 5, 2024

View reviewed changes

MixusMinimax changed the title ~~fix #1001 and add tests~~ include_file should handle proto without package Apr 5, 2024

MixusMinimax added 3 commits April 6, 2024 01:25

rewrite write_includes to allow for empty modules.

511fdd5

Merge branch 'master' into fix/write_includes

f2bf323

Merge branch 'master' into fix/write_includes

b87c205

caspermeijn reviewed Apr 12, 2024

View reviewed changes

MixusMinimax added 2 commits April 12, 2024 20:09

Merge branch 'master' into fix/write_includes

376adcb

create test fixture for write_includes

4c52df9

fix lints, remove line feeds

4ff26e5

caspermeijn requested changes Apr 26, 2024

View reviewed changes

prost-build/src/lib.rs Outdated Show resolved Hide resolved

MixusMinimax added 2 commits April 26, 2024 12:17

Merge remote-tracking branch 'refs/remotes/upstream/master' into fix/…

755ea0d

…write_includes # Conflicts: # prost-build/src/lib.rs

fixes after merge master

56c1a2f

MixusMinimax added 2 commits April 26, 2024 12:41

remove some duplicate tests and alter existing ones to test write_inc…

8ea9ec9

…ludes

more test

7b0ccb2

caspermeijn requested changes Apr 26, 2024

View reviewed changes

module.rs Module::starts_with visibility

bfb0896

caspermeijn approved these changes May 5, 2024

View reviewed changes

caspermeijn changed the title ~~include_file should handle proto without package~~ fix: include_file should handle proto without package May 5, 2024

caspermeijn added this pull request to the merge queue May 5, 2024

Merged via the queue into tokio-rs:master with commit baddf98 May 5, 2024
15 checks passed

caspermeijn mentioned this pull request May 5, 2024

chore: Release version 0.12.5 #1050

Merged

MixusMinimax deleted the fix/write_includes branch May 7, 2024 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: include_file should handle proto without package #1002

fix: include_file should handle proto without package #1002

MixusMinimax commented Mar 13, 2024

MixusMinimax commented Mar 13, 2024

caspermeijn commented Mar 22, 2024

MixusMinimax commented Mar 24, 2024 •

edited

Loading

MixusMinimax commented Mar 24, 2024 •

edited

Loading

MixusMinimax commented Mar 24, 2024 •

edited

Loading

caspermeijn left a comment

caspermeijn Apr 5, 2024

MixusMinimax Apr 5, 2024

caspermeijn Apr 12, 2024

caspermeijn Apr 5, 2024

caspermeijn Apr 12, 2024

caspermeijn left a comment

caspermeijn Apr 12, 2024

caspermeijn Apr 12, 2024

caspermeijn Apr 12, 2024

caspermeijn Apr 12, 2024

MixusMinimax commented Apr 12, 2024

caspermeijn left a comment

MixusMinimax commented Apr 26, 2024 •

edited

Loading

MixusMinimax commented Apr 26, 2024

MixusMinimax commented Apr 26, 2024

caspermeijn Apr 26, 2024

MixusMinimax May 4, 2024

caspermeijn commented May 5, 2024

	let expected_content =
	read_all_content("src/fixtures/helloworld/_expected_helloworld_formatted.rs")
	.replace("\r\n", "\n");

fix: include_file should handle proto without package #1002

fix: include_file should handle proto without package #1002

Conversation

MixusMinimax commented Mar 13, 2024

MixusMinimax commented Mar 13, 2024

caspermeijn commented Mar 22, 2024

MixusMinimax commented Mar 24, 2024 • edited Loading

MixusMinimax commented Mar 24, 2024 • edited Loading

MixusMinimax commented Mar 24, 2024 • edited Loading

caspermeijn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caspermeijn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MixusMinimax commented Apr 12, 2024

caspermeijn left a comment

Choose a reason for hiding this comment

MixusMinimax commented Apr 26, 2024 • edited Loading

MixusMinimax commented Apr 26, 2024

MixusMinimax commented Apr 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caspermeijn commented May 5, 2024

MixusMinimax commented Mar 24, 2024 •

edited

Loading

MixusMinimax commented Mar 24, 2024 •

edited

Loading

MixusMinimax commented Mar 24, 2024 •

edited

Loading

MixusMinimax commented Apr 26, 2024 •

edited

Loading