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

Run in unsupported architectures #22

Merged
merged 12 commits into from
Mar 27, 2025
Merged

Run in unsupported architectures #22

merged 12 commits into from
Mar 27, 2025

Conversation

come-maiz
Copy link
Contributor

Fixes #21.

@CLAassistant
Copy link

CLAassistant commented Mar 21, 2025

CLA assistant check
All committers have signed the CLA.

@prestonvanloon
Copy link
Member

How would this work? If you aren't building for amd64 or arm64, what assembly file would the compiler use?

@@ -28,8 +28,6 @@ import (
"unsafe"
)

func _hash(digests *byte, p [][32]byte, count uint32)
Copy link
Member

Choose a reason for hiding this comment

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

Why move this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make space for an empty implementation in the hash_unsupported. Otherwise there would be two declarations of this function.

@come-maiz
Copy link
Contributor Author

How would this work? If you aren't building for amd64 or arm64, what assembly file would the compiler use?

It uses sha256_1_generic.

Copy link
Collaborator

@potuz potuz left a comment

Choose a reason for hiding this comment

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

Nice thanks for the PR, I assume this would be triggered for RISCV, would you care to add a CI evaluator that would run on RISCV and fail without this PR? You can look in the github workflow of this same repo for it.

@come-maiz
Copy link
Contributor Author

would you care to add a CI evaluator that would run on RISCV and fail without this PR?

What about this? https://github.com/prysmaticlabs/gohashtree/pull/23/files

@potuz
Copy link
Collaborator

potuz commented Mar 27, 2025

Took the liberty to add an update of the index before downloading qemu because locally the tests would fail otherwise in my machine

Copy link
Collaborator

@potuz potuz left a comment

Choose a reason for hiding this comment

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

Thanks so much for this PR @come-maiz !

I've run the tests locally, with and without the PR and also ran them on qemu without the workflow.

@potuz potuz merged commit 7914b4a into prysmaticlabs:main Mar 27, 2025
8 checks passed
@come-maiz
Copy link
Contributor Author

Thank you @prestonvanloon and @potuz .

@come-maiz come-maiz deleted the riscv branch March 31, 2025 13:54
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.

Fails in unsupported architecture
4 participants