Disable asserts in release builds when using Swift Package Manager

[rcancro]

When using Swift Package Manager to integrate Purchases into an app, Xcode does not automatically set NS_BLOCK_ASSERTIONS to 1 when building the package in release. This means that any NSAsserts that are hit in release cause crashes. This appears to only be an issue with ObjC packages.

The change I've made is to Package.swift to set NS_BLOCK_ASSERTIONS to 1 when building in release.

(Note: I didn't actually hit an assert in this Purchases, but the possibility is there)

I haven't added unit tests as I can't think of a way to do that using SPM. I did, however, point to your branch of 3.13.1 and change a configure method to look like:

+ (instancetype)configureWithAPIKey:(NSString *)APIKey appUserID:(nullable NSString *)appUserID {
    NSAssert(0, @"");
    return [self configureWithAPIKey:APIKey appUserID:appUserID observerMode:false];
}

When building in release and pointing to your 3.13.1 branch, this causes a crash. I think change SPM to point to my fork with the change in Package.swift and the assert no longer caused a crash.

[aboedo]

Hi @rcancro! Thanks for reporting!

We try to be extremely conservative with NSAssert, where the only assertions are done for situations that would cause an irrecoverable internal inconsistency, only recoverable by a restart. An equivalent of FatalErrors for Swift.

Have you hit any of them while running the app?

Note: we're working on a v4 of the SDK, built entirely in Swift, that handles these a bit differently. This is currently in beta, though.

[NachoSoto]

I don't love disabling assertions, but I suppose this change makes SPM consistent with the project.

[taquitos]

I think given that we're very close to releasing RCv4, we should audit fatalError to ensure we only have that for cases where it truly is irrecoverable.

As for updating RCv3 for setting NS_BLOCK_ASSERTIONS to 1, I think that's too big of a change in behavior too close to RCv4 release. By changing the behavior for when the SDK willfully crashes, that means developers might need to inspect more things to ensure consistency, which, might not be easily possible for things that aren't explicitly public. So by blocking assertions we could be silently enabling a class of internal consistency bugs that are super hard to track down 😬

Of course, that is worst case scenario, but IMO I'd prefer to have the SDK crash right now instead of having the potential to silently do the wrong thing.

We're going to continue to examine how we willfully crash, though. This is really good feedback.

[rcancro]

I did notice there are very few places where you use NSAssert in your SDK, and I haven't hit any of them. But is your intention to crash the app when one of these is hit? If you really are using NSAssert like fatalError(), then I think the more Objective-C way to do this is to raise an exception.

IMO I'd prefer to have the SDK crash right now instead of having the potential to silently do the wrong thing.

Actually, right now it appears that asserts are disabled in release for Cocoapods. If leaving assertions on in release is your desired behavior, then you need to add that to the Podspec I believe. Or if you turn asserts off in SPM it will make both methods of installing Purchases consistent.

[taquitos]

Yeah, great point and I didn't realize we had different configs for SPM/Pods, whoops. I'll chat with the team about this. We should, at least, be consistent!

[NachoSoto]

I'm in favor of this to have consistency across the different types of integration, but I'll let the team chime in.
If we merge this, could you add a comment linking to https://forums.swift.org/t/assertions-in-swift-packages/42692 for future reference?

[taquitos]

I need to do some research before I can 👍

[aboedo]

@taquitos any updates here?

[taquitos]

I'm ok with this. That being said, we're about to launch RCv4, so we'd want to merge this in a new hotfix branch for v3.

[aboedo]

@rcancro thanks for the PR, explanation and patience! I'm merging this now and making a release