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

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

Closed
bpowers opened this issue Feb 1, 2022 · 9 comments
Closed

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

bpowers opened this issue Feb 1, 2022 · 9 comments

Comments

@bpowers
Copy link

bpowers commented Feb 1, 2022

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
Copy link
Owner

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

vitalyd commented May 18, 2022

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
Copy link
Owner

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.

1 similar comment
@fdeantoni
Copy link
Owner

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

vitalyd commented May 18, 2022

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

SGTM

Thanks @fdeantoni!

fdeantoni added a commit that referenced this issue May 21, 2022
containing the pbtime functions is now
no longer downloaded from the Prost
repo. Instead, the file is now committed
under `wkt-types/resources`. The `build.rs`
will use that instead.
@fdeantoni
Copy link
Owner

I hopefully addressed this issue with commit a269f4d.

@vitalyd
Copy link

vitalyd commented May 21, 2022

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

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

@fdeantoni
Copy link
Owner

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

vitalyd commented May 22, 2022

Sounds good - thanks again @fdeantoni!

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

No branches or pull requests

3 participants