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

feat: update build process #35

Merged
merged 11 commits into from
Jun 9, 2024

Conversation

matthewkeil
Copy link
Contributor

@matthewkeil matthewkeil commented May 28, 2024

Motivation
Had some issues with compilation of rust bindings for hashtree-js which is a node.js wrapper around this lib. The rust bindings did not build for Windows

Inclusions

  • Fix build.rs to compile on Windows
  • Update Makefile
    • Make platform selection more robust so its not necessary to pass in which CC for default situations
    • Allow for passing CC for cross-compilation situations
    • unify objarm and objx86 into OBJ_LIST dependent on platform
  • Add Dockerfile for compiling and testing rust bindings on linux-arm64
  • Update workflow.yaml to test for more cases of both the base lib and the rust bindings.

@CLAassistant
Copy link

CLAassistant commented May 28, 2024

CLA assistant check
All committers have signed the CLA.

Comment on lines 87 to +92
- name: install rustup
if: ${{ !matrix.settings.use-docker }}
run: |
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs > rustup-init.sh
sh rustup-init.sh -y --default-toolchain none
rustup target add ${{ matrix.target }}
rustup target add ${{ matrix.settings.target }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a note, this step can be replaced with an action

      - name: Install
        uses: dtolnay/rust-toolchain@stable
        if: ${{ !matrix.settings.use-docker }}
        with:
          toolchain: stable
          targets: ${{ matrix.settings.target }}

Comment on lines +67 to +70
# - name: Windows arm64
# target: aarch64-pc-windows-msvc
# runner: windows-latest
# use-docker: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a hard time finding a container that was easy to setup for building Windows on arm64. I suppose if someone wants in the future should be easy enough to turn on but ran out of time and just commented this case out.

@matthewkeil
Copy link
Contributor Author

I also got x86 musl building natively and with rust, but not sure if you want to include that. I had trouble with arm but also didnt try super hard because its sort of a minority architecture. Could be turned on but wasnt sure of the implications relative to the assembly in the lib. I think they are compatible and its just the build tooling and abi layout that changes. Honestly, I'm not super familiar with musl is why I removed it.

https://github.com/matthewkeil/hashtree/actions/runs/9272182080/job/25509333449

Screenshot 2024-05-28 at 10 41 29 PM

@arnetheduck
Copy link
Contributor

are you using the rust bindings in js?

@matthewkeil
Copy link
Contributor Author

matthewkeil commented Jun 5, 2024

are you using the rust bindings in js?

Yes @arnetheduck. In ChainSafe/hashtree-js#3

Are using a submodule from the branch this PR is built from but will move to the crate once its published.

Would you mind rerunning that linux/arm64 build that was hanging as a sanity check they are all working here. I ran in my fork but would love to see the green check here too

@arnetheduck
Copy link
Contributor

(sorry for off-topic)

@matthewkeil ah curious - I would have thought that the C files are both easier to use and "thinner" in terms of compiler support that's needed to get to the finish line - what motivates the use of rust in this case?

@matthewkeil
Copy link
Contributor Author

(sorry for off-topic)

@matthewkeil ah curious - I would have thought that the C files are both easier to use and "thinner" in terms of compiler support that's needed to get to the finish line - what motivates the use of rust in this case?

Our bindings were built with napi-rs in rust for this project, as a trial, to see how that library did from a DX perspective. Building JS binding with c++ is a bear of a job. Big picture, if we can move to rust we think it'll be better (at least we hope).

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 one, after offline discussions this looks good to me

@potuz potuz merged commit 1713941 into prysmaticlabs:main Jun 9, 2024
14 checks passed
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.

5 participants