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

[Project] Remove automatic preprocessor macro named after configuration #4148

Closed
wants to merge 1 commit into from

Conversation

mrackwitz
Copy link
Member

This addresses #4143.

CocoaPods extended the original Xcodeproj behavior when adding a new configuration. A preprocessor definition named after the newly created configuration was automatically configured. This was done to support the TargetEnvironmentHeader specification of Pods available only on certain build configurations. The TargetEnvironmentHeader was removed meanwhile, but that was left.

Note:

This has potential to break a lot of pods for debugging, especially those which have relied on the DEBUG=1 definition.

Agreement:

DEBUG=1 should be kept, additionally all other configuration macros should be prefixed by POD_CONFIGURATION_

CocoaPods extended the original Xcodeproj behavior when adding a new configuration. A preprocessor definition named after the newly created configuration was automatically configured. This was done to support the TargetEnvironmentHeader specification of Pods available only on certain build configurations. The TargetEnvironmentHeader was removed meanwhile, but that was left.
@mrackwitz mrackwitz added the t1:enhancement Enhancements that have not been picked up yet. Please comment if you plan to work on it label Sep 9, 2015
@segiddins
Copy link
Member

I'm not convinced we can get rid of the DEBUG define without breaking lots of pods.

@segiddins
Copy link
Member

Changes are fine if we decide the change in behavior is what we want

@mrackwitz
Copy link
Member Author

Yes, we should try to make a well-thought-out decision on that. If we decide against, which would be reasonable, I would update instead just the comment accordingly.

For most cases, this will mean that neither DEBUG nor RELEASE for the according configuration will be defined. Podspecs can't take care of defining these macros as the xcconfigs attributes are not configuration(-type) dependent. This will make it hard to define the macro as user from the podfile, you will likely need to add a post_install hook. It is not enough to define the macro in the user project, when the pod makes use of the macro in the implementation files.

An alternative could be to use a less conflict-prone name for the macro, e.g. COCOAPODS_CONFIG_DEBUG. This can be still aliased to other names using the pod_target_xcconfig or headers.

/c @CocoaPods/core

@neonichu
Copy link
Member

I would say, let's break it now and move to a COCOAPODS_ prefixed variant instead.

@orta
Copy link
Member

orta commented Sep 11, 2015

I'm not entirely sure I get this change, can someone ELI5 for me? 👼

@neonichu
Copy link
Member

@orta TIL ELI5 :)

CP automatically generates preprocessor defines named after the build configurations right now, e.g. DEBUG. These can clash with other uses of the same name in user projects, as described in #4143.

This PR removes those preprocessor defines completely, but as it is hard to replicate their existence manually, there would also be the possibility of renaming them to something less likely to cause naming clashes. In any case, if we change this, Pods that rely on the defines for conditional compilation will break.

@segiddins
Copy link
Member

Thinking more about this, I don't think it's a good idea to get rid of the DEBUG define, since Xcode generates it by default in debug mode... I think, ultimately, this is trying to fix a bit of an edge case. If a project truly wants to have a variable named DEBUG, they can #undef it?

@mrackwitz
Copy link
Member Author

I'd agree for DEBUG at least, especially as Xcode sets it by default.
For the other configurations, I could still imagine that we define them only with a namespacing, which I would apply back to the debug configuration for consistency.

@segiddins
Copy link
Member

I don't think the other configurations are strictly necessary, but if its something like POD_CONFIGURATION_FOO, I wouldn't object to it 👍

@segiddins
Copy link
Member

Scoping for 1.0, as this is a breaking change.

@segiddins segiddins added this to the 1.0.0 milestone Oct 5, 2015
@segiddins
Copy link
Member

I have an implementation of this locally, will push up tonight.

@segiddins
Copy link
Member

This has been succeeded by #4702.

@mrackwitz mrackwitz closed this Dec 30, 2015
@mrackwitz mrackwitz deleted the mr-remove-preprocessor-configuration-name branch December 30, 2015 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t1:enhancement Enhancements that have not been picked up yet. Please comment if you plan to work on it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants