-
Notifications
You must be signed in to change notification settings - Fork 105
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
build script was missing platform-specific deps #243
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM, though I'd like a second opinion from @UebelAndre on this if available.
This does appear to be a regression so this LGTM! Thanks for fixing that 😄 🙏 |
707768d
to
063449e
Compare
LGTM! Thanks for updating the PR 😄 |
@acmcarther will have to do final sign off though |
063449e
to
0edd153
Compare
The trailing comma was missing in the targeted deps case; I've moved it outside the endif to fix this. |
LGTM! Thanks for putting this together! |
Prior to #212 being merged, the following in Cargo.toml was included the resulting BUILD file:
This patch makes it work again, gated with a platform check, eg: