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

Add support for compilation on apple silicon #13

Merged
merged 2 commits into from
Jul 24, 2022

Conversation

myobie
Copy link
Contributor

@myobie myobie commented Jul 29, 2021

  • Add macarm target and filename
  • Test on the M1 iMac
  • 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.

@myobie myobie changed the title Add support for compiling on apple silicon Add support for compilation on apple silicon Jul 29, 2021
@thbar
Copy link

thbar commented Nov 8, 2021

@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
Copy link

marcelotto commented Jan 18, 2022

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
Copy link
Contributor Author

myobie commented Jan 20, 2022

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
Copy link

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

@luka-TU
Copy link

luka-TU commented Mar 9, 2022

@jayjun +1 to the above!

@achedeuzot
Copy link

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

@cseeger
Copy link

cseeger commented Mar 17, 2022

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

@achedeuzot
Copy link

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
Copy link

thbar commented Jul 19, 2022

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
Copy link

achedeuzot commented Jul 19, 2022

@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
Copy link

thbar commented Jul 23, 2022

@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
Copy link

thbar commented Jul 23, 2022

A couple of notes :

@jayjun
Copy link
Owner

jayjun commented Jul 24, 2022

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 jayjun merged commit ad6307b into jayjun:master Jul 24, 2022
@jayjun jayjun mentioned this pull request Jul 24, 2022
thbar added a commit to etalab/transport-site that referenced this pull request Apr 13, 2023
@thbar thbar mentioned this pull request Apr 13, 2023
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.

7 participants