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

Apple Silicon Foreign Library Support (#8227) #8232

Merged
merged 3 commits into from
Jun 22, 2022

Conversation

yobson
Copy link
Contributor

@yobson yobson commented Jun 18, 2022

Fix for issue #8227 and #7837

  • Allowed building Foreign Libraries for all architecture on apple
    silicon
  • Updated error message to name Mac OS instead of OSX as supported
    platform

Please include the following checklist in your PR:

I successfully compiled a dylib, although I confess that I have't tested said dylib.

- Allowed building Foreign Libraries for all architecture on apple
  silicon
- Updated error message to name Mac OS instead of OSX as supported
  platform
Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

LGTM.

Would it be possible to have a tiny test so that we know for a fact it's fixed next time the code around this feature changes (or fails to change)?

The change is in package Cabal, so I think that's what should be in packages: in the changelog.

@yobson
Copy link
Contributor Author

yobson commented Jun 20, 2022

Not a problem! I've not looked at Cabal's testing system before, any pointers?

@Mikolaj
Copy link
Member

Mikolaj commented Jun 20, 2022

git grep OSX turns out a test with some of the same keywords: cabal-testsuite/PackageTests/ForeignLibs/setup.test.hs

This kind of testing is described at https://github.com/haskell/cabal/tree/master/cabal-testsuite

@Mikolaj
Copy link
Member

Mikolaj commented Jun 20, 2022

BTW, does our CI run Apple Silicon? could it? @jneira, @andreabedini?

@yobson
Copy link
Contributor Author

yobson commented Jun 20, 2022

I'm running the tests now; the file you referenced doesn't look like it needs any cases added, it just needs to be run on an apple silicon Mac. If not, I'm not certain how is best to ensure it runs on A64 without an A64 Mac builder.

I'm running the tests now to see if they pass on my Mac

@Mikolaj
Copy link
Member

Mikolaj commented Jun 20, 2022

We could try somehow faking it, so that cabal thinks it's Apple Silicon. In any case, if the test tests what it should and if all tests pass for you, that's a good foundation, whether we extend CI or not.

@yobson
Copy link
Contributor Author

yobson commented Jun 20, 2022

ForeignLibs $ cabal run cabal-tests
Up to date
threads: 1
tests to run: 1
setup.test.hs  OK (9.01s)
SKIPPED 0 tests
OK

Looks like it passed! This test builds a foreign library, links it to a c program and runs that c program. Pretty comprehensive.

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

That's probably the best we can do.

@andreabedini
Copy link
Collaborator

@Mikolaj AFAIK GitHub Actions doesn't support Apple Silicone yet (see roadmap issue). The GitHub Actions runner does so if someone has spare apple m1 machines one could self-host it.

@yobson
Copy link
Contributor Author

yobson commented Jun 21, 2022

https://www.macstadium.com/opensource

They say that they offer free macs (remote) to open source projects. I wonder if the haskell project as a whole could approach them? Or maybe they have? I think it would be easier to get a machine for ghc, cabal etc, that it would for just cabal.

Anyway, if no one has approached them, I'd be happy to

@Kleidukos
Copy link
Member

Can't we mutualise CI resources with the GHC project?
cc @chreekat

@chreekat
Copy link
Collaborator

@Kleidukos GHC does have some mac runners for gitlab.haskell.org! I'm not aware of any runners for github, unfortunately.

Creating (shared?) resources for other projects besides GHC is on the backlog for the Haskell Foundation. You could already talk to @david-christiansen about just getting some devices sponsored...

@angerman
Copy link
Collaborator

All this change does is remove an architecture filter. I have some serious doubt that this PR does need a validation on aarch64-darwin in CI to be acceptable. Let's just merge it. This PR is perfectly fine.

On the general CI topic. GHC does have 6 M1 macs. 3 sponsored by simspace, 3 sponsored by me. We used to have one or two from macstadium, and I'm not sure if @bgamari is still renting some there?

As with many providers getting someone to actually offer you performant and recent mac hardware is fairy tricky. (or get's expensive fast); if cabal was hosted on gitlab.haskell.org, it would get access to all the GHC builders. There are even two windows runners I also sponsor. Moving stuff to gitlab is clearly not an easy option. At the same time providing non containerised ressources to github opens up all kinds of other concerns (gitlab effectively only offers security through obscurity here).

@Mikolaj
Copy link
Member

Mikolaj commented Jun 22, 2022

We do want to test even silly things, but not at the cost of maintaining CI on github and gitlab, so let's wait with the testing until we can extend our single CI script to Apple Silicon easily. @yobson: anything to add or do we merge now (via merge_me label)?

@yobson
Copy link
Contributor Author

yobson commented Jun 22, 2022

I'm happy to merge!

@Mikolaj Mikolaj added the merge me Tell Mergify Bot to merge label Jun 22, 2022
@mergify mergify bot merged commit 3c09dc6 into haskell:master Jun 22, 2022
@Mikolaj
Copy link
Member

Mikolaj commented Jun 22, 2022

@Mergifyio backport 3.8

@mergify
Copy link
Contributor

mergify bot commented Jun 22, 2022

backport 3.8

✅ Backports have been created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
attention: needs-review merge me Tell Mergify Bot to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants