Skip to content

Conversation

buchwasa
Copy link
Contributor

This PR depends on #635

This PR aims to add support for Apple Silicon.
When compiling, the build script will now automatically select x64 or arm64 to compile with.
Avalonia has been bumped to version 0.10.13 as 0.10.7 crashed on arm64 (x64 through Rosetta worked fine on that version, however).

I have tested this PR through Rosetta and native and everything works perfectly, however, this is lacking sufficient testing on linux-x64.

@sn3akrr

This comment was marked as abuse.

Copy link
Contributor

@ldennington ldennington left a comment

Choose a reason for hiding this comment

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

This is a great start! I've left a couple comments and also have one high-level note:

We're going to have to update CI to publish packages for both x86_64 and ARM64. The relevant CI/release pipelines can be found in the .azure-pipelines directory. This could end up being a bit tricky, given that these pipelines run in a restricted Azure DevOps account, but we can work together to validate changes as needed.

@buchwasa
Copy link
Contributor Author

buchwasa commented Apr 4, 2022

Hello! So sorry that I haven't gotten to this recently, been very busy. Is there anywhere I can contact someone regarding the Azure Pipelines stuff (@ldennington)? I don't want to be spamming this PR asking if it worked or not.

@ldennington
Copy link
Contributor

ldennington commented Apr 4, 2022

Hello! So sorry that I haven't gotten to this recently, been very busy. Is there anywhere I can contact someone regarding the Azure Pipelines stuff (@ldennington)? I don't want to be spamming this PR asking if it worked or not.

I will run this today and let you know! Also, don't worry about "spamming the PR." It's helpful to me when you comment reminders like this because they surface in my GitHub notifications 😊.

@ldennington
Copy link
Contributor

ldennington commented Apr 4, 2022

@buchwasa - ignore the check saying that the release build failed. I re-ran with cf2eec3 applied and it passed. However, it still doesn't look like we're publishing separate packages for x86_64 and ARM64.

Screen Shot 2022-04-04 at 1 04 07 PM

Looking through the commits, I don't think that change has been made yet (forgive me if I missed it). Once this change has been applied, I can re-run the pipeline to verify the separate packages are generated.

@ldennington
Copy link
Contributor

ldennington commented Apr 4, 2022

@buchwasa - also, while I definitely understand how it feels to have a case of the Mondays 😉, I wonder if you wouldn't mind making your commit messages a little more descriptive? One of my co-workers just gave an excellent talk on commit message formatting with some helpful guidance:

https://www.youtube.com/watch?v=4qLtKx9S9a8

If you'd rather just view the slides, you can find them here:

https://www.youtube.com/watch?v=4qLtKx9S9a8

@buchwasa
Copy link
Contributor Author

@buchwasa - ignore the check saying that the release build failed. I re-ran with cf2eec3 applied and it passed. However, it still doesn't look like we're publishing separate packages for x86_64 and ARM64.

Screen Shot 2022-04-04 at 1 04 07 PM

Looking through the commits, I don't think that change has been made yet (forgive me if I missed it). Once this change has been applied, I can re-run the pipeline to verify the separate packages are generated.

At the time, I didn't have any code committed, I just didn't want to spam people who have notifications on for this repo

@buchwasa
Copy link
Contributor Author

This should be ready for another test, I didn't start allowing to be building both, because I want to make sure that the initial changes at least work. Apologies if this stuff is super obvious, I haven't worked with Azure Pipelines yet, so this is a big trial and error thing for me.

@ldennington
Copy link
Contributor

ldennington commented Apr 13, 2022

This should be ready for another test, I didn't start allowing to be building both, because I want to make sure that the initial changes at least work. Apologies if this stuff is super obvious, I haven't worked with Azure Pipelines yet, so this is a big trial and error thing for me.

No need to apologize - this is in no way your fault. The current setup with Azure Pipelines is incredibly opaque for our contributors. I am currently working to migrate it to GitHub actions, at which point you will be able to run these types of tests yourself. For now, though, I've kicked off a new Azure Pipelines run and will report on results tomorrow!

@ldennington
Copy link
Contributor

Sorry for the delay @buchwasa - there were a few issues with the release build that I worked through. I've opened a PR into this branch with my updates; once those are merged, I think you should be good to go with side-by-side publishing of the macOS binaries!

@buchwasa
Copy link
Contributor Author

This should be ready for another test, I didn't start allowing to be building both, because I want to make sure that the initial changes at least work. Apologies if this stuff is super obvious, I haven't worked with Azure Pipelines yet, so this is a big trial and error thing for me.

No need to apologize - this is in no way your fault. The current setup with Azure Pipelines is incredibly opaque for our contributors. I am currently working to migrate it to GitHub actions, at which point you will be able to run these types of tests yourself. For now, though, I've kicked off a new Azure Pipelines run and will report on results tomorrow!

Oh cool! I've actually worked a lot with GitHub actions in the recent months, and would love to help out with that, if possible!

@buchwasa
Copy link
Contributor Author

Maybe I'm overthinking, but would it be best to just add a separate job in release.yml for osx arm64?

@ldennington
Copy link
Contributor

Maybe I'm overthinking, but would it be best to just add a separate job in release.yml for osx arm64?

You're not overthinking - I agree that this would probably be a good idea at some point. However, my work to migrate our release flows to GitHub Actions led me to discover this issue. It seems that there is no real support for either self-hosted or GitHub-hosted arm64 runners. In light of this, I actually don't think it's worth trying to create a separate release workflow or build two different binaries at this time.

@buchwasa
Copy link
Contributor Author

Isn't it possible to cross compile for arm64 on x64? I know that I can cross compile for x64 on arm64 without Rosetta, I'd imagine it can go either way

@ldennington
Copy link
Contributor

@buchwasa - you are correct, cross compilation is possible. Good call on that. However, we still need to break this out into separate installers for arm64 and x64. This will involve creating a new distribution.xml file, perhaps we can have distribution.arm64.xml and distribution.x64.xml.

@buchwasa
Copy link
Contributor Author

buchwasa commented May 8, 2022

I'm not sure if I can pass a parameter to a template and set it as a variable, otherwise, I'd do that to reduce the amount of parameters I have in build.yml (and there is one more parameter that I plan on adding)

@ldennington
Copy link
Contributor

ldennington commented May 10, 2022

This is looking good! I can kick off another Azure DevOps run for validation. And don't sweat the amount of parameters in build.yml. Templates aren't supported with GitHub actions, so that's going to undergo a major refactor anyway.

Edit: Hmmm...the Azure DevOps YAML validator isn't loving build.yml. I think the issue is that we need to specify a trigger and pool. I'm off to sleep for the evening, but will give that a whirl tomorrow.

@ldennington
Copy link
Contributor

An update: I spent a good part of the day trying to get this to work in Azure Pipelines but couldn't quite get it there. @mjcheetham - what do you think about waiting for the migration to GitHub actions to land then fast following with some refactoring of this PR to make it work with the new infrastructure? Happy to partner with @buchwasa on that (since he should have more visibility into the new workflows).

@mjcheetham
Copy link
Contributor

@mjcheetham - what do you think about waiting for the migration to GitHub actions to land then fast following with some refactoring of this PR to make it work with the new infrastructure?

That's probably a good call to avoid wasting time getting the old CI working just to throw it away shortly afterwards. Right now native Apple Silicon support is not a blocker as running the x86_64 version under Rosetta works OK.

@buchwasa
Copy link
Contributor Author

buchwasa commented May 12, 2022

Happy to partner with @buchwasa on that (since he should have more visibility into the new workflows).

Keep me posted on this, I'd be happy to help!

@ldennington ldennington self-assigned this Jun 1, 2022
@ldennington
Copy link
Contributor

ldennington commented Jun 2, 2022

Hey @buchwasa! Sorry for the delay, but we've officially migrated our release processes to GitHub Actions! I've done some work to update this branch to work with these changes in my m1-release-updates branch. However, this m1 branch will need to be rebased onto main before I can open a PR into it with my changes. Would you mind doing the rebase when you get a chance?

The specification of a macOS runtime led to the need to update the
Microsoft.NET.Test.Sdk package. The old version, when combined with this
specificationi, caused dependent packages to be downgraded, which resulted
in NuGet errors. Upgrading to the latest version resolved this issue.
@buchwasa
Copy link
Contributor Author

buchwasa commented Jun 7, 2022

Hey @buchwasa! Sorry for the delay, but we've officially migrated our release processes to GitHub Actions! I've done some work to update this branch to work with these changes in my m1-release-updates branch. However, this m1 branch will need to be rebased onto main before I can open a PR into it with my changes. Would you mind doing the rebase when you get a chance?

There ya are. This is like the second time where I've successfully rebased, so I have high hopes that it's smooth sailing from here

@ldennington
Copy link
Contributor

There ya are. This is like the second time where I've successfully rebased, so I have high hopes that it's smooth sailing from here

Great job and thank you! PR is here.

@ldennington ldennington changed the base branch from main to feature/m1 June 9, 2022 23:06
@ldennington ldennington merged commit daa15c8 into git-ecosystem:feature/m1 Jun 9, 2022
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.

5 participants