cstr icon indicating copy to clipboard operation
cstr copied to clipboard

Support no_std (Rust 1.64 and later) (Take 2)

Open chrysn opened this issue 3 years ago • 5 comments

Closes: https://github.com/upsuper/cstr/issues/12 (see previous discussion there) Replaces: https://github.com/upsuper/cstr/pull/13

Based on the discussion in #13 I've revisited some assumptions, and removed parts of the PR that were based on my wrong understanding of proc-macro crates.

The patches are now in a sequence that, by running CI on them, shows that the test for no-std behavior alone fails, whereas the second patch fixes it.

As before, this breaks support for Rust versions < 1.64, so CI will report all but the "Build on no_std" tests as failing -- so this only makes sense to merge at the point when 1.64 is stable (at which the "nightly" in the test can be switched over).

chrysn avatar Jul 31 '22 18:07 chrysn

Relevant build reports, now that I've made the github actions happy:

  • When only the new test is added, https://github.com/chrysn-pull-requests/cstr/runs/7600662935?check_suite_focus=true#step:4:18 shows how running in a no_std environment fails.
  • After std is switched to core, https://github.com/chrysn-pull-requests/cstr/runs/7600665038?check_suite_focus=true shows how the same test passes.

(But of course everything else is not yet on 1.64, so several other tests fail).

chrysn avatar Jul 31 '22 18:07 chrysn

It's certainly nice to have ready-made actions, but I'm not sure this will save more than the rustup target add step -- the dedicated test crate will still be necessary, and only that can be built on the thumb target.

chrysn avatar Aug 06 '22 12:08 chrysn

No, because trybuild already generates test crate automatically. If it can generate a test crate that uses a specific target, the test would be runnable with plain cargo test, and the only change needed for CI task is to install the needed target.

upsuper avatar Aug 07 '22 00:08 upsuper

Short of patching trybuild (which I don't have any capacity to do right now), is there anything I can do to advance this issue?

chrysn avatar Sep 10 '22 14:09 chrysn

Nothing you need to do. I'll probably just merge it when 1.64 reaches stable.

upsuper avatar Sep 11 '22 12:09 upsuper

With 1.64 released, I've been running a round of CI tests at https://github.com/RIOT-OS/rust-riot-wrappers/pull/10#partial-pull-merging in parallel to local tests -- unsurprisingly, there were no surprises.

I've added one commit updating all mentions of required Rust versions to reflect the strategy indicated in the last comment, and taken the liberty to add a rust-version = "1.64" as that enhances the user experience in the error case.

Would you consider making a cstr release soon after merging this?

chrysn avatar Sep 22 '22 15:09 chrysn

New version published.

upsuper avatar Sep 24 '22 12:09 upsuper

Thanks a bunch!

chrysn avatar Sep 24 '22 17:09 chrysn