WIP: feedback needed - no_std

[whalelephant]

Hi,

Referring to #24, this PR supports no_std by

1. removing the date-encoding-macros - proc_macro crate used required std, as a result some the const required compile time eval are now using the suggested lazy_static with spin_no_std
2. using alloc directly
3. Currently pointing to a base-x repo, no_std branch forked from published crates. PR here

Just want to start the discussion as to how best to proceed to get this PR merge. Thanks!

[Ericson2314]

You shouldn't use spin_no_std with this package's std feature. Spin locks are almost always bad when there is another scheduler.

[Ericson2314]

Also, I'd say rather than using lazy_static, which has runtime overhead which might matter in small no-std environments. I would just (maybe conditionally) define the constants the results of evaluation for no-std, and test that those hand-written constants equal to the original compile-time-evaluated definitions.

[whalelephant]

There are currently const &str for base10 and some base58 which are then used as base input for base-x encoding / decoding when implementing BaseCodec. But it looks like base-x shouldn't be used for base16 (hex), base32, or base64 as it is not compliant. I am not sure if this is what you had in mind or I might have misunderstood. Please could you clarify? thanks!

[mriise]

@whalelephant lazy_static really isn't needed at all, as we know everything we need to make everything purely compile.
I made another fork of this PR to change that out for macros here.
@vmx mind taking a look at this?

[vmx]

Could we just use core directly, without special-casing no_std?

So this would just be use core::fmt and further below core::result::Result<T, Error>`.

[vmx]

@whalelephant sorry for the lack of feedback.

I am not sure if this is what you had in mind or I might have misunderstood.

Yes that's correct.

--

@mriise when I look into the diff between between this master branch and yours master...mriise:nstd, it seems that src/encoding.rs didn't really change (except for one import), it is just a re-ordering. So I think it makes sense to change it back to the original order to have minimal changes.

I also think the extern crate data_encoding_macro; is not needed.

I've also left some comments on this PR which are still part of yours.

[whalelephant]

Hi @vmx thanks for the feedback regarding using core.

Agree with not using lazy_static but I did not find a way to move forward, the data_encoding_macro does not support no_std and from the above comment I left earlier ... it looks like base-x shouldn't be used for base16 (hex), base32, or base64 as it is not compliant. What am I missing?

[mriise]

@whalelephant while it does say that in the doc, forcing #!(no_std) had no complaints and all tests were passing. sadly I don't know enough to know if this holds true for actual production use.

[whalelephant]

@mriise I am not very familiar with it either, but running your branch with cargo-nono gave the following results

▶ multibase: ✅
base-x: ✅
data-encoding: ✅
data-encoding-macro: ❌
  - Did not find a #![no_std] attribute or a simple conditional attribute like #[cfg_attr(not(feature = "std"), no_std)] in the crate source. Crate most likely doesn't support no_std without changes.

[mriise]

@whalelephant I've never seen that tool before, looks useful.
However, to test if it was actually no_std capable I compiled with the flag and ran tests to make sure.The reason the crate docs say it's not ready for no_std is that it uses the primary crate as a build dependency which is fine.

[wigy-opensource-developer]

The reason the crate docs say it's not ready for no_std is that it uses the primary crate as a build dependency which is fine.

Is it possible that a flag in a dev-dependency used in the build leaks into the production crate? There is an unstable solution for that in cargo, but on stable build still pollutes production dependency flags.

[vmx]

that it uses the primary crate as a build dependency which is fine

I had a look. I guess it's referring to the use of the internal macro, though it's strange that if it is only a build dependency, it's still listed under the normal dependencies.

Though, even if it would be a dev dependency, it won't change the fact that std would leak into the library.

[mriise]

So after doing a few hours of playing around with the data-encoding library, it turns out while normally std does leak into the proc macro as the proc_macro crate depends on std, the compiler seems to be able to understand and build the proc_macro with std before using it if you set the no_std attribute inside the lib.

I made sure this worked properly by building with thumbv6m-none-eabi as the target. While this still has some more work to do to properly scrutinize and clean up after testing, it definitely seems possible.

[ia0]

Sorry if the missing no-std support for data-encoding-macro has delayed this pull-request. Thanks @mriise for reaching out. Let me know if I can help further (I'll wait confirmation to know if a release is needed otherwise I'll wait to do further improvements before releasing which may take up to a month).

[mriise]

@ia0 no worries! Thanks for the quick response to get this through!

@whalelephant @vmx would it be preferable to make PRs on whale's fork or to make a separate PR on this repo?

[vmx]

I'll wait confirmation to know if a release is needed otherwise I'll wait to do further improvements before releasing which may take up to a month

@ia0: Waiting sounds good to me, there is no immediate rush (I hope others agree, if not, please comment).

@whalelephant @vmx would it be preferable to make PRs on whale's fork or to make a separate PR on this repo?

I have no preference, whatever is easier for you @mriise.

[whalelephant]

it turns out while normally std does leak into the proc macro as the proc_macro crate depends on std, the compiler seems to be able to understand and build the proc_macro with std before using it if you set the no_std attribute inside the lib

Awesome!

Happy for this to be closed and @mriise can work on the latest code. 👍

[ia0]

Actually I managed to do the other improvements I wanted (essentially support for no-alloc in addition to no-std). So you can use version 0.1.9 in no-std environment now. You might need to add the following to your .cargo/config.toml until rust-lang/cargo#7915 is fixed though:

[unstable]
features = ["host_dep"]

[mriise]

@whalelephant no_std compatable with #30 🎉
thanks for the hard work!