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

Don't generate empty google.protobuf.rs #2005

Merged
merged 2 commits into from Jul 8, 2022

Conversation

tustvold
Copy link
Contributor

@tustvold tustvold commented Jul 5, 2022

Which issue does this PR close?

Closes #.

Rationale for this change

Since tokio-rs/prost#639 was reverted, prost generates some empty files. Lets explicitly make sure this file is expunged.

What changes are included in this PR?

Automatically deletes the empty file if it is created

Are there any user-facing changes?

No

@github-actions github-actions bot added the arrow-flight Changes to the arrow-flight crate label Jul 5, 2022
@@ -88,6 +88,13 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
file.write_all(buffer.as_bytes())?;
}

// Prost currently generates an empty file, this was fixed but then reverted
// https://github.com/tokio-rs/prost/pull/639
let google_protobuf_rs = Path::new("src/sql/google.protobuf.rs");
Copy link
Contributor

Choose a reason for hiding this comment

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

should we also test that it is actually empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could do, but given it would be a code change to actually include!() it in the source code, I'm inclined to think it doesn't matter

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking something like https://doc.rust-lang.org/stable/std/fs/struct.Metadata.html#method.len

I was worrying about some poor soul in the future who expects the file to be there (because maybe some new version of prost puts something useful in it) but it is mysteriously deleted

@alamb alamb merged commit 9333a85 into apache:master Jul 8, 2022
@alamb alamb added the development-process Related to development process of arrow-rs label Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow-flight Changes to the arrow-flight crate development-process Related to development process of arrow-rs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants