search

delba / Permission

A unified API to ask for permissions on iOS
MIT License
2.91k stars 324 forks source link

[WIP] Compiled Permission Build Flags #75

Closed justinmakaila closed 7 years ago

justinmakaila commented 7 years ago

Began sketching out an idea in order to ensure that this can be compiled with/out certain permissions, based on the developer's choice. Resolution for #57.

Idea is to just use build flags defined in PermissionConfiguration.xcconfig to conditionally compile the individual Permission extensions with/out their dependencies. A fatalError will occur if the developer attempts to request permission for an ability they did not compile the framework with.

TODO

AndrewSB commented 7 years ago

This is awesome @justinmakaila.

How does a consumer to this library use the xcconfig file? Must one edit it directly within the Permission module before compiling the framework?

justinmakaila commented 7 years ago

Still working on that, following the link from ISHPermission On Thu, Oct 27, 2016 at 18:19 Andrew Breckenridge notifications@github.com wrote:

This is awesome @justinmakaila https://github.com/justinmakaila.

How does a consumer to this library use the xcconfig file? Must one edit it directly within the Permission module before compiling the framework?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/delba/Permission/pull/75#issuecomment-256786149, or mute the thread https://github.com/notifications/unsubscribe-auth/ABkCVONqcRDWc9EneelW4PcyM7DMuDXcks5q4SNEgaJpZM4Ki4Fz .

justinmakaila commented 7 years ago

@AndrewSB So what I'm thinking is that a user will either add a custom .xcconfig file to the root of their project, or this will automatically use the config file in the git repo.

justinmakaila commented 7 years ago

I could use some serious help getting this to work for CocoaPods. I'm afraid I haven't touched it in years, and don't want to break existing configurations.

@delba, Any ideas on who could handle that?

justinmakaila commented 7 years ago

Updated the README. This is ready for review, and will be ready for merge after we add CocoaPods support. @AndrewSB You can use this fork in your current project, I just integrated and everything works as described above.

@delba, I'm handing this off to you. Will resolve the bold confirmation text next in my pending PR.

AndrewSB commented 7 years ago

@justinmakaila thank you for setting this up!

I'll try checking this out with my project 😄

Can you also explain how the .xcconfig search works pre-compilation? Or just point me to the relevant code?

justinmakaila commented 7 years ago

@AndrewSB The .xcconfig just adds SWIFT_ACTIVE_COMPILATION_CONDITIONS to your build. All you have to do is un/comment the permissions that you want to add. I'm resolving a quick typo for the notifications permission right now...

delba commented 7 years ago

Thank you for the PR @justinmakaila ! A lot 🙏

I added some modifications here https://github.com/delba/Permission/commit/0486daa3d5d7cc6cb936974372baa18bf74026d4 because:

Could you please review it when you have some time ? 😃

Btw, that's a shame that Swift doesn't allow the use of macros in switch statements like this:

switch type {
    #if PERMISSION_CONTACTS
    case .contacts: return statusContacts
    #endif

    // ...
}
justinmakaila commented 7 years ago

So the reason why I put the macros in the status* methods was so we could keep the PermissionType enum complete, and if the developer tries to add a Permission that they didn't compile with, they could get the runtime fatal error and be aware of what needs to be done to fix it.

It also removed all of the logic and linked frameworks from the binary, which would (hopefully) resolve the #57. My biggest fear with the current revisions is that #57 is still a thing because the frameworks/code for the permission requesting still ends up in the binary.

If you can verify that #57 is resolved by leaving all of the code and imports in, then I'm all good with it.

justinmakaila commented 7 years ago

https://developer.apple.com/library/content/qa/qa1937/_index.html

After reviewing this document, I'm not sure the changes you made will resolve #57 because the core of the request code is still compiled into the binary. @delba

delba commented 7 years ago

The frameworks/code are still wrapped if a flag check like this:

#if PERMISSION_CONTACTS
import Contacts

internal extension Permission {
    // statusContacts + requestContacts
}
#endif

see here

It should prevent it to end up in the binary, no?

justinmakaila commented 7 years ago

👍 sorry, my sleepy morning eyes skimmed right over that part, i only saw the flags in the switch statements

delba commented 7 years ago

Ahahah ok

So we're good ? 😄

justinmakaila commented 7 years ago

I don't think so, I removed a few permissions from the configuration file and immediately got some build errors:

PermissionConfiguration.xcconfig:

PERMISSION_ADDRESS_BOOK      = //PERMISSION_ADDRESS_BOOK
PERMISSION_BLUETOOTH         = //PERMISSION_BLUETOOTH
PERMISSION_CAMERA            = PERMISSION_CAMERA
PERMISSION_CONTACTS          = //PERMISSION_CONTACTS
PERMISSION_EVENTS            = PERMISSION_EVENTS
PERMISSION_LOCATION          = //PERMISSION_LOCATION
PERMISSION_MICROPHONE        = //PERMISSION_MICROPHONE
PERMISSION_MOTION            = //PERMISSION_MOTION
PERMISSION_NOTIFICATIONS     = PERMISSION_NOTIFICATIONS
PERMISSION_PHOTOS            = //PERMISSION_PHOTOS
PERMISSION_REMINDERS         = PERMISSION_REMINDERS
PERMISSION_SPEECH_RECOGNIZER = //PERMISSION_SPEECH_RECOGNIZER
PERMISSION_MEDIA_LIBRARY     = PERMISSION_MEDIA_LIBRARY

// Do not modify this line. Instead, remove comments above as needed to enable the categories your app uses.
PERMISSION_FLAGS = $(PERMISSION_ADDRESS_BOOK) $(PERMISSION_BLUETOOTH) $(PERMISSION_CAMERA) $(PERMISSION_CONTACTS) $(PERMISSION_EVENTS) $(PERMISSION_LOCATION) $(PERMISSION_MICROPHONE) $(PERMISSION_MOTION) $(PERMISSION_NOTIFICATIONS) $(PERMISSION_PHOTOS) $(PERMISSION_REMINDERS) $(PERMISSION_SPEECH_RECOGNIZER) $(PERMISSION_MEDIA_LIBRARY)

SWIFT_ACTIVE_COMPILATION_CONDITIONS= $(inherited) $(PERMISSION_FLAGS)
screen shot 2016-10-31 at 10 47 43
delba commented 7 years ago

I had a build error on the very first build too after commenting out some permissions. It seems like some permissions are still referenced from a previous the previous one (?) The subsequent builds are ok though.

Also, make sure to include the latest commit https://github.com/delba/Permission/commit/5e1e6f9591be51998d68545dbe02f8ef6464883d

justinmakaila commented 7 years ago

Pulled the latest, cleaned, and built:

screen shot 2016-10-31 at 10 59 26

I'm not sure how you're getting builds to work, because there's an explicit mention of an enum case that will not exist if PERMISSION_NOTIFICATIONS is removed

justinmakaila commented 7 years ago

I removed all of the permissions, and the only errors seem to be caused by the location* enum cases

delba commented 7 years ago

Include https://github.com/delba/Permission/commit/5e1e6f9591be51998d68545dbe02f8ef6464883d

justinmakaila commented 7 years ago

Ok dope, yeah, my git was at like, e1 something, so I thought I was there. Just pulled, testing now.

justinmakaila commented 7 years ago

Removed and built with most permissions, tested a Carthage build... everything looks legit to me.

justinmakaila commented 7 years ago

@delba Did you test on CocoaPods? There's something completely different you have to do to get the .xcconfig to work, and I'm not familiar with CP at all.

delba commented 7 years ago

@justinmakaila Could you take a look at https://github.com/delba/Permission/commit/abd6a047b2edce61cf10beaed96d165f5df0b19f please?

The user defined configuration file should be named PermissionConfiguration.xcconfig instead of PermissionConfiguration.xcconfig, right?

The support for CocoaPods will come later, I'm afraid...