Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Add no_std embedded test in CI and fix for no_std #122

Merged
merged 2 commits into from
Apr 27, 2021

Conversation

RCasatta
Copy link
Contributor

@RCasatta RCasatta commented Apr 15, 2021

Introduce a test for no_std launched by the CI through qemu.

Firstly the test is failing due to the way core is exported, d1cfc2c fix that

Note: to use this version in rust-bitcoin, extern crate core is needed in rust-bitcoin

@RCasatta RCasatta changed the title add no_std embedded test in CI Add no_std embedded test in CI and fix for no_std Apr 15, 2021
@RCasatta RCasatta marked this pull request as ready for review April 15, 2021 07:56
@apoelstra
Copy link
Member

Neat! Can you reorder the commits so that they both pass during bisection?

@RCasatta
Copy link
Contributor Author

Neat! Can you reorder the commits so that they both pass during bisection?

yes, done

@RCasatta
Copy link
Contributor Author

I force pushed a second time because of the apt error in CI that happened a second time, will re-try in while

- name: Checkout
uses: actions/checkout@v2
- name: Set up QEMU
run: sudo apt install qemu-system-arm
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You usually want to run an apt update or apt-get update immediately before an install to make sure you're pulling the latest versions (which is why you're getting 404s in CI now).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, thanks

Copy link
Member

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I admit I don't know what half the embedded things here do, but they seem to be actually running correctly in CI and the main patch looks good.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK.

Agree with Matt that I don't really understand how this test works but it looks right. Good call fixing CI 404s - I just kicked it :)

@apoelstra apoelstra merged commit 5518293 into rust-bitcoin:master Apr 27, 2021
@@ -37,7 +37,7 @@
#![cfg_attr(all(test, feature = "unstable"), feature(test))]
#[cfg(all(test, feature = "unstable"))] extern crate test;

#[cfg(any(test, feature="std"))] pub extern crate core;
#[cfg(any(test, feature="std"))] extern crate core;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is technically a breaking change, although it seems to have been a bug in the first place to re-export core.

@sgeisler
Copy link
Contributor

I'm a bit unhappy that this was merged so quickly now that CI finally worked (I typically ignore PRs as long as they have failing CI). We should keep in mind that removing the core re-export is breaking. It's dumb for anyone to use it, but IntelliJ Rust for example has used it to auto-import stuff from std for me in the past. If someone did not notice and kept it that way that might be a problem.

@RCasatta
Copy link
Contributor Author

Probably by luck, and in the end it's better that the lack of apt update has been exposed, but the CI was working before the commit reordering https://github.com/rust-bitcoin/bitcoin_hashes/actions/runs/751200720

@sgeisler
Copy link
Contributor

Oh, I see, then sorry for not reviewing earlier. Just saw it today in the broken state and restarted the tests to come back later 😬

@apoelstra
Copy link
Member

@sgeisler I merged it so quickly only because it looked pretty-much trivial.

It did occur to me that the core re-export is technically breaking but I find it really hard to imagine it actually breaking anything in practice and I don't think we should bother doing a major version bump for it.

@sgeisler
Copy link
Contributor

I agree that normally it might be worth an exception to SemVer, but with auto importers being stupid at times I can totally see it break someone's project. We will see I guess 😅

@TheBlueMatt
Copy link
Member

This broke building rust-bitcoin, which now fails with eg

error[E0433]: failed to resolve: maybe a missing crate `core`?
  --> /home/matt/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/bitcoin-0.26.0/src/hash_types.rs:54:1
   |
54 | hash_newtype!(FilterHeader, sha256d::Hash, 32, doc="Filter header, as defined in BIP-157");
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ maybe a missing crate `core`?
   |
   = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

@RCasatta
Copy link
Contributor Author

RCasatta commented May 4, 2021

I wrote in the PR description (not now, 19 days ago) this would need extern crate core in rust-bitcoin

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

Successfully merging this pull request may close these issues.

4 participants