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

Improved CI and Builds #285

Closed
wants to merge 19 commits into from
Closed

Improved CI and Builds #285

wants to merge 19 commits into from

Conversation

phklive
Copy link
Contributor

@phklive phklive commented Mar 25, 2024

In this PR I propose improvements on the builds and CI.

After working on these PR's:

I have found new ways of improving ci and builds, hence adding them to the node.

bobbinth and others added 18 commits March 11, 2024 15:19
* ci: turn doc warnings into errors (#259)

* remove unnecessary comments

* Add script to check rust versions

* removed versions and editions except from root, updated script

* add back cargo make doc to ci

* Added section for testing

* Fixed typo

* Set versions to 0.1.0 + inherit all infos from workspace

* Fix use of versioning

* Set individual versions for crates

* Fixed nits, code organization and updated links

* Improve naming of ci job

* Fix formatting in contributing.md

* Add task to format not only check

* Reduced indent

---------

Co-authored-by: Augusto Hack <[email protected]>
* Working initial docker

* fmt

* added script

* Removed docker script + updated integration

* script works

* Need to install grpcurl

* Docker works

* Working Dockerfile

* ci: turn doc warnings into errors (#259)

* Removed makefile, removing start.sh file moving in Dockerfile

* Added bookworm + alpine + removed gcc + added caching

* Moved Dockerfile to node and added Makefile.toml

* cargo make works + builds dockerfile for node

* remove start script

* added comment regarding PID1

* Set labels at top of Dockerfile + added comments

* Added correct dependencies and explanation

* Volume works persisting db files between runs

* Added documentation

* Moved labels + enable mounting of miden-node.toml

* Added docker-run-node command + added build arguments

* Add git commit as arg to docker container

* Added spacing

---------

Co-authored-by: Augusto Hack <[email protected]>
* feat: implement nullifier tree wrapper over `Smt`

* refactor: move `NullifierTree` to separated file, renames ans small fixes

* fix: address review comments
* ci: turn doc warnings into errors (#259)

* Separated different CI jobs in their respective files

* Add next branch in ci

* Fix wrong naming in CI + use cargo make for doc

* Fixed badge + removed unnecessary additional file

* Change naming of ci and jobs

* Fix naming

* Fix comment

---------

Co-authored-by: Augusto Hack <[email protected]>
@phklive
Copy link
Contributor Author

phklive commented Mar 25, 2024

Depends on #282 to be merged.

@phklive phklive marked this pull request as ready for review March 25, 2024 12:23
@@ -19,7 +19,6 @@ jobs:
with:
profile: minimal
override: true
- name: Install cargo make
run: cargo install cargo-make
- uses: davidB/rust-cargo-make@v1
Copy link
Contributor

Choose a reason for hiding this comment

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

What benefit do we gain from using this custom version?

Copy link
Contributor Author

@phklive phklive Mar 25, 2024

Choose a reason for hiding this comment

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

It is now the new "Gold standard" for this use case, it's the most used one if you look at github actions pertaining to rust installation:

https://github.com/marketplace?category=&type=actions&verification=&query=sort%3Apopularity-desc+rust

Second most used rust github action

I need to look deeper into it for the full details of improvements, but at least it enables us to go away from an unmaintained action.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you referring to rust-cargo or rust-cargo-make? The latter one doesn't seem to be all the popular.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed in this PR I have added rust-cargo-make action. Taken this advice from the documentation of cargo-make here: https://github.com/sagiegurari/cargo-make/blob/d3c74835774b957cccba6ad3be057545d24729b6/README.md?plain=1#L3165

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not widely used indeed but speeds up the CI where the installation / compilation of the cargo-make crate takes some time. In case you prefer to use the install method I can go back to using it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it depends on how much of a difference it makes in CI. How much time are we saving by going to rust-cargo-make?

@phklive
Copy link
Contributor Author

phklive commented Apr 18, 2024

Closing this PR, rebased here: #325

@phklive phklive closed this Apr 18, 2024
@hackaugusto
Copy link
Contributor

@phklive you shouldn't need to close a PR and open it again after a rebase. You can do the rebase locally and force push to update your branch.

@hackaugusto hackaugusto deleted the phklive-improve-ci-and-builds branch April 18, 2024 13:51
@phklive
Copy link
Contributor Author

phklive commented Apr 18, 2024

@phklive you shouldn't need to close a PR and open it again after a rebase. You can do the rebase locally and force push to update your branch.

Yes absolutely, the rebase was large and slow because the PR stayed open long. Was quicker, but might not be a good ( accepted in Miden ) way to do it.

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