Aggressive Pod project warning settings break pods #1629

Closed
rivera-ernesto opened this Issue Nov 28, 2013 · 20 comments

Projects

None yet

8 participants

@rivera-ernesto
Contributor

On top of modules being on by default breaking some builds (#1575), now also some warnings are set to "treat as error" breaking some extra ones.

Namely:

  • CLANG_WARN_DIRECT_OBJC_ISA_USAGE = YES_ERROR (Xcode default YES)
  • GCC_WARN_ABOUT_RETURN_TYPE = YES_ERROR (Xcode default NO)
  • CLANG_WARN_OBJC_ROOT_CLASS = YES_ERROR (Xcode default YES)

As I said before, I'm all in for extra warnings and enforcing better usage but breaking Pods can be frustrating for App developers that are not involved in the development of the libraries they use and that just stopped working once updating CocoaPods.

Owner

I see your point my counterarguments ares:

  • Those are Xcode defaults, the Xcode team definitely was aware that they would break existing projects not overriding them and they decided to turn them on even taking into account that.
  • Issues like this can lead to runtime crashes so it is better to fail early in the build process. For CLANG_WARN_DIRECT_OBJC_ISA_USAGE prevent accessing the isa directly in iOS 64bit which definitely would lead to bad things.
  • If the community or the author of Pod think that those warnings can safely be disabled for a specific Pod, they can do it via the xcconfig DSL attribute.
Contributor

Turning it on is okay. It is the "treat as error" that breaks it.

For instance CLANG_WARN_DIRECT_OBJC_ISA_USAGE may be bad on iOS 64 bits but it won't happen on projects with iOS 5 deployment target, and off course nobody is going to update a Pod with a DSL attribute for that.

My point is that warnings are okay and help to improve code, but treating them as errors just seems unnecessary and not the task of a tool like CocoaPods.

Owner

@alloy what is your take?

Owner
alloy commented Dec 2, 2013

Hmm, @rivera-ernesto makes good points regarding:

For instance CLANG_WARN_DIRECT_OBJC_ISA_USAGE may be bad on iOS 64 bits but it won't happen on projects with iOS 5 deployment target

Can we do this conditionally? As in, make them warnings on pods with old deployment targets and errors on new ones? And @rivera-ernesto do you think that would make sense?

Contributor

Well iOS 64 are not that tied. You could still have iOS 6+ projects with no iOS 64 support. What you can't have for sure is iOS 64 on iOS 5 projects.

On the other hand, I don't think CocoaPods should "break" projects by treating warnings as errors.
Just giving extra warnings should be enough to make developers upgrade their pods and/or use better maintained ones.

Owner
alloy commented Dec 3, 2013

Fair enough. I think we can switch to warnings for this.

Contributor
swizzlr commented Dec 3, 2013

inhibit_all_warnings! ought to suppress this?

Owner

@swizzlr I don't know.

The logic would be a bit complicated but the I started to get convinced that CocoaPods should use the build settings that Xcode had for a given deployment target. Also another solution would be to keep the current behavior and speed up the development of user configurable build settings which would allow to override the defaults.

Contributor
swizzlr commented Dec 3, 2013

Wait, hang on a minute, is my "-isystem" fix out yet? When that is, the pod targets should be built at the default level (their build stages will be properly siloed from the user targets). If the default level fails, that is the pod maintainer's fault and they ought to supply an xcconfig.

Owner

@swizzlr Your fix is not out yet. It is in master and will be part of the next release. However to my understanding it doesn't affect how the pods are built (after all is a flag used for the system frameworks which are already built). Even with the current implementation each Pod is built in dedicated target (with a default configuration that we set and any build setting specified in the podspec) and thus they are already silted from the build settings of the user project.

Contributor

The thing is most of our audience turn to CocoaPods because of its simplicity: pod 'Whatever' and your good.
So we should try to keep it that way as much as we can without extra build settings or steps.

Also asking for additional xcconfig files would make it more difficult to maintain for pod submitters.
And also those files' contents don't show up in the podpsec, so makes it more difficult to have a glance at a given pod problem.

Contributor
swizzlr commented Dec 4, 2013

the xcconfig attribute in a podspec is an officially supported part. Does anyone have "steps to reproduce"?

Contributor

Issue persists as of 0.29.0.

@rivera-ernesto rivera-ernesto added a commit to rivera-ernesto/cocoapods-integration-specs that referenced this issue Mar 12, 2014
@rivera-ernesto rivera-ernesto Make warning flags less aggressive: "Warn and treat as error" -> "Warn"
The following aggressive flags:

* `GCC_WARN_ABOUT_RETURN_TYPE: YES_ERROR`
* `CLANG_WARN_DIRECT_OBJC_ISA_USAGE: YES_ERROR`
* `CLANG_WARN_OBJC_ROOT_CLASS: YES_ERROR`

Break some Pods.

While in theory the concerned libraries could be updated, this happens with non-current, non-longer maintained version that will see no further updates (For instance PhoneGap/Cordova 2.x).

Users can manually clear the flags and it works but they have to do it every time they run a pod command.

This commit replaces those flags for regular warnings (non-treated as errors), which fulfills the original goal (highlight library potential issues) while not braking them.

Related to CocoaPods/CocoaPods#1629.
38d1df8
@rivera-ernesto rivera-ernesto referenced this issue in CocoaPods/cocoapods-integration-specs Mar 12, 2014
Closed

Make warning flags less aggressive: "Warn and treat as error" -> "Warn" #7

frosty commented Mar 29, 2014

@rivera-ernesto: Can you give an example of pods that are broken by these settings?

Issue has been confirmed by @neonichu

Contributor

Cordova 2.x. for instance. And because version 3.x changed may things many people still are using older versions.

I'll just chime in with a dissenting opinion--I'm in favor of warnings as errors. People should clean up the warnings--you can always push a clang diagnostic if you can't figure out any other way to get rid of them.

In one of my projects I went through a lot of trouble to remove each and every warning from our projects and turn on warnings as errors. Then someone was in a crunch and removed that flag so they could ship, and now our project is back to 35 warnings.

Contributor

Which is your code and you have direct control over it.

On the other hand breaking 3rd party code that you may not understand or be able to fix is not a good idea.
If we were to set them to errors we should remove all the libraries and old specs that have those warnings as they won't build anyway.

Personally I didn't encounter this issue again, so just giving my opinion.

@rivera-ernesto Thanks for your opinion. Makes sense that we don't want the CocoaPods tool to break old code and prevent it being used. Would be nice though if CocoaPods did enforce warnings as errors for new code somehow. Personally I did use a CocoaPod recently (UNIRest) that's introducing a warning into my project, and I find it unacceptable but haven't had time to fix it.

Seems like there could be a workaround as part of the Podspec to make warnings not errors in the specific projects where this is causing problems, instead of reducing the warning level across the board.

Owner

inhibit_warnings?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment