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

bring ARM support #2130

Closed
6 tasks
ilgooz opened this issue Mar 1, 2022 · 16 comments · Fixed by #2247
Closed
6 tasks

bring ARM support #2130

ilgooz opened this issue Mar 1, 2022 · 16 comments · Fixed by #2247
Assignees
Labels
bounty type:feat To implement new feature.

Comments

@ilgooz
Copy link
Member

ilgooz commented Mar 1, 2022

Starport CLI should be able to run on ARM x64 Linux/Darwin platforms.

CLI is written in Go, compiling it to these platforms is pretty straightforward by using the go build tool.

But CLI has:

binaries embed inside.

These binaries copied to the filesystem to execute their commands when running some certain Starport CLI commands.

For example, nodetime is being used:

  • To perform JS code generation when you run starport g vuex
  • Expose confio/ts-relayer as a backend to starport relayer command.

See related scripts that creates these binaries here (currently, protoc binaries are added manually).

For ARM support, we should also have ARM darwin & linux binaries of these programs embed into Starport CLI.

Make sure that:

  • nodetime bundled by using vercel/pkg. Use the latest version of vercel/pkg to create binaries for nodetime.
  • Binaries shouldn't be directly committed to the repository because of security reasons. Create a single CI workflow to download/build them (nodetime, protoc, protoc-gen-dart) automatically. This workflow should create a pull request after or update the existing one only if there is a diff.
  • Update the release workflow to also release Starport CLI binaries for ARM Linux and Darwin. Nightly and Production.

To test things make sure that:

  • starport relayer commands are running OK.
  • Run starport g vuex --proto-all-modules in the root of a blockchain app, it has to be running OK. You should observe changes in the vue/src/store/generated dir of the blockchain.
  • starport g dart should be running OK.
@ilgooz ilgooz added the type:feat To implement new feature. label Mar 1, 2022
@ilgooz ilgooz changed the title Add ARM support bring ARM support Mar 3, 2022
@ilgooz ilgooz added the bounty label Mar 3, 2022
@gjermundgaraba
Copy link
Contributor

gjermundgaraba commented Mar 11, 2022

@ilgooz I've been doing some experimentation on this one as I was considering to apply for the bounty on it. I've got a good overview of the work, but there is one show-stopper right now, which is the protoc-gen-dart. Nodetime was possible to cross-compile with some libraries installed on Linux, but dart does not have this capability for their executable files (https://dart.dev/tools/dart-compile#exe see the part of about cross-compilation).

If Github Actions had ARM versions of mac we might be able to get it to work, but it doesn't. There is a chance that we could build for linux arm64 with qemu, but I haven't tried that yet.

What are you thinking here as a solution? protoc-gen-dart doesn't seem to have any prebuilt binaries unfortunately. I could of course build them manually on each architecture by spinning up some VM's here and there, but would violate Binaries shouldn't be directly committed to the repository because of security reasons.

If you have any Mac M1 GitHub Runners laying around (or willing to create them on scaleway or something) we could use that too of course, but would require a lot more infra setup that I cannot do from this end.

An alternative approach would be to build with a kernel build, but that would make dart a requirement for running the generation (since it doesn't output a full self-contained executable). However, if you are generating dart code anyway, it might not be too crazy to assume dart as a required dependency?

Edit: After testing more I also realized something was wrong nodetime after upgrading pkg to the newest version. After a lot of digging I found out the problem was this issue: vercel/pkg#1130. The problem has since been fixed but we need a new release from pkg (I have verified this by pkg directly from their main and everything works again).

@gjermundgaraba
Copy link
Contributor

Another question: is it desirable to update more libraries (in the gen-nodetime package.json) at the same time here? There is also a new LTS version of node (currently used 14, LTS now 16).

I also have a question to understand a bit more context: As part of codegen we also run typescript compilation (tsc from nodetime), but I am a little confused why this is part of codegen as it seems to me to be more of a build step that could be included in the generated vue project instead?

@ilgooz
Copy link
Member Author

ilgooz commented Mar 16, 2022

This is a so valuable investigation! I really appreciate that.

About building the Dart plugin for ARM; would it solve if we create a Github Runner from an ARM machine? Do we need a Linux ARM or a Mac ARM or both?

We can create machines and add to Starport repo as Github Runners. And we can create some more machines and share the SSH keys with you for your CI testing purposes.

Please let me know how do you want to proceed on this, I credit your decision. Which solution we go with, it's better to not require our users to install any dependencies other than the Go installation itself.

cc @helder-moreira @shahbazn

Edit: After testing more I also realized something was wrong nodetime after upgrading pkg to the newest version. After a lot of digging I found out the problem was this issue: vercel/pkg#1130. The problem has since been fixed but we need a new release from pkg (I have verified this by pkg directly from their main and everything works again).

This sounds good, if that makes sense we can build it on CI from the source for the time being.

Another question: is it desirable to update more libraries (in the gen-nodetime package.json) at the same time here? There is also a new LTS version of node (currently used 14, LTS now 16).

That would be perfect.

I also have a question to understand a bit more context: As part of codegen we also run typescript compilation (tsc from nodetime), but I am a little confused why this is part of codegen as it seems to me to be more of a build step that could be included in the generated vue project instead?

JS transpilation planned to be dropped. @marinhoarthur is redesigning our VueJS architecture and I think he was aligned for the same outcome. If you're interested, we can also remove it now.

@ilgooz
Copy link
Member Author

ilgooz commented Mar 16, 2022

@bjaanes you have been assigned to this issue. Thank you for your bounty participation!

@gjermundgaraba
Copy link
Contributor

Great! I am all over this (i.e. I confirm my participation).

I am finishing up some testing with the hosted GitHub Runners and qemu emulation now to see how far I'll get with those for building the arm binaries without any self-run ones. It is likely not to be excellent for native arm64 for OSX. I have some thoughts on that too, but
I'll get back to you with comments, questions and my recommendations and we can decide the best course of action from there.

@ilgooz
Copy link
Member Author

ilgooz commented Mar 16, 2022

Gjermund, that's perfect!

I requested two Mac M1s just in case, to be attached to this repo as a GH runner and one for your testing purposes. They expected to be ready within 48hours.

@gjermundgaraba
Copy link
Contributor

I requested two Mac M1s just in case

I think that is a good idea, using qemu for this is likely to require some hacky OSX images with a bit "shady" licencing.

Do we need a Linux ARM or a Mac ARM or both?

It looks like it works to build the arm binaries for linux with qemu, but it is a bit slow (7+ minutes). Having separate runners for linux arm64 would also simplify the github tasks a bit to run in a matrix (which is not really great right now).

This sounds good, if that makes sense we can build it on CI from the source for the time being.

Great, that should work. I am also reaching to the team there to get an ETA on a new stable release. Looking at their release history it shouldn't be too far into the future.

If you're interested, we can also remove it now.

I will certainly look into it and see if it works well. I think it would make a lot of sense. If there is any context or additional info I should know about just let me know.

One last question for right now is about the workflows. The issue mentions "Create a single CI workflow". This is certainly doable, but I the trigger might differ for each part because:
Building the nodetime binary creates nondeterministic bytecode. This is mentioned in the pkg docs as well, and can in theory be avoided by building with a --no-bytecode flag. I've spent some time trying to make this work, but it seems like because we include binaries like tsc the build fails with an error that those files include bytecode. I will try to figure this out, but at least for nodetime it would make sense that the trigger for building a new binary comes from changes in that part of the code (updates to package.json, the nodetime file etc).

Got any thoughts on that?

@ilgooz
Copy link
Member Author

ilgooz commented Mar 17, 2022

It looks like it works to build the arm binaries for linux with qemu, but it is a bit slow (7+ minutes). Having separate runners for linux arm64 would also simplify the github tasks a bit to run in a matrix (which is not really great right now).

Do we specifically need Linux? We're already preparing M1 machines. Which one do you think is more suitable for this?

AFAIK, while building ARM binary for MAC through vercel/pkg, it requires an M1 machine. But still we can just benefit from Rosetta for this by using AMD64 binaries and AMD64 Starport CLI. But again AFAIK, if Starport compiled to M1, there was a problem running binaries as child process(e.g. running nodetime through Starport CLI) if these binaries are AMD64.

I will certainly look into it and see if it works well. I think it would make a lot of sense. If there is any context or additional info I should know about just let me know.

This is recently removed: #2163

Building the nodetime binary creates nondeterministic bytecode. This is mentioned in the pkg docs as well, and can in theory be avoided by building with a --no-bytecode flag. I've spent some time trying to make this work, but it seems like because we include binaries like tsc the build fails with an error that those files include bytecode. I will try to figure this out, but at least for nodetime it would make sense that the trigger for building a new binary comes from changes in that part of the code (updates to package.json, the nodetime file etc).

AFAIK --no-bytecode was working properly in one of Linux AMD64, Darwin AMD64 or M1. Please try to check out previous attempts about this topic, maybe there can be an answer there:

We can have multiple CI workflows if that what's needed.

@gjermundgaraba
Copy link
Contributor

Do we specifically need Linux

I can test if the output from the mac will run on linux as well, but they might require different machines, at least for the dart compilation.

AFAIK, while building ARM binary for MAC through vercel/pkg, it requires an M1 machine.

For the vercel/pkg compilation it actually works on linux (I've tested in on a cloud-based M1), but it requires some slightly hacky setup to be able to sign the binary. So better to run that on the mac anyway.

This is recently removed: #2163

Nice, that is excellent! Will look more into that.

AFAIK --no-bytecode was working properly in one of Linux AMD64, Darwin AMD64 or M1. Please try to check out previous attempts about this topic, maybe there can be an answer there:

Oh, interesting. I will try this out a bit more then and see what I come up with.

Thanks for all the answers.

I am making good progress and have already created binaries that I've run on M1 and Linux arm64. Should this be compiled to more arm versions than just arm64/aarch64?

@ilgooz
Copy link
Member Author

ilgooz commented Mar 17, 2022

I think arm64 would be enough for now?

@gjermundgaraba
Copy link
Contributor

Sounds good.

Just FYI: There is a chance I might get somewhat delayed on this (relative to the 10 day deadline) because I got Covid and I am not very productive right now. I'll keep you posted, and also let me know when the M1 become available so I can do some testing with that.

@ilgooz
Copy link
Member Author

ilgooz commented Mar 18, 2022

Sorry to hear that, get well. 🙏

No worries about the deadline.

@gjermundgaraba
Copy link
Contributor

@ilgooz Back in action. I've set up the provided M1 machine as a runner and got it to build both amd64 and arm64 for darwin. The dart build unfortunately does not work on linux arm64. I think the only way to get that done is to also get a runner for Linux arm64/aarch64.

Just for documentation purposes: The GitHub runner software is not quite yet ready for M1 machines, but there are some workarounds for this until it is released: actions/runner#805 (comment)

@ilgooz
Copy link
Member Author

ilgooz commented Mar 29, 2022

Super cool! We're preparing ARM Linux machines. 👍

@gjermundgaraba
Copy link
Contributor

gjermundgaraba commented Apr 1, 2022

@ilgooz draft PR up now. There are some things to be sorted out (github runners and practical merge strategy)

I left it in merge until we sort those out, but the PR is ready for review otherwise.

@ilgooz ilgooz linked a pull request Apr 5, 2022 that will close this issue
@ilgooz
Copy link
Member Author

ilgooz commented Apr 13, 2022

All green! 👏

https://github.com/ignite-hq/cli/releases/tag/v0.0.0-nightly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bounty type:feat To implement new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants