Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable RISC-V build #8

Merged
merged 5 commits into from
Jul 26, 2024
Merged

Enable RISC-V build #8

merged 5 commits into from
Jul 26, 2024

Conversation

haurog
Copy link
Contributor

@haurog haurog commented Jul 24, 2024

Building lighthouse on RISC-V fails building this library which has cpufeatures as a hard dependency even though it only uses it on x86-64. See for the bug report and analysis in cpufeatures github: RustCrypto/utils#1087

Tested fix on a RISC-V board (lichee Pi 4a) and works. It also still builds on my x86_64 PC.

To incorporate this change in Lighthouse itself we need a new release of this package and updates in the lighthouse Cargo.toml as well.

@michaelsproul
Copy link
Member

I think we may be able to get Lighthouse building without this change, because the dep is already marked optional. Happy to make this change to make it a bit easier though

@haurog
Copy link
Contributor Author

haurog commented Jul 24, 2024

I tried to build the stable branch of Lighthouse and it failed with trying to build the cpufeatures package which is a dependency of ethereum_hashing. Is the optional marking new and only on a different branch? I am asking because I have not seen that it is optional, but to be honest, my knowledge of how Rust handles these more complex dependencies is rather limited.

@haurog
Copy link
Contributor Author

haurog commented Jul 24, 2024

Ah, you obviously mean that cpufeatures is optional in the ethereum_hashing Cargo.toml. Unfortunately that does not seem to work, as the build of this package fails on my RISC-V board without explicitly limiting this dependency to a specific plattform. No idea how the optional flag is supposed to work though.

@haurog
Copy link
Contributor Author

haurog commented Jul 24, 2024

Now that I read into optional attribute and the features it sounds like removing detect-cpufeature from the default might be a better option:

diff --git a/Cargo.toml b/Cargo.toml
index 82a941e..e56f1b0 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -23,6 +23,6 @@ rustc-hex = "2"
 wasm-bindgen-test = "0.3.33"
 
 [features]
-default = ["zero_hash_cache", "detect-cpufeatures"]
+default = ["zero_hash_cache"]
 zero_hash_cache = ["lazy_static"]
 detect-cpufeatures = ["cpufeatures"]

Building still works on both platforms x86_64 and RISC-V and I guess it makes more sense with the features already defined and in place.

@michaelsproul
Copy link
Member

michaelsproul commented Jul 24, 2024

Yeah, and we could try this out in Lighthouse immediately by changing the ethereum_hashing dep to build with default-features = false.

I suspect there will be other deps that block a successful RISC-V build, but we will see

@michaelsproul
Copy link
Member

Actually I think the default features thing won't work, so yeah, we will need something like this change a new release. I can help get that out tomorrow

@haurog
Copy link
Contributor Author

haurog commented Jul 24, 2024

I tested a full lighthouse build once with my locally fixed ethereum_hashing repo and also once with the vanilla lighthouse stable version but using the --no-default-features flag. Both still failed at the cpufeatures build step. If I have not done anything stupid we think the issue is that cpufeatures is references by around a dozen of other cargo packages within lighthouse itself. Will have to check which one might be the culprit now.

Cargo.toml Outdated Show resolved Hide resolved
@michaelsproul
Copy link
Member

Lots of Lighthouse deps depend on cpufeatures, you can see the full extent of it in the Cargo.lock:

https://github.com/sigp/lighthouse/blob/stable/Cargo.lock

@michaelsproul
Copy link
Member

Some of them like aes have it as a mandatory dependency:

@lazyprogrammerio
Copy link

Would be great to have this PR merged, thanks. slowly, we will try to fix the proper upstream issues, but first we need to see how far we can get with the current status quo of the risc-v assembly based support.

@michaelsproul michaelsproul merged commit 3bfe7ec into sigp:main Jul 26, 2024
4 of 5 checks passed
@michaelsproul
Copy link
Member

michaelsproul commented Jul 26, 2024

Merged!

I had to make a bunch of other changes to actually prevent the cpufeatures crate from being compiled:

  • Removed the detect-cpufeatures default feature, as it was enabling cpufeatures even on architectures other than x86_64.
  • Feature-gated the sha2 crate so that it only gets compiled on x86_64. The sha2 crate depends on cpufeatures in a non-optional way, so it needs to be cut out to compile on RISC-V.

This will be a breaking change due to the removal of a feature, so I'll cut a new v0.7.0 release.

@haurog
Copy link
Contributor Author

haurog commented Jul 26, 2024

That is great. Thanks for fixing and including this PR. As mentioned we are looking into getting lighthouse to compile on risc-v and there are a lot smaller and larger issues along the way which will probably make it necessary to update a quite few dependencies in Lighthouse. We will open an issue over there as soon as we have a good overview of how to get a build running.

@michaelsproul
Copy link
Member

Sounds good. We are in the process of a big dep upgrade to switch from ethereum-types to alloy, so I'll make sure we bump ethereum_hashing as part of that

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

Successfully merging this pull request may close these issues.

4 participants