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

General fixes to protobuf bindings, depreciation warnings and publish.sh #504

Merged
merged 5 commits into from
Jul 26, 2020

Conversation

sashahilton00
Copy link
Member

@sashahilton00 sashahilton00 commented Jul 24, 2020

Should fix the rust docs error mentioned by @ashthespy in Gitter. Also updates the publish script with some minor tweaks to deal with lock file and issues with remote server timing.

Have also fixed the depreciation warnings by removing redundant description methods and changing description() instances to use to_string()

N.B. This is non-functional atm, build.rs/lib.rs needs further updating.

Fixes error description depreciation warnings and removes unnecessary parentheses
@sashahilton00 sashahilton00 changed the title Update build.rs & publish.sh with minor fixes Update build.rs & publish.sh with minor fixes, fix depreciation warnings Jul 24, 2020
@ashthespy
Copy link
Member

ashthespy commented Jul 24, 2020

@sashahilton00 have you seen the upstream issue and this particular "solution" stepancheg/rust-protobuf#324 (comment)?

That being said, I am not sure why we need to change this for docs.rs at all. The base issue as I understood it is the docs builder doesn't allow write access to the src directory. But don't we check in the generated files to git to begin with? So build.rs should not have to write anything when we publish on crates.io. Or am I missing something?

@sashahilton00
Copy link
Member Author

I saw a solution similar to that on SO. I foolishly thought I could just change the destination from src to OUT_DIR and it would work, but it turns out it's not that simple, hence the fist commit in this or doesn't fix the build.rs issue. I've started the solution mentioned upstream, not at home today though so might have to wait a bit, feel free to jump ahead of me on that if you want it fixed as soon as possible.

@sashahilton00
Copy link
Member Author

Ok perhaps I'm just having a bad day but I'm struggling to get this build.rs script working. I keep hitting a must reside in include path error whilst trying to include these files. Will take a look again tomorrow but I'm missing something currently.

@sashahilton00
Copy link
Member Author

sashahilton00 commented Jul 25, 2020

And as I post that, I get it working. C'est la vie. I'm not familiar with the rust docs build process, but I believe this should fix it. Essentially it creates the proto bindings at compile time as opposed to retaining the generated bindings in the repo. This will cause a slight performance hit, but the main brrier that we faced previously whereby the user was required to have protobuf installed on the system is no longer an issue now that we use the pure rust version of protobuf. Thus I figured generating at compile time is not longer such a burden. Feel free to disagree/amend if you think these assumptions are poor.

@ashthespy I believe this should fix the docs issue, but give it a once over. Will squash these commits before merging if they solve the issue.

@sashahilton00 sashahilton00 changed the title Update build.rs & publish.sh with minor fixes, fix depreciation warnings General fixes to protobuf bindings, depreciation warnings and publish.sh Jul 25, 2020
@sashahilton00
Copy link
Member Author

Have done my best to test this in a sandbox using Bubblewrap, it should fix the docs.rs issue. If there are no objections I'll merge this.

@sashahilton00 sashahilton00 merged commit 4886d4e into dev Jul 26, 2020
@sashahilton00 sashahilton00 deleted the build-fixes branch July 29, 2020 15:45
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