Document non-functional no-std for bitcoinconsensus#1352
Closed
tcharding wants to merge 1 commit intorust-bitcoin:masterfrom
Closed
Document non-functional no-std for bitcoinconsensus#1352tcharding wants to merge 1 commit intorust-bitcoin:masterfrom
tcharding wants to merge 1 commit intorust-bitcoin:masterfrom
Conversation
Member
|
concept ACK, though we should try to investigate upstream what's wrong |
apoelstra
previously approved these changes
Oct 27, 2022
Member
apoelstra
left a comment
There was a problem hiding this comment.
ACK 3988e4a1d2e2aa7b727b5c70d9bbe40d371718b0
Collaborator
|
What's the error message? Are you sure it's not some ARM issue instead of |
Member
Author
|
Its a build problem with building the C code, I did not do more than a cursory read of the output but here it is: |
3988e4a to
dc0ae1a
Compare
Member
Author
|
Rebased on master, no other changes. |
Collaborator
|
Then I guess it's a problem with the specific toolchain, not But this message looks quite weird anyway:
|
Collaborator
|
My guess is that the g++ there is just too old and the code is using some magic C++ craziness that the old version can't handle. If you could compare the versions that'd help investigating. Also I think we should print them out in CI for quicker access. |
Member
Author
I think you could be right but without building with |
Collaborator
|
If we can prove it then it's probably not even ARM build but specific version of GCC. If you have the tie to try out on your machine first that'd be great. |
Member
Author
|
Cool, leave it with me. |
Member
Author
|
Putting on ice till after release of 0.30.0 |
Member
Author
|
There is no "no-std" feature any more. |
Collaborator
|
There is a no-std build (by disabling default features). Does it no longer apply? |
Member
Author
|
Gee wiz, I was on fire yesterday. |
dc0ae1a to
2c1cc66
Compare
Member
Author
|
I don't have clock cycles to dig into this at the moment. We learned yesterday that LDK is using |
Member
Author
|
CI fail is unrelated to this PR. |
Member
|
Can we fix it in this PR though? I think it's a pretty small fix, it's just that some import needs to be feature gated. I think it's a bad interaction between #1225 ( |
Member
Author
I was going to rebase everything when #2279 merges. |
Member
Author
Oh this was the first PR I did this morning before I tackled #2279. I just wrote that comment to save you clicking on the CI failing job and investigating for me. |
The `rust-bitcoinconsensus` dependency is not able to be built with `--thumbv7m-none-eabi`. This means, at least as far as verifying our `no-std` build, `no-std` does not work when the `bitcoinconsensus` dependency is enabled. Instead of fixing the problem, document that it is broken.
2c1cc66 to
9955756
Compare
Member
Author
|
Rebased to pick up #2279, no other changes. |
apoelstra
approved these changes
Dec 12, 2023
Kixunil
reviewed
Dec 12, 2023
| bitcoinconsensus = { version = "0.20.2-0.5.0", default-features = false, optional = true } | ||
|
|
||
| # Please note, bitocinconsensus does not currently work with no-std. | ||
| bitcoinconsensus = { version = "0.20.2-0.5.0", optional = true } |
Collaborator
There was a problem hiding this comment.
This is not documenting anything and as I understand it, bitcoinconsensus may be able to compile on some no_std platforms but not all of them?
Member
Author
|
There are bigger fish to fry. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
rust-bitcoinconsensusdependency is not able to be built with--thumbv7m-none-eabi. This means, at least as far as verifying ourno-stdbuild,no-stddoes not work when thebitcoinconsensusdependency is enabled.Instead of fixing the problem, document that it is broken.
Context
cargo +nightly build --target thumbv7m-none-eabifails on every tag inrust-bitcoinconsensusback to v0.17.1