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 libyrs.dylib built for osx arm #81

Merged
merged 1 commit into from Jan 12, 2024
Merged

Conversation

vdurante
Copy link
Contributor

Add libyrs.dylib for OSX ARM

Copy link
Collaborator

@SebastianStehle SebastianStehle left a comment

Choose a reason for hiding this comment

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

As mentioned, a separate package would be better.

native/YDotNet.Native.MacOS/macos-arm64/.gitignore Outdated Show resolved Hide resolved
native/YDotNet.Native.MacOS/YDotNet.Native.MacOS.csproj Outdated Show resolved Hide resolved
@SebastianStehle
Copy link
Collaborator

Btw: For testing I would just reference this project from the unit tests (temporarily) and then run the tests.

@vdurante
Copy link
Contributor Author

vdurante commented Jan 11, 2024

@SebastianStehle

I used the .dylib file on my custom project and it worked, but for some reason if I add the native ARM project as a reference to the unit tests project, it still fails

This is how the project looks like:

image

Added this to YDotNet.Tests.Unit:

<ProjectReference Include="..\..\native\YDotNet.Native.MacOS.ARM\YDotNet.Native.MacOS.ARM.csproj" />

Still failing when I run. Checking the bin folder, it doesn't contain the runtimes folder.

@SebastianStehle
Copy link
Collaborator

Perhaps this is the reason why it is called "PackagePath" ;)...So I guess we have to publish it. But you have to explain me the other changes in the yml file before I can merge it.

@vdurante
Copy link
Contributor Author

vdurante commented Jan 11, 2024

Regarding the yml file:

Here you can find a comment that directs to the other repository that contains a workflow file that was used as inspiration:

# Based on https://www.rohanjain.in/cargo-cross/

Going to the other repository, you can find how it builds for both Mac x64 and arm64

https://github.com/crodjer/sysit/blob/b03d0d4920ab43241d8e0976a5de3962b3bce016/.github/workflows/release.yml#L62-L71

I basically copied over the yaml config to my PR and adjusted the Content's Include and PackagePath accordingly to the new directories.

  • changed mac to macos-x86_64, so I modified the Include path from output\mac to output\macos-x86_64 and the PackagePath to osx-x64
  • added macos-aarch64, so Include path to output\macos-aarch64 and PackagePath to osx-arm64

I am not entirely sure where the output\<os-arch> folder naming is configured, but from what it seems the subfolder uses the name of the build step, hence why I use macos-x86_64 and macos-aarch64 for those

- build: macos-x86_64
os: macos-latest
rust: stable
target: x86_64-apple-darwin
cross: false
- build: macos-aarch64
os: macos-latest
rust: stable
target: aarch64-apple-darwin
cross: false

Not easy to test this locally, since nektos/act doesn't have Docker images for MacOS

@SebastianStehle
Copy link
Collaborator

Ah, I see. aarch is just an arm variation. The point is: You cannot build for arm yet because there are not arm github runners yet. Therefore we have to build it locally and this is the reason why I cannot do it myself (because I do not have a mac).

So for now, we should:

  1. Just keep the build process as is,.
  2. In the current MacOS project file, use runtimes\osx-x64\native and target path to avoid confusion (but use the existing source file).
  3. Add your binary to the repo.

When ARM runners are available we can make another PR.

@vdurante
Copy link
Contributor Author

vdurante commented Jan 11, 2024

@SebastianStehle from what I read online rust is capable to build for ARM using Intel, so it should technically be possible.

If you want, we can test it, otherwise I can follow your approach!

Also, Linux contains all architectures in the same project, why separate into Intel and ARM only for Mac? Wouldn't it make sense to follow the same pattern? The file is not that big, roughly 1.5 MB

@SebastianStehle
Copy link
Collaborator

That would be great. Can you confirm that? Just run the build in your fork and download the artifacts. If it works: Great :)

I agree with your argument about Mac and Linux project. I have forgotten that. Lets do one project again

@vdurante
Copy link
Contributor Author

vdurante commented Jan 12, 2024

@SebastianStehle it works! Just tried on my project and it worked well. I built using github actions, downloaded the artifact and used it on my local yjs project.

I also reverted the other changes and kept a single Mac project

@SebastianStehle SebastianStehle merged commit 94abadd into y-crdt:main Jan 12, 2024
@akatakritos akatakritos mentioned this pull request Feb 11, 2024
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.

None yet

2 participants