Add support for compilation on apple silicon

myobie commented 3 years ago

[x] Add macarm target and filename
[x] Test on the M1 iMac
[x] Test in a mix project
- It only worked for me when I added :rambo to my list of compilers in my project's mix.exs. I think this is because it was installed through the :git way and not the normal hex way. But I'm not sure what is expected here. I imagine once CI runs and it's published to hex it will work auto-magically.

thbar commented 2 years ago

@myobie thanks for this PR. I was stuck with my M1 and this appears to fix the issue (tested via a github: link in my mix.exs). FYI @jayjun!

marcelotto commented 2 years ago

Any progress here? Switching to this branch also solved my problems with this lib on a M1. However, the need to add :rambo to the list of compilers introduced a problem on our CI, since it now requires a Rust environment, which it previously didn't.

myobie commented 2 years ago

Any progress here? Switching to this branch also solved my problems with this lib on a M1. However, the need to add :rambo to the list of compilers introduced a problem on our CI, since it now requires a Rust environment, which it previously didn't.

If you want to be clever you can use a conditional in the mix.exs file which uses the hex package for CI. This will cause a change to the lock file on the CI machine; however, just don't commit that change 😅

I know this isn't ideal, but I also maintained a rust environment for a while until I started doing this method instead.

achedeuzot commented 2 years ago

Hello @jayjun Any hope of having this fix in an upcoming release ? 🙏 That would be very nice 🥺 Thanks !

luka-TU commented 2 years ago

@jayjun +1 to the above!

achedeuzot commented 2 years ago

I've just tested it and it does fix my issues too, thanks @myobie 💙

cseeger commented 2 years ago

I'd love to see this merged as well. @jayjun - anything we can do to help?

achedeuzot commented 2 years ago

Hey @jayjun Sorry to ping about this but is there any way we can help to make it released ? Adding a maintainer that would be ok to do a release on hex.pm could be nice 😉 Hope you're well 😄

thbar commented 2 years ago

I have figured something out while attempting to add this PR to a project today (see https://github.com/etalab/transport-site/pull/2524/files).

Apparently this PR alone will not be sufficient all the time: in my case (umbrella project, although I do not know if this is the culprit or not), if I attempt to use {:rambo, "~> 0.3.4", github: "myobie/rambo", ref: "e321db8e4f035f2a295ee2a5310dcb75034677ce"} as a dependency (so link to GitHub), the use of Rambo will fail.

The reason is that I need to manually call mix compile.rambo, the custom built-in task responsible to generate the Rust binary.

It is like if the compilers: [:rambo] option was not having effect.

On the same project, using the Hex.pm version on a non-M1-Mac works perfectly.

I wonder if merging this PR then publishing the package will be enough (maybe it is a compilers: option bug).

I would be happy to see this PR merged and a new version released, so that I can test this specific problem and create a new issue if needed.

achedeuzot commented 2 years ago

@thbar I think this fix should be enough. The releases of the package to hex.pm contains compiled binaries (I guess there is a call to mix compile.rambo before packaging and uploading it to hex.pm) so I think it should probably be enough with the current fix but until we have an answer from @jayjun, we're kinda stuck in any case.

I'll reiterate but if there's anything I can do to make this land on a proper release on hex.pm don't hesitate to ping me :)

thbar commented 2 years ago

@achedeuzot I have reached out to @jayjun via various ways, and luckily he is going to respond soon! Maybe we can assemble a small team of maintainers if he agrees to.

It would be nice to find a way to fully automate the release process, including binaries for ARM/M1, so that this reduces the burden of the author.

Also I can confirm that the mix compile.rambo bug I have seen is an umbrella-related issue, unless I'm mistaken. I will investigate further and maybe it will end up in a bug report somewhere.

thbar commented 2 years ago

A couple of notes :

as a M1 user, the M1 is used only for development, so if the package was released with M1 support, without the M1 binary built-in, as long as I can build the binary at install time, it would be good enough!
M1 GitHub actions runners are on the roadmap! https://github.com/github/roadmap/issues/528 (in case we want to automate the builds here)

jayjun commented 2 years ago

Thanks for the pull request @myobie and @thbar for drawing my attention here.

So Rambo is published to Hex with compiled binaries for convenience only. It will support any architecture as long as Cargo is found, but I found a bug preventing auto compilation. The fix has the same effect of adding

def project do
  [
    compilers: Mix.compilers() ++ [:rambo]
  ]
end

to your project’s mix.exs or manually running mix compile.rambo so please do so until the next release.

As for bundling an Apple silicon binary, this pull request looks good so I’m going to merge it. But I cannot publish yet because I failed to compile the Windows binary. Tracking with #21, feel free to help.

jayjun / rambo

Add support for compilation on apple silicon #13