Rust fix compilation for no_std targets

[jul-sh]

core2 is only used if the no_std feature is active. As of https://github.com/technocreatives/core2/commit/4072d2a09ba0d656571b1cd2ef6d95aa575c4a69 core2 includes std features by default. To achieve no_std compataibility this must be disabled. Fixes broken no_std builds on rustc 1.62.0-nightly.

[dbaileychess]

@CasperN

[dbaileychess]

@CasperN Could you take a look?

[CasperN]

Hello, I'm having trouble reproducing a build error at head. I ran

 cargo +nightly-2022-06-06 test --features no_std

in flatbuffers/tests/rust_usage_test and it came out fine

[dbaileychess]

@jul-sh Any update to @CasperN comments?

[jul-sh]

@jul-sh Any update to @CasperN comments?

cargo test doesn't ordinarily test no_std compatibility in dependency of libraries. This is likely why it looks fine there. Ref https://blog.dbrgn.ch/2019/12/24/testing-for-no-std-compatibility/

At a conf rn, can look into this in more detail later.

[CasperN]

Thanks for the explanation. Whenever you're free, can you add the no_std smoke test to tests/rust_usage_test, RustTest.sh, and RustTest.bat to prevent regressions?

[dbaileychess]

@jul-sh Any update to this?

[jul-sh]

@jul-sh Any update to this?

Haven't looked into yet, but should be able to in mid September

[jul-sh]

This is blocked by https://github.com/bbqsrc/thiserror-core2/pull/3 getting merged upstream, as flatbuffer depends on that crate. It needs (very minor) changes to ensure no_std compatibility. Unfortunately no news from the maintainer so far.

We could proceed with using a fork until https://github.com/bbqsrc/thiserror-core2/pull/3 is merged. (we did so in https://github.com/project-oak/oak).

[jul-sh]

@CasperN Could you approve running the workflows? Attempted the CI integration. Would be surprised if it works right away (Windows runs especially might be tricky), but would be good to be able to test it.

[CasperN]

@jul-sh thanks for the fix and test. I approved the workflows, looks like CI found something.

We don't publish often but we cannot publish our library on crates.io unless all our dependencies are in crates.io too. So I'm happy to submit this (once CI passes) so long the thiserror upstream change gets merged soon-ish

[bbqsrc]

This crate doesn't follow the appropriate idiom for how no_std is used in production in Rust projects.

The pattern is to have a std feature, on by default. Here's some highly popular crates demonstrating this pattern:

• serde
• regex
• rand
• libc

In your code, you then just write #![cfg_attr(not(feature = "std"), no_std)] to enable no_std, and #[cfg(not(feature = "std"))] for all the no_std implementations.

This has the benefit that you can easily disable std for all crates that use this pattern with default-features = false, and then in your std feature, enable all the std features in dependencies, eg:

default = ["std"]
std = ["thiserror-core2/std"]

[dbaileychess]

@jul-sh update to this given @bbqsrc comment?

[danlapid]

Seems to me like core2 and thiserror can be removed entirely. They are only used in verifier.rs to derive Error for one of the error types even though the other error type is implementing format regularly Less dependencies is always good imho and no_std will be as easy as adding the following code:

impl Error for InvalidFlatbuffer {
    fn source(&self) -> Option<&(dyn Error + 'static)> {
        match self {
            InvalidFlatbuffer::MissingRequiredField { .. } => None,
            InvalidFlatbuffer::InconsistentUnion { .. } => None,
            InvalidFlatbuffer::Utf8Error { error: source, .. } => Some(source),
            InvalidFlatbuffer::MissingNullTerminator { .. } => None,
            InvalidFlatbuffer::Unaligned { .. } => None,
            InvalidFlatbuffer::RangeOutOfBounds { .. } => None,
            InvalidFlatbuffer::SignedOffsetOutOfBounds { .. } => None,
            InvalidFlatbuffer::TooManyTables { .. } => None,
            InvalidFlatbuffer::ApparentSizeTooLarge { .. } => None,
            InvalidFlatbuffer::DepthLimitReached { .. } => None,
        }
    }
}

impl core::fmt::Display for InvalidFlatbuffer {
    fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result {
        match self {
            InvalidFlatbuffer::MissingRequiredField {
                required,
                error_trace,
            } => {
                writeln!(f, "Missing required field `{}`.\n{}", required, error_trace)?;
            }
            InvalidFlatbuffer::InconsistentUnion {
                field,
                field_type,
                error_trace,
            } => {
                writeln!(f,"Union exactly one of union discriminant (`{}`) and value (`{}`) are present.\n{}", field_type, field, error_trace)?;
            }
            InvalidFlatbuffer::Utf8Error {
                error,
                range,
                error_trace,
            } => {
                writeln!(
                    f,
                    "Utf8 error for string in {:?}: {}\n{}",
                    range, error, error_trace
                )?;
            }
            InvalidFlatbuffer::MissingNullTerminator { range, error_trace } => {
                writeln!(
                    f,
                    "String in range [{}, {}) is missing its null terminator.\n{}",
                    range.start, range.end, error_trace
                )?;
            }
            InvalidFlatbuffer::Unaligned {
                position,
                unaligned_type,
                error_trace,
            } => {
                writeln!(
                    f,
                    "Type `{}` at position {} is unaligned.\n{}",
                    unaligned_type, position, error_trace
                )?;
            }
            InvalidFlatbuffer::RangeOutOfBounds { range, error_trace } => {
                writeln!(
                    f,
                    "Range [{}, {}) is out of bounds.\n{}",
                    range.start, range.end, error_trace
                )?;
            }
            InvalidFlatbuffer::SignedOffsetOutOfBounds {
                soffset,
                position,
                error_trace,
            } => {
                writeln!(
                    f,
                    "Signed offset at position {} has value {} which points out of bounds.\n{}",
                    position, soffset, error_trace
                )?;
            }
            InvalidFlatbuffer::TooManyTables {} => {
                writeln!(f, "Too many tables.")?;
            }
            InvalidFlatbuffer::ApparentSizeTooLarge {} => {
                writeln!(f, "Apparent size too large.")?;
            }
            InvalidFlatbuffer::DepthLimitReached {} => {
                writeln!(f, "Nested table depth limit reached.")?;
            }
        }
        Ok(())
    }
}

It's basically what Derive(Error) creates

[CasperN]

I’m happy with removing the dependency if it’s getting in the way On Sep 27, 2022, 09:32 -0400, Dan Lapid @.***>, wrote:

Seems to me like core2 and thiserror can be removed entirely. They are only used in verifier.rs to derive Error for one of the error types even though the other error type is implementing format regularly Less dependencies is always good imho and no_std will be as easy as adding the following code: impl Error for InvalidFlatbuffer { fn source(&self) -> Option<&(dyn Error + 'static)> { match self { InvalidFlatbuffer::MissingRequiredField { .. } => None, InvalidFlatbuffer::InconsistentUnion { .. } => None, InvalidFlatbuffer::Utf8Error { error: source, .. } => Some(source), InvalidFlatbuffer::MissingNullTerminator { .. } => None, InvalidFlatbuffer::Unaligned { .. } => None, InvalidFlatbuffer::RangeOutOfBounds { .. } => None, InvalidFlatbuffer::SignedOffsetOutOfBounds { .. } => None, InvalidFlatbuffer::TooManyTables { .. } => None, InvalidFlatbuffer::ApparentSizeTooLarge { .. } => None, InvalidFlatbuffer::DepthLimitReached { .. } => None, } } }

impl core::fmt::Display for InvalidFlatbuffer { fn fmt(&self, f: &mut core::fmt::Formatter) -> core::fmt::Result { match self { InvalidFlatbuffer::MissingRequiredField { required, error_trace, } => { writeln!(f, "Missing required field {}.\n{}", required, error_trace)?; } InvalidFlatbuffer::InconsistentUnion { field, field_type, error_trace, } => { writeln!(f,"Union exactly one of union discriminant ({}) and value ({}) are present.\n{}", field_type, field, error_trace)?; } InvalidFlatbuffer::Utf8Error { error, range, error_trace, } => { writeln!( f, "Utf8 error for string in {:?}: {}\n{}", range, error, error_trace )?; } InvalidFlatbuffer::MissingNullTerminator { range, error_trace } => { writeln!( f, "String in range [{}, {}) is missing its null terminator.\n{}", range.start, range.end, error_trace )?; } InvalidFlatbuffer::Unaligned { position, unaligned_type, error_trace, } => { writeln!( f, "Type {} at position {} is unaligned.\n{}", unaligned_type, position, error_trace )?; } InvalidFlatbuffer::RangeOutOfBounds { range, error_trace } => { writeln!( f, "Range [{}, {}) is out of bounds.\n{}", range.start, range.end, error_trace )?; } InvalidFlatbuffer::SignedOffsetOutOfBounds { soffset, position, error_trace, } => { writeln!( f, "Signed offset at position {} has value {} which points out of bounds.\n{}", position, soffset, error_trace )?; } InvalidFlatbuffer::TooManyTables {} => { writeln!(f, "Too many tables.")?; } InvalidFlatbuffer::ApparentSizeTooLarge {} => { writeln!(f, "Apparent size too large.")?; } InvalidFlatbuffer::DepthLimitReached {} => { writeln!(f, "Nested table depth limit reached.")?; } } Ok(()) } } It's basically what Derive(Error) creates — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

[bbqsrc]

I'd note it's a separate issue from how the feature flags are specified.

And given the extremely small amount of code using core2, would highly recommend dropping it.

[tiziano88]

Friendly ping on this issue, it's currently still not possible to use flatbuffers as no_std in practice.

[danlapid]

Friendly ping on this issue, it's currently still not possible to use flatbuffers as no_std in practice.

#7553 was merged, you can try again, no_std should work now.

[dbaileychess]

@jul-sh There are merge conflicts now.

[CasperN]

This has been superseded with #7553