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 More Valid Architectures #21

Merged
merged 16 commits into from
Jan 17, 2019
Merged

Add More Valid Architectures #21

merged 16 commits into from
Jan 17, 2019

Conversation

giantramen
Copy link

Add armv7s armv7 and arm64 so it will build for 32 bit
iPhones.

@arturgrigor
Copy link
Contributor

arturgrigor commented Nov 15, 2017

@giantramen Adding VALID_ARCHS is not needed. You just need to select the proper Xcode destination and it will build for that architecture. Removing the UIRequiredDeviceCapabilities key from the Info.plist it's a good idea though.

As a side note: This project is using almost the same configuration as PromiseKit (https://github.com/mxcl/PromiseKit) especially regarding the build configuration and platforms support.

@giantramen
Copy link
Author

I see that PromiseKit only has i386 and x86_64 for VALID_ARCHS, but according to the documentation that shouldn't work:
"Specifies the architectures for which the binary may be built. During the build, this list is intersected with the value of ARCHS build setting; the resulting list specifies the architectures the binary can run on. If the resulting architecture list is empty, the target generates no binary."

Is there something I'm missing here or is the documentation just misleading?

@arturgrigor
Copy link
Contributor

Here's the doc about VALID_ARCHS from Xcode:

A space-separated list of architectures for which the target should actually be built. For each target, this is intersected with the list specified in ARCHS, and the resulting set is built. This allows individual targets to opt out of building for particular architectures. If the resulting set of architectures is empty, no executable will be produced.

It seems that VALID_ARCHS is used to filter out the architectures supported by a target. What they do not specify is what happens if the VALID_ARCHS setting is missing but I think it's clear what happens after looking at the results.

@codecov-io
Copy link

codecov-io commented Nov 15, 2017

Codecov Report

Merging #21 into master will increase coverage by 0.56%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master    #21      +/-   ##
========================================
+ Coverage   99.43%   100%   +0.56%     
========================================
  Files           7      4       -3     
  Lines         176     51     -125     
========================================
- Hits          175     51     -124     
+ Misses          1      0       -1
Impacted Files Coverage Δ
Sources/AwaitKit/AwaitKitExtension.swift 100% <ø> (ø)
Sources/AwaitKit/DispatchQueue+Await.swift 100% <100%> (ø)
Sources/AwaitKit/DispatchQueue+Async.swift 100% <100%> (ø)
Sources/AwaitKit/AwaitKit.swift 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be47f5a...516cfd2. Read the comment docs.

Removed 64 bit device required so it
will build for 32 bit iPhones.
@giantramen
Copy link
Author

Removed the Valid Architectures. I was confused because in the empty state Xcode shows i386 and x86_64, even though it will build for all architectures.

@giantramen
Copy link
Author

Any chance you can merge this in? Would like to point back at a real release.

@yannickl
Copy link
Owner

@giantramen Sorry for the very late, your last commits and I don't know why. I merge it now, thank you for your contribution!

@yannickl yannickl merged commit 55e4bf8 into yannickl:master Jan 17, 2019
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.

6 participants