yonaskolb / Mint

A package manager that installs and runs executable Swift packages
MIT License
2.27k stars 122 forks source link

fix support for Apple Silicon #184

Closed amine2233 closed 3 years ago

amine2233 commented 4 years ago

Remove some coded not needed for Mac with Apple Silicon (ARM)

LinusU commented 4 years ago

Hmm, if we merge this as-is we will re-break #58...

Maybe we could just do a check and replace the x86_64 part dynamically instead?

amine2233 commented 4 years ago

Hmm, if we merge this as-is we will re-break #58...

Maybe we could just do a check and replace the x86_64 part dynamically instead?

Ok, actually i don't have access to my apple silicon mac, i will bring a solution when i can,

esteluk commented 4 years ago

I've had luck with an approach that uses sysctl to check the current architecture:

let arch = targetArchitecture()
let target = "\(arch)-apple-macosx\(osVersion.majorVersion).\(osVersion.minorVersion)"

...

private func targetArchitecture() -> String {
  var size = 0
  sysctlbyname("hw.machine", nil, &size, nil, 0)
  var machine = [CChar](repeating: 0,  count: size)
  sysctlbyname("hw.machine", &machine, &size, nil, 0)
  return String(cString: machine)
}

This seems to work on both x86_64 and arm64.

liamnichols commented 4 years ago

@LinusU: Is this deployment target work-around still required actually? Apple shipped proper support for deployment targets somewhere around Swift 4 and looking at the docs all the Supported Platform stuff came with Xcode 10.2 (the requirement from the README)

https://developer.apple.com/documentation/swift_packages/supportedplatform/3112757-ios

So potentially it's redundant now unless I've missed something?


I did go ahead and draft up the changes in #185 but if I'm right about the above then it's not necessary and this change would be enough? ~I think we'd need to reconsider this anyway given that we'll probably also need to support Universal Binaries at some point~

jbehrens94 commented 4 years ago

@liamnichols @yonaskolb What would be needed to get this support moving?

liamnichols commented 4 years ago

@jbehrens94: It depends if we still need to support the deployment target workaround or not. As per my last comment, I don't think we do but somebody else needs to confirm.

If we don't need it then the changes in 4ecde168958bb472c616b84bf555e064e56f4e53 were enough to do the trick and the last commit can be reverted.

yonaskolb commented 3 years ago

Thank you for the PR @amine2233. This seems to do the job great, but closing in favour of #185