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

Standardise CI & Makefile across Miden repos #1342

Merged
merged 38 commits into from
Jul 17, 2024
Merged

Conversation

phklive
Copy link
Contributor

@phklive phklive commented May 28, 2024

In this PR I propose a standardisation of the CI and Makefile across Miden repositories.

With the following changes:

  • Cargo make to Make
  • CI using Cargo make to Make

Once this first one is approved I will port it to the following repos:

  • Crypto
  • Client
  • Compiler

@bobbinth
Copy link
Contributor

Let's hold off on this until we figure out how to deal with async features.

@bobbinth
Copy link
Contributor

I think we can pick this up again now. This would include:

  • Update the Makefile to be similar to the one in miden-base (but accounting for some unique tasks here).
    • As a side effect, we'll also need to switch to nextest here.
    • This will also require updating the docs (e.g., here) and potentially in other places.
  • Update the CI to be similar to the one in miden-base.
    • This may also require updating badges in the main readme.
  • Update the CONTRIBUTING file (probably just a few minor updates).

@phklive
Copy link
Contributor Author

phklive commented Jun 17, 2024

I think we can pick this up again now. This would include:

  • Update the Makefile to be similar to the one in miden-base (but accounting for some unique tasks here).

    • As a side effect, we'll also need to switch to nextest here.
    • This will also require updating the docs (e.g., here) and potentially in other places.
  • Update the CI to be similar to the one in miden-base.

    • This may also require updating badges in the main readme.
  • Update the CONTRIBUTING file (probably just a few minor updates).

Absolutely, working on it.

@phklive
Copy link
Contributor Author

phklive commented Jun 18, 2024

@bobbinth I removed the tests on Windows2022, let me know if we should add them back.

@phklive
Copy link
Contributor Author

phklive commented Jun 18, 2024

In the VM workspace rust version is 1.75 everywhere except in the assembly where it's 1.76.

I bumped everything to rust 1.78 and standardised all the cargo.tomls. Let me know if I should revert.

@phklive
Copy link
Contributor Author

phklive commented Jun 18, 2024

@bobbinth, I tried compiling with metal but I am receiving a large number of errors, I followed the instructions in the documentation I am not sure if I am doing something wrong or if the metal compilation is broken.

@phklive
Copy link
Contributor Author

phklive commented Jun 18, 2024

@bobbinth or @bitwalker to make sure that the Miden VM passes cargo doc I will need your help concerning this piece of documentation:

// ASSEMBLER
// ================================================================================================
/// The [Assembler] is the primary interface for compiling Miden Assembly to the Miden Abstract
/// Syntax Tree (MAST).
///
/// # Usage
///
/// Depending on your needs, there are multiple ways of using the assembler, and whether or not you
/// want to provide a custom kernel.
///
/// By default, an empty kernel is provided. However, you may provide your own using
/// [Assembler::with_kernel] or [Assembler::with_kernel_from_source].
///
/// <div class="warning">
/// Programs compiled with an empty kernel cannot use the `syscall` instruction.
/// </div>
///
/// * If you have a single executable module you want to compile, just call [Assembler::compile] or
/// [Assembler::compile_ast], depending on whether you have source code in raw or parsed form.
///
/// * If you want to link your executable to a few other modules that implement supporting
/// procedures, build the assembler with them first, using the various builder methods on
/// [Assembler], e.g. [Assembler::with_module], [Assembler::with_library], etc. Then, call
/// [Assembler::compile] or [Assembler::compile_ast] to get your compiled program.

Multiple comments have been made referencing non existing methods:

  • Assembler::with_kernel_from_source
  • Assembler::compile
  • Assembler::compile_ast
  • Assembler::compile_with_opts

And here:

/// [crate::Assembler::compile_in_context] API, which will use the provided context in place of

  • crate::Assembler::compile_in_context

I am sure we can easily fix these issues considering they should be pointing to existing code, I just need your support to understand which pieces of code you meant when writing the docs.

@bobbinth
Copy link
Contributor

In the VM workspace rust version is 1.75 everywhere except in the assembly where it's 1.76.

I bumped everything to rust 1.78 and standardised all the cargo.tomls. Let me know if I should revert.

1.78 sounds good - thank you! I think we'll probably skip 1.79 and will go straight to 1.80 once that's released in a couple of months.

@bobbinth, I tried compiling with metal but I am receiving a large number of errors, I followed the instructions in the documentation I am not sure if I am doing something wrong or if the metal compilation is broken.

@TheMenko will make a PR to fix it soon.

@bobbinth
Copy link
Contributor

@bobbinth I removed the tests on Windows2022, let me know if we should add them back.

Unless they complicate things too much, I'd prefer to keep them (would be good to make sure the VM works fine on Windows machines too). iirc, these were run only when merging into main.

@phklive
Copy link
Contributor Author

phklive commented Jun 18, 2024

In the VM workspace rust version is 1.75 everywhere except in the assembly where it's 1.76.
I bumped everything to rust 1.78 and standardised all the cargo.tomls. Let me know if I should revert.

1.78 sounds good - thank you! I think we'll probably skip 1.79 and will go straight to 1.80 once that's released in a couple of months.

@bobbinth, I tried compiling with metal but I am receiving a large number of errors, I followed the instructions in the documentation I am not sure if I am doing something wrong or if the metal compilation is broken.

@TheMenko will make a PR to fix it soon.

Should we wait for his PR to merge this?

This breaks all the --all-features commands. Which was requested by @plafer.

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a few comments inline.

Also, do we not need a config file for nextest? (i.e., something similar to what we have in miden-base).

Lastly, let's update the changelog.

.github/workflows/book.yml Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Comment on lines 35 to 41
For a faster build, you can compile with less optimizations, replacing `--profile optimized` by `--release`. Example:

```
cargo build --release --features concurrent,executable
make build-release
```

In this case, the `miden` executable will be placed in the `./target/release` directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can probably get rid of this option.

Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to update the CLI Interface section in this file.

@bobbinth
Copy link
Contributor

In the VM workspace rust version is 1.75 everywhere except in the assembly where it's 1.76.
I bumped everything to rust 1.78 and standardised all the cargo.tomls. Let me know if I should revert.

1.78 sounds good - thank you! I think we'll probably skip 1.79 and will go straight to 1.80 once that's released in a couple of months.

@bobbinth, I tried compiling with metal but I am receiving a large number of errors, I followed the instructions in the documentation I am not sure if I am doing something wrong or if the metal compilation is broken.

@TheMenko will make a PR to fix it soon.

Should we wait for his PR to merge this?

This breaks all the --all-features commands. Which was requested by @plafer.

Yes, let's wait for #1357 to be merged.

Regarding --all-features - is this because of the metal feature? How did it work before (i.e., we had this feature before this PR)?

@bobbinth
Copy link
Contributor

@phklive - now that #1357 has been merged, let's pick this up again.

@phklive
Copy link
Contributor Author

phklive commented Jun 21, 2024

@phklive - now that #1357 has been merged, let's pick this up again.

Will do.

@phklive
Copy link
Contributor Author

phklive commented Jul 16, 2024

@bobbinth or @bitwalker to make sure that the Miden VM passes cargo doc I will need your help concerning this piece of documentation:

// ASSEMBLER
// ================================================================================================
/// The [Assembler] is the primary interface for compiling Miden Assembly to the Miden Abstract
/// Syntax Tree (MAST).
///
/// # Usage
///
/// Depending on your needs, there are multiple ways of using the assembler, and whether or not you
/// want to provide a custom kernel.
///
/// By default, an empty kernel is provided. However, you may provide your own using
/// [Assembler::with_kernel] or [Assembler::with_kernel_from_source].
///
/// <div class="warning">
/// Programs compiled with an empty kernel cannot use the `syscall` instruction.
/// </div>
///
/// * If you have a single executable module you want to compile, just call [Assembler::compile] or
/// [Assembler::compile_ast], depending on whether you have source code in raw or parsed form.
///
/// * If you want to link your executable to a few other modules that implement supporting
/// procedures, build the assembler with them first, using the various builder methods on
/// [Assembler], e.g. [Assembler::with_module], [Assembler::with_library], etc. Then, call
/// [Assembler::compile] or [Assembler::compile_ast] to get your compiled program.

Multiple comments have been made referencing non existing methods:

  • Assembler::with_kernel_from_source
  • Assembler::compile
  • Assembler::compile_ast
  • Assembler::compile_with_opts

And here:

/// [crate::Assembler::compile_in_context] API, which will use the provided context in place of

  • crate::Assembler::compile_in_context

I am sure we can easily fix these issues considering they should be pointing to existing code, I just need your support to understand which pieces of code you meant when writing the docs.

@bobbinth, @bitwalker could you give me a hand on this one.

It's mainly the only thing we need before being able to PR.

@bobbinth
Copy link
Contributor

@bobbinth, @bitwalker could you give me a hand on this one.

I pushed a small commit which should fix the doc build.

@phklive
Copy link
Contributor Author

phklive commented Jul 16, 2024

I have been facing errors while trying to run the tests on Windows: https://github.com/0xPolygonMiden/miden-vm/actions/runs/9963373945/job/27529273724?pr=1342#step:5:272

To make sure that we can merge this PR without waiting more I have opened an issue to solve these problems: #1402

@phklive phklive marked this pull request as ready for review July 16, 2024 20:23
@phklive phklive requested a review from bobbinth July 16, 2024 20:23
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left a few comments inline - but all of them are pretty small.

Makefile Outdated Show resolved Hide resolved
.github/workflows/changelog.yml Outdated Show resolved Hide resolved
.github/workflows/build.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
miden/README.md Outdated Show resolved Hide resolved
@phklive phklive merged commit 56ccf86 into next Jul 17, 2024
9 checks passed
@phklive phklive deleted the phklive-standardise-make-ci branch July 17, 2024 13:01
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.

2 participants