Skip to content

rust: Allow for usage of flatbuffers in #![no_std] environment #6989

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

Merged
merged 1 commit into from
Jan 20, 2022

Conversation

MarWit
Copy link
Contributor

@MarWit MarWit commented Dec 13, 2021

Hey,

this PR introduces changes that allow for usage of flatbuffers in #![no_std] environments. The main difference to this PR is that instead of removing thiserror for no_std builds it uses core2 and thiserror_core2 in its place. The core2 crate needed some small changes, so I had to use fork for the time being.

@github-actions github-actions bot added the rust label Dec 13, 2021
@dbaileychess dbaileychess requested a review from CasperN January 14, 2022 22:40
@dbaileychess
Copy link
Collaborator

@CasperN Could you check this? This was an internal request.

@CasperN
Copy link
Collaborator

CasperN commented Jan 14, 2022

LGTM, sorry for the delayed review. Please pull from master, sign the CLA, and I'll merge when tests pass.

@MarWit
Copy link
Contributor Author

MarWit commented Jan 17, 2022

Rebased and CLA sorted out.

@CasperN
Copy link
Collaborator

CasperN commented Jan 17, 2022

Sorry @MarWit, one more thing, can you bump the minor version number in the flatbuffers/Cargo.toml?

Looks like the CI failure has to do with Java and Android, so it seem irrelevant to this PR.

@MarWit
Copy link
Contributor Author

MarWit commented Jan 18, 2022

Done. Just a little reminder that this change is using our forks of core2 and thiserror-core2 crates -- there is ongoing PR for them, so it could be addressed in subsequent patch, though.

@MarWit
Copy link
Contributor Author

MarWit commented Jan 18, 2022

core2 changes have been merged into the upstream; changed cargo manifest accordingly.

@CasperN
Copy link
Collaborator

CasperN commented Jan 20, 2022

Rust CI failed with 403, which is ...interesting

I think its fine to merge since the only change from the force push should've been the version number and cargo.toml.

@MarWit are you good to merge?

@MarWit
Copy link
Contributor Author

MarWit commented Jan 20, 2022

Yes. There is still thiserror_core2 that needs to be upstreamed, but this can be done later.

@CasperN CasperN merged commit aff818c into google:master Jan 20, 2022
@CasperN
Copy link
Collaborator

CasperN commented Jan 20, 2022

There is still thiserror_core2 that needs to be upstreamed, but this can be done later.

I'll publish to crates.io once that is upstreamed and updated in our Cargo.toml (they require dependencies to have published versions to publish).

@CasperN
Copy link
Collaborator

CasperN commented Jan 24, 2022

@MarWit when do you expect the upstreaming to happen?

@MarWit
Copy link
Contributor Author

MarWit commented Jan 25, 2022

I am currently waiting for the feedback from the crate's maintainer, so I sadly do not have any ETA right now. I will let you know when I will have any information.

@CasperN
Copy link
Collaborator

CasperN commented Jan 25, 2022

Gotcha, note that while we do not publish often, this is blocking publishing; so I'd prefer it gets resolved within a couple weeks - lest we risk slowing down another contributor. I hope that's plenty of time, but let me know if upstreaming gets blocked and we can figure something out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants