Skip to content

Conversation

@psteinroe
Copy link
Contributor

@psteinroe psteinroe commented Apr 1, 2025

upgrade prost to latest to fix gnu builds. tested it locally via docker. I think the only relevant breaking change is that protoc needs to be available in the path now. to not break anything for consumers of this library, I added the protoc-bin-vendored crate.

Breaking Changes from the Changelog

v13

v12

None?

v11

  • prost-build now requires protoc to be available in the path or set via the PROTOC env var.
  • prost-types now contains new Timestamp/Duration FromStr implementations.
  • MSRV bump to 1.56 and all crates have been moved to edition 2021

@seanlinsley
Copy link
Member

seanlinsley commented Apr 1, 2025

I can understand not wanting to break installs for existing users, but I'm a little uneasy about introducing a new supply chain risk by relying on a third party to re-upload protoc to crates.io.

I wonder if we could avoid requiring protoc when installing this crate by storing the generated Rust structs in source control. In other projects we do this by overriding the OUT_DIR in build.rs, and then include! the generated file.

let out_dir = concat!(env!("CARGO_MANIFEST_DIR"), "/src");
unsafe { env::set_var("OUT_DIR", &out_dir) };

That can be combined with an envvar check to only compile the protobuf if protoc is installed:

if env::var("PROTOC").is_ok() {
    prost_build::compile_protos(...)?;
} else {
    println!("protoc not found, skipping protobuf generation");
}

@psteinroe
Copy link
Contributor Author

I can understand not wanting to break installs for existing users, but I'm a little uneasy about introducing a new supply chain risk by relying on a third party to re-upload protoc to crates.io.

I wonder if we could avoid requiring protoc when installing this crate by storing the generated Rust structs in source control. In other projects we do this by overriding the OUT_DIR in build.rs, and then include! the generated file.

let out_dir = concat!(env!("CARGO_MANIFEST_DIR"), "/src");
unsafe { env::set_var("OUT_DIR", &out_dir) };

That can be combined with an envvar check to only compile the protobuf if protoc is installed:

if env::var("PROTOC").is_ok() {
    prost_build::compile_protos(...)?;
} else {
    println!("protoc not found, skipping protobuf generation");
}

thanks for the review! I removed the dependency.

I am not sure 100% if my solution is what you had in mind though since I simply added pub mod protobuf instead of include! on the generated file.

also added a custom env var check.

@seanlinsley
Copy link
Member

Yeah, that's roughly what I was thinking. But it looks like CI needs fixing (unrelated to your change)

Error: This request has been automatically failed because it uses a deprecated version of actions/cache: v2. Please update your workflow to use v3/v4 of actions/cache to avoid interruptions. Learn more: https://github.blog/changelog/2024-12-05-notice-of-upcoming-releases-and-breaking-changes-for-github-actions/#actions-cache-v1-v2-and-actions-toolkit-cache-package-closing-down

@seanlinsley
Copy link
Member

I'm not sure where the PROTOC envvar is supposed to come from... I get the feeling that's a new thing that prost-build invented. Ideally we'd check to see if protoc is on the load path. Maybe this approach works?

@psteinroe
Copy link
Contributor Author

I'm not sure where the PROTOC envvar is supposed to come from... I get the feeling that's a new thing that prost-build invented. Ideally we'd check to see if protoc is on the load path. Maybe this approach works?

I think it's something they invented. this env var was never set for me. I replaced it with a Command check as suggested.

@seanlinsley
Copy link
Member

Looks good! There's just one more failure because of the actions/cache deprecation

@psteinroe
Copy link
Contributor Author

Looks good! There's just one more failure because of the actions/cache deprecation

ahh missed that - fixed! 🙏🏼

Copy link
Member

@seanlinsley seanlinsley left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

@seanlinsley seanlinsley merged commit 3240f5a into pganalyze:main Apr 2, 2025
7 checks passed
@seanlinsley seanlinsley mentioned this pull request Apr 2, 2025

prost_build::compile_protos(&[&out_protobuf_path.join(LIBRARY_NAME).with_extension("proto")], &[&out_protobuf_path])?;

std::fs::rename(src_dir.join("pg_query.rs"), src_dir.join("protobuf.rs"))?;
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this line broke the docs.rs build https://docs.rs/crate/pg_query/6.1.0/builds/1945029

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should detect the DOCS_RS envvar like polywrap/rust-wrap-client#245 does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pr with the fix is up: #56

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.

2 participants