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.

Sign up for GitHub

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

Jump to bottom

feat: Add ARM build for Mac + cleanup artifacts #1837

Merged
merged 9 commits into from
Aug 28, 2023
Merged

feat: Add ARM build for Mac + cleanup artifacts #1837

merged 9 commits into from
Aug 28, 2023

Conversation

kevaundray
Copy link
Contributor

@kevaundray kevaundray commented Aug 26, 2023 •
edited
Loading

resolves #1836
resolves AztecProtocol/barretenberg#601
resolves #1841

Checklist:

Remove the checklist to signal you've completed it. Enable auto-merge if the PR is ready to merge.

  • If the pull request requires a cryptography review (e.g. cryptographic algorithm implementations) I have added the 'crypto' tag.
  • I have reviewed my diff in github, line by line and removed unexpected formatting changes, testing logs, or commented-out code.
  • Every change is related to the PR description.
  • I have linked this pull request to relevant issues (if any exist).

@kevaundray
Copy link
Contributor Author

kevaundray commented Aug 26, 2023 •
edited
Loading

Blocking: This needs testing.

Note:

  • We also merged a change to make barretenberg nightly builds, though I don't think those are being made. One reason could be missing the nightly tag.

  • Apple can probably cross compile using xcode, so you could use the same machine and just modify the xcode config. That would run sequentially though, so this way was chosen.

  • The naming for ubuntu artifacts should be changed to be similar to Mac naming. (barretenberg-x86_64-linux-gnu.tar.gz) [DONE]

  • We need to cd into the path when tarring the linux build to remove the directory structure (as is done with Mac) [DONE]

  • Once chore: compile minimal WASM binary needed for blackbox functions #1824 gets merged, the new acvm_backend.wasm should be added to the release workflow too. Don't think this should be blocking and can be done in a separate PR.

@kevaundray kevaundray changed the title chore: Add ARM build for Mac chore: Add ARM build for Mac + cleanup artifacts Aug 26, 2023
@kevaundray kevaundray mentioned this pull request Aug 26, 2023
Closed
6 tasks
@kevaundray
Copy link
Contributor Author

Added the changes that I mentioned in note into this PR. Happy to update if acvm_backend.wasm gets merged.

This is still blocked by testing.

@kevaundray
Copy link
Contributor Author

Tested this and it works on a fork I made (removed the deploy to npm step which should be inconsequential):

Screenshot 2023-08-27 at 12 40 43

@kevaundray kevaundray marked this pull request as ready for review August 27, 2023 11:41
@kevaundray
Copy link
Contributor Author

Notice, the linux build takes quite a lot of time -- this is because its building barretenberg three times, one for linux-gnu, one for single threaded wasm and another for multithreaded wasm. We can split out the wasm builds from the native linux-gnu build

@kevaundray
Copy link
Contributor Author

Splitting out the WASM build from the linux build saves us five minutes, due to it taking roughly five minutes to setup clang16. This is why the linux build also takes five minutes more than the mac build.

Five minutes is not a lot, though this also makes the code more modular, I think its a net positive. Could probably lower the time slightly more by building the WASM using Mac, but since this is not usually on the hot path, I don't think we need to do this. A bigger win would be to reduce the WASM compilation times in cmake, by using some form of caching.

Screenshot 2023-08-27 at 13 52 44

@kevaundray kevaundray changed the title chore: Add ARM build for Mac + cleanup artifacts feat: Add ARM build for Mac + cleanup artifacts Aug 27, 2023
kobyhallx
kobyhallx reviewed Aug 28, 2023
View reviewed changes
circuits/cpp/barretenberg/.github/workflows/publish.yml
@@ -71,7 +110,8 @@ jobs:
yarn install && yarn && yarn build

- name: Tar and GZip barretenberg.wasm
run: tar -cvzf barretenberg.wasm.tar.gz cpp/build-wasm/bin/barretenberg.wasm
working-directory: cpp/build-wasm/bin
run: tar -cvzf barretenberg.wasm.tar.gz barretenberg.wasm

- name: Setup Node.js
Copy link

Choose a reason for hiding this comment

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

Do we need that ? This should be already available on image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on image?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We currently use this for blackbox function solving, we natively download this from the releases and it will polyfill the pedersen, schnorr and scalar mul functionality

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though its always been there (previously because for windows builds we were using it for proving and verifying) -- probably okay to leave it as part of the build artifacts

Copy link

Choose a reason for hiding this comment

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

https://github.com/actions/runner-images/blob/main/images/linux/Ubuntu2204-Readme.md says node/yarn shoud be on GH Runner image, no need to setup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh you mean node, when I ran it, it kept saying it couldn't find it

@kevaundray
Copy link
Contributor Author

Seems to be a flaky aztec-node-test since I only modified the github action file for bb which doesn't get ran on this repo anyways

@kevaundray
Copy link
Contributor Author

since #1824 got merged, I'll sneak in the acvm_backend.wasm as a release artifact too

@kevaundray kevaundray enabled auto-merge (squash) August 28, 2023 15:16
@kevaundray kevaundray merged commit 270a4ae into master Aug 28, 2023
@kevaundray kevaundray deleted the kw/add-mac-arm branch August 28, 2023 15:21
@github-actions github-actions bot mentioned this pull request Aug 28, 2023
Merged
@kevaundray kevaundray mentioned this pull request Aug 28, 2023
Open
PhilWindle pushed a commit that referenced this pull request Aug 30, 2023
@github-actions
chore(master): release 0.1.0-alpha48 (#1804)
e89cd26 
🤖 I have created a new Aztec Packages release
---


##
[0.1.0-alpha48](v0.1.0-alpha47...v0.1.0-alpha48)
(2023-08-30)


### Features

* Add ARM build for Mac + cleanup artifacts
([#1837](#1837))
([270a4ae](270a4ae))
* broadcasting 'public key' and 'partial address' as L1 calldata
([#1801](#1801))
([78d6444](78d6444)),
closes
[#1778](#1778)
* Check sandbox version matches CLI's
([#1849](#1849))
([7279730](7279730))
* **docs:** adding some nitpick suggestions before sandbox release
([#1859](#1859))
([c1144f7](c1144f7))
* More reliable getTxReceipt api.
([#1793](#1793))
([ad16b22](ad16b22))
* **noir:** use `#[aztec(private)]` and `#[aztec(public)` attributes
([#1735](#1735))
([89756fa](89756fa))
* Recursive fn calls to spend more notes.
([#1779](#1779))
([94053e4](94053e4))
* Simulate enqueued public functions and locate failing constraints on
them
([#1853](#1853))
([a065fd5](a065fd5))
* Update safe_math and move to libraries
([#1803](#1803))
([b10656d](b10656d))
* Write debug-level log to local file in Sandbox
([#1846](#1846))
([0317e93](0317e93)),
closes
[#1605](#1605)


### Bug Fixes

* Conditionally compile base64 command for bb binary
([#1851](#1851))
([be97185](be97185))
* default color to light mode
([#1847](#1847))
([4fc8d39](4fc8d39))
* Disallow unregistered classes in JSON RPC interface and match by name
([#1820](#1820))
([35b8170](35b8170))
* Set side effect counter on contract reads
([#1870](#1870))
([1d8881e](1d8881e)),
closes
[#1588](#1588)
* Truncate SRS size to the amount of points that we have downloaded
([#1862](#1862))
([0a7058c](0a7058c))


### Miscellaneous

* add browser test to canary flow
([#1808](#1808))
([7f4fa43](7f4fa43))
* **ci:** fix output name in release please workflow
([#1858](#1858))
([857821f](857821f))
* CLI tests
([#1786](#1786))
([2987065](2987065)),
closes
[#1450](#1450)
* compile minimal WASM binary needed for blackbox functions
([#1824](#1824))
([76a30b8](76a30b8))
* fixed linter errors for `ecc`, `numeric` and `common` modules
([#1714](#1714))
([026273b](026273b))
* Refactor Cli interface to be more unix-like
([#1833](#1833))
([28d722e](28d722e))
* sync bb master
([#1842](#1842))
([2c1ff72](2c1ff72))
* sync bb master
([#1852](#1852))
([f979878](f979878))
* sync bb master
([#1866](#1866))
([e681a49](e681a49))
* typescript script names should be consistent
([#1843](#1843))
([eff8fe7](eff8fe7))
* use 2^19 as `MAX_CIRCUIT_SIZE` for NodeJS CLI
([#1834](#1834))
([c573282](c573282))


### Documentation

* Account contract tutorial
([#1772](#1772))
([0faefba](0faefba))
* Wallet dev docs
([#1746](#1746))
([9b4281d](9b4281d)),
closes
[#1744](#1744)

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot mentioned this pull request Sep 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kobyhallx kobyhallx kobyhallx left review comments

@Savio-Sou Savio-Sou Savio-Sou approved these changes

Assignees
No one assigned
Labels
None yet
Projects
A3
Archived in project
Milestone
No milestone
3 participants
@kevaundray @Savio-Sou @kobyhallx