0xPolygonMiden / miden-vm

STARK-based virtual machine
MIT License
611 stars 148 forks source link

Standardise CI & Makefile across Miden repos #1342

Open phklive opened 1 month ago

phklive commented 1 month ago

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

With the following changes:

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

bobbinth commented 1 month ago

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

bobbinth commented 3 weeks ago

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

phklive commented 2 weeks ago

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 commented 2 weeks ago

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

phklive commented 2 weeks ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

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

https://github.com/0xPolygonMiden/miden-vm/blob/bf885b7bf306d283c23dba44efe0dbd2db11efe6/assembly/src/assembler/mod.rs#L61-L85

Multiple comments have been made referencing non existing methods:

And here:

https://github.com/0xPolygonMiden/miden-vm/blob/bf885b7bf306d283c23dba44efe0dbd2db11efe6/assembly/src/assembler/context.rs#L21

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 commented 2 weeks ago

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 commented 2 weeks ago

@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 commented 2 weeks ago

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.

bobbinth commented 2 weeks ago

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 commented 2 weeks ago

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

phklive commented 2 weeks ago

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

Will do.