search

fdeantoni / prost-wkt

Prost Well-Known-Types serialization and deserialization.
Apache License 2.0
76 stars 35 forks source link

prost-wkt's build.rs shouldn't require network access #8

Closed bpowers closed 2 years ago

bpowers commented 2 years ago

prost-wkt reaches out to download a file from github: https://github.com/fdeantoni/prost-wkt/blob/master/wkt-types/build.rs#L49

This breaks docs.rs builds for things that depend on prost-wkt, like this: https://docs.rs/crate/envoy-control-plane/0.4.0/builds/502978

It looks like we're pretty dependent on that specific version of the file from prost - can we just check it in/update it when it changes? I'm happy to put a PR up if that sounds reasonable

fdeantoni commented 2 years ago

Good point. Maybe a better way to go about it is to use git submodules to 'link' the required prost code? If that makes the process too convoluted then I suppose just checking in the appropriate file (with a good README instruction about it) will also do. Let me know what you think.

vitalyd commented 2 years ago

I hit the same issue (with build.rs reaching out to the network). We have a private cargo registry backed by locally vendored crates. The build environment does not allow arbitrary network access.

I tend to agree with @bpowers suggestion of simply checking in the required file into prost-wkt and maintaining/updating it as needed. A submodule "link" would also be fine for my use case since cargo vendor ought to pick it up.

fdeantoni commented 2 years ago

So you guys can move forward I think it is best if we just check in the file instead, and have the build use that instead of reaching out over the net. I stil like to have it separate from src directory, but I'm still contemplating where exactly to place it in the project. Maybe under ${PROJECT_ROOT}/wkt-types/resources with a README.md on how to manage this file? Let me know your thoughts.

fdeantoni commented 2 years ago

So you guys can move forward I think it is best if we just check in the file instead, and have the build use that instead of reaching out over the net. I stil like to have it separate from src directory, but I'm still contemplating where exactly to place it in the project. Maybe under ${PROJECT_ROOT}/wkt-types/resources with a README.md on how to manage this file? Let me know your thoughts.

vitalyd commented 2 years ago

Maybe under ${PROJECT_ROOT}/wkt-types/resources with a README.md on how to manage this file?

SGTM

Thanks @fdeantoni!

fdeantoni commented 2 years ago

I hopefully addressed this issue with commit a269f4d.

vitalyd commented 2 years ago

Thanks @fdeantoni! I think you can drop the reqwest build dep now?

Any idea when this will be published to crates.io?

fdeantoni commented 2 years ago

I will still do a general refactoring pass over things this coming week, where I will cleanup the dependencies and clean up the code (including the code changes you recommended - PRs are welcome!). After that is done I will give it another week to verify things work correctly with the other projects that use this crate. So expect it published within the next two weeks.

vitalyd commented 2 years ago

Sounds good - thanks again @fdeantoni!