search

kif-framework / KIF

Keep It Functional - An iOS Functional Testing Framework
Other
6.21k stars 914 forks source link

`#define system` breaks <SceneKit/SCNParticleSystem.h> #1231

Open ashi009 opened 3 years ago

ashi009 commented 3 years ago

I've found #733, however, it appears to me that the issue is still there.

We have accidently imported KIF right before <SceneKit/SCNParticleSystem.h>, and got this:

image

The macro expansion took place in the wrong place. It's really not a good idea to define a token without using any prefixes.

It seems that until the macro is removed/substituted, the only reliable workaround would be import KIF at the last place.

justinseanmartin commented 3 years ago

This has already been addressed, but had to be done in a non-breaking way. You'll need to add -DDEPRECATE_KIF_SYSTEM=1 to OTHER_CFLAGS build setting to opt-in and uses systemTester instead of system.

ashi009 commented 3 years ago

It has been addressed, but not fixed.

Even with -DDDEPRECATE_KIF_SYSTEM=1, it will still define system as a function call and break all these:

image

justinseanmartin commented 3 years ago

I see your point about the conflicting system define. The solution seemed to work at the time to avoid the issue initially called out in #733 that the PR was addressing. This could probably be addressed by either:

  1. #undef system after importing KIF in your code, or manage the import ordering (not great)
  2. Introduce another pragma, like NO_KIF_SYSTEM_DEPRECATION_WARNING that avoids having the system compatibility warning define.
  3. Remove the system definition define within DEPRECATE_KIF_SYSTEM. This would technically be a breaking change, but is potentially still reasonable. This was always opt-in and was only intended to be here to generate a more helpful compiler error.
  4. Remove definition of system entirely from KIF and bump to 4.0 (breaking change)

@ashi009 - Thoughts?

I think 3 would be my preferred approach, but I'd defer to @dostrander.

dostrander commented 3 years ago

@justinseanmartin @ashi009 I think option 3 is fine. However it might be a good time to rip the bandaid off and just go for option 4, bump KIF to 4.0 and have a long running 3.x branch, support that for another 6 months to a year and then formally stop supporting system completely.

Is there a way to show a warning in cocoapods as you install something, might be good to show folks that this is going to be removed.

justinseanmartin commented 3 years ago

People will still be able to use KIF 3.x if they wanted, but it will decay over time. CocoaPods doesn't have that facility, but that was somewhat the intent behind introducing the DEPRECATE_KIF_SYSTEM pragma. We could start making that deprecation warning show up for folks in master on bumping KIF (but have it still be functional until 4.x).

The biggest downside of introducing the breaking change is that it could fracture the users of the library, and potentially incentivize migrating away from KIF. Also, if we're going to rip the bandaid, there are other things we'd probably want to batch together at the same time potentially, for example removing KIFUITestActor. We would probably want to start by making that deprecation warning by default in master first.

I think I'd probably time it to be after fixing everything in 3.x to work with iOS 15, so people have the longest runway possible.