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

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

Open
ashi009 opened this issue Jun 7, 2021 · 5 comments
Open

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

ashi009 opened this issue Jun 7, 2021 · 5 comments

Comments

@ashi009
Copy link

ashi009 commented Jun 7, 2021

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
Copy link
Contributor

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
Copy link
Author

ashi009 commented Jun 7, 2021

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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

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.

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

No branches or pull requests

3 participants