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

Conversation

MixusMinimax
Copy link
Contributor

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

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

@caspermeijn
Copy link
Collaborator

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

MixusMinimax commented Mar 24, 2024

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

MixusMinimax commented Mar 24, 2024

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

MixusMinimax commented Mar 24, 2024

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

Copy link
Collaborator

@caspermeijn caspermeijn left a comment

Choose a reason for hiding this comment

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

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

prost-build/src/lib.rs Outdated Show resolved Hide resolved
tests/src/build.rs Outdated Show resolved Hide resolved
@@ -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?

Comment on lines 1 to 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.

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

@caspermeijn caspermeijn left a comment

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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");

@@ -1833,4 +1824,57 @@ mod tests {
f.read_to_string(&mut content).unwrap();
content
}

#[test]
fn test_write_includes() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

tests/src/build.rs Outdated Show resolved Hide resolved
@@ -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.

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?

Comment on lines 1 to 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.

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

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.

Copy link
Collaborator

@caspermeijn caspermeijn left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@MixusMinimax
Copy link
Contributor Author

MixusMinimax commented Apr 26, 2024

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

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

Ok this should be it

@@ -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
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 function should be pub(crate). Similar to the function you removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done 👍🏽

@caspermeijn caspermeijn changed the title include_file should handle proto without package fix: include_file should handle proto without package May 5, 2024
@caspermeijn 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
Copy link
Collaborator

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

caspermeijn added a commit to caspermeijn/prost that referenced this pull request May 5, 2024
_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)
@MixusMinimax MixusMinimax deleted the fix/write_includes branch May 7, 2024 20:41
github-merge-queue bot pushed a commit that referenced this pull request May 8, 2024
_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)
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.

include_file does not work if any of the modules does not declare a package
2 participants