search

rollbar / rollbar-ios

Objective-C library for crash reporting and logging with Rollbar.
https://docs.rollbar.com/docs/ios
MIT License
65 stars 61 forks source link

Deprecation warnings break app builds when "treat warnings as errors" is enabled #314

Closed sjmadsen closed 4 years ago

sjmadsen commented 4 years ago

All of the deprecation warnings recently added to the 1.12.x series generate a lot of warning noise in apps that import Rollbar.h. If the project uses the "treat warnings as errors" flag, those warnings also break the build.

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/swift -frontend ...

<module-includes>:1:9: note: in file included from <module-includes>:1:
#import "Headers/Rollbar.h"
        ^
/Users/.../Carthage/Build/iOS/Rollbar.framework/Headers/Rollbar.h:42:9: note: in file included from /Users/.../Carthage/Build/iOS/Rollbar.framework/Headers/Rollbar.h:42:
#import <Rollbar/RollbarFacade.h>
        ^
/Users/.../Carthage/Build/iOS/Rollbar.framework/Headers/RollbarFacade.h:5:9: note: in file included from /Users/.../Carthage/Build/iOS/Rollbar.framework/Headers/RollbarFacade.h:5:
#import "RollbarTelemetry.h"
        ^
/Users/.../Carthage/Build/iOS/Rollbar.framework/Headers/RollbarTelemetry.h:7:9: note: in file included from /Users/.../Carthage/Build/iOS/Rollbar.framework/Headers/RollbarTelemetry.h:7:
#import "RollbarTelemetryEvent.h"
        ^
/Users/.../Carthage/Build/iOS/Rollbar.framework/Headers/RollbarTelemetryEvent.h:20:36: error: 'DataTransferObject' is deprecated: In v2, use RollbarDTO instead.
@interface RollbarTelemetryEvent : DataTransferObject
                                   ^
/Users/.../Carthage/Build/iOS/Rollbar.framework/Headers/DataTransferObject.h:14:1: note: 'DataTransferObject' has been explicitly marked deprecated here
DEPRECATED_MSG_ATTRIBUTE("In v2, use RollbarDTO instead.")
^
/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator13.6.sdk/usr/include/AvailabilityMacros.h:182:64: note: expanded from macro 'DEPRECATED_MSG_ATTRIBUTE'
            #define DEPRECATED_MSG_ATTRIBUTE(s) __attribute__((deprecated(s)))
                                                               ^
<module-includes>:1:9: note: in file included from <module-includes>:1:
#import "Headers/Rollbar.h"
        ^
/Users/.../Carthage/Build/iOS/Rollbar.framework/Headers/Rollbar.h:42:9: note: in file included from /Users/.../Carthage/Build/iOS/Rollbar.framework/Headers/Rollbar.h:42:
#import <Rollbar/RollbarFacade.h>
        ^
/Users/.../Carthage/Build/iOS/Rollbar.framework/Headers/RollbarFacade.h:5:9: note: in file included from /Users/.../Carthage/Build/iOS/Rollbar.framework/Headers/RollbarFacade.h:5:
#import "RollbarTelemetry.h"
        ^
/Users/.../Carthage/Build/iOS/Rollbar.framework/Headers/RollbarTelemetry.h:8:9: note: in file included from /Users/.../Carthage/Build/iOS/Rollbar.framework/Headers/RollbarTelemetry.h:8:
#import "RollbarTelemetryBody.h"
        ^
/Users/.../Carthage/Build/iOS/Rollbar.framework/Headers/RollbarTelemetryBody.h:15:35: error: 'DataTransferObject' is deprecated: In v2, use RollbarDTO instead.
@interface RollbarTelemetryBody : DataTransferObject
                                  ^
/Users/.../Carthage/Build/iOS/Rollbar.framework/Headers/DataTransferObject.h:14:1: note: 'DataTransferObject' has been explicitly marked deprecated here
DEPRECATED_MSG_ATTRIBUTE("In v2, use RollbarDTO instead.")
^
/Applications/Xcode.app/Contents/Developer/Platforms/iPhoneSimulator.platform/Developer/SDKs/iPhoneSimulator13.6.sdk/usr/include/AvailabilityMacros.h:182:64: note: expanded from macro 'DEPRECATED_MSG_ATTRIBUTE'
            #define DEPRECATED_MSG_ATTRIBUTE(s) __attribute__((deprecated(s)))

(trimmed for brevity)

akornich commented 4 years ago

@sjmadsen , yes that is understandable. is there any alternative to the current deprecation warning approach that would allow communicating soon-upcoming API changes in v2 of our SDK while not using build warnings?

akornich commented 4 years ago

@sjmadsen , would you be able to add -Wno-error=deprecated to your build scripts on the build server to suppress the treatment of the deprecation warnings as errors?

sjmadsen commented 4 years ago

I don't have a problem with deprecations in general, but I only expect them to trigger if my code is using that API. In this case, the app is only importing Rollbar.h and the warnings are triggered by things used in other headers included by Rollbar.h. The library's code shouldn't trigger its own deprecation warnings; as soon as an interface is replaced and warrants a deprecation attribute, the library should already be using the replacement. Deprecations are primarily useful to warn third parties that an API usage is out of date.

We could include the -Wno-error=deprecated flag, but I think in the short term we'll just stick with 1.12.3, since most of the changes since then are related to CocoaPods packaging.

akornich commented 4 years ago

@sjmadsen, agree. we wanted to give heads up within the recent v1 releases about upcoming v2 changes, hence the deprecated warnings were added. btw, v2 alpha is already available (in both, GitHub Releases and as new Cocoapods: RollbarNotifier and RollbarDeploys) with the first beta coming out any day. Would you be ready to give it a try? At the moment crash reporting capability is missing from v2. We made it optional and crash reporting option via KSCrash will be added sometime during beta phase with some other alternative crash reporting options (for example, via PLCrash reporter, etc.)

Main features of v2:

sjmadsen commented 4 years ago

We're unlikely to ship a beta SDK in a production version regardless, but without crash reporting it definitely won't happen.

I understand what you're saying about the deprecations and giving developers a heads-up, but again in this case we're not using any of the deprecated interfaces. So for this app, there should be no warnings because the changes in 2.0 will not affect us.

A concrete example: RollbarTelemetryEvent inherits from DataTransferObject, which is now deprecated in favor of RollbarDTO. As a user of the SDK, I cannot fix this problem. This is an internal implementation detail of the Rollbar SDK itself. A deprecation warning should be something that I can fix to make the warning go away.

akornich commented 4 years ago

@sjmadsen , i see. Both, the legacy DataTransfetObject and the new RollbarDTO, are publicly available to the SDK users to create their own DTOs as needed as well as they are the base classes for other more concrete DTOs (like RollbarTelemetryEvent), hence, they are a part of these concrete types' interfaces - and not an internal implementational detail of the SDK. You are absolutely right, the majority of the deprecation warnings can be only eliminated by moving to v2 of the SDK (but the warnings do help you to evaluate how much of your code would be affected by upgrading to the v2). Thank you for the feedback provided here and we are looking forward to feedback about the new v2 SDK API (once you get a chance to evaluate it). Best regards!

jacobh0 commented 4 years ago

I want to echo this statement by @sjmadsen:

As a user of the SDK, I cannot fix this problem. This is an internal implementation detail of the Rollbar SDK itself. A deprecation warning should be something that I can fix to make the warning go away.

In what is supposed to be a bug fix according to semver... deprecation warnings were introduced that users of the SDK cannot actually resolve? That is not helpful. If there is an actual action we can take to mitigate the warnings.. sure, but this is an improper use of a deprecation warning (there is no alternative).

We have to lock the version at v1.12.8 to get around this

piercifani commented 4 years ago

Are there any updates on this? It's really a pain to see a bunch of warnings on your project and know the there's nothing you can do about it.

Classes are marked as deprecated and the replacement offered has no public API available to initialise the SDK

image
jacobh0 commented 4 years ago

@piercifani they are unlikely to do anything about it. I recommend fixing your version to v1.12.8 for now (1 "bug" release behind current, with the only diff being these warnings).

piercifani commented 4 years ago

@grioja it's a shame how this library is maintained...

sjmadsen commented 4 years ago

@akornich Thank you for the changes in #320. I saw a comment on that PR that you plan to bring the deprecation warnings back in a final v1 release, once v2 is out of beta. This may be asking too much, but would you also please ensure that the SDK's internal implementation is moved to the new, non-deprecated API? This would ensure that only those users who are implementing their own subclasses of deprecated API see the warnings.

akornich commented 4 years ago

@sjmadsen , if understand you correctly, are you asking about keeping v1 implementation side by side with the new v2 implementation in upcoming v2 official releases?

sjmadsen commented 4 years ago

No, I'm asking that if you publish a final v1 release with the deprecations re-enabled, that the SDK itself has migrated to the new API.

akornich commented 4 years ago

yes, that is the plan. Basically:

Does it make sense?

sjmadsen commented 4 years ago

What you write makes sense, but it's unclear if you plan to migrate concrete SDK classes away from deprecated API as part of that last v1 release.

Using the specific case from above, since I'm struggling to communicate my concern: do your plans for the last v1 release include migrating RollbarTelemetryEvent away from DataTransferObject and to RollbarDTO?

if your answer is no, then that last v1 release will cause the same problems for people that led to my opening this issue. You can call it a transitional release, but as long as its version is < 2.0, many package managers will see it as a safe upgrade and people will almost certainly have the same problems with unexpected warnings when they build, even when their apps are not using the deprecated API.

I understand what you're trying to do with a migration path, but if you want to follow semantic versioning and the deprecated API still exists in v2, the first v2 release is the place to deprecate it. People can migrate away for the life of v2, and the old API can disappear entirely in v3.

jacobh0 commented 4 years ago

Deprecation warnings serve their purpose only if you can take direct action on them within the immediate version.

Having unreconcilable warnings is not useful to any consumer.

There is an inherent expectation when upgrading from 1.x to 2.x that there will be potential breaking changes. You could simply highlight the intended migration path in your docs and not even rely on a deprecation warning if 2.x deletes the old code path entirely.

At any rate, if you introduce warnings.. let us take action on them, otherwise do not introduce them. If you want to deprecate the old type of configuration in 1.x then please allow us to move to the new configuration model in 1.x