Skip to content

Don't generate missing Analytics warning on Catalyst #7719

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 15, 2021
Merged

Don't generate missing Analytics warning on Catalyst #7719

merged 1 commit into from
Mar 15, 2021

Conversation

paulb777
Copy link
Member

@paulb777 paulb777 commented Mar 15, 2021 •
edited
Loading

Fix #7693

Don't generate a warning for missing Analytics when including Performance for Catalyst.

Relatedly and separately the Perf and RC teams should work out the strategy for these warnings on iOS. See #7487

@google-cla google-cla bot added the cla: yes label Mar 15, 2021
@paulb777 paulb777 mentioned this pull request Mar 15, 2021
Closed
@google-oss-bot
Copy link

Coverage Report

Affected SDKs

  • FirebaseRemoteConfig-iOS-FirebaseRemoteConfig.framework

    SDK overall coverage changed from ? (0175b86) to 81.14% (124ade3) by ?.

    Click to show coverage changes in 11 files.
    Filename Base (0175b86) Head (124ade3) Diff
    FIRConfigValue.m ? 58.70% ?
    FIRRemoteConfig.m ? 84.07% ?
    FIRRemoteConfigComponent.m ? 96.05% ?
    RCNConfigContent.m ? 80.34% ?
    RCNConfigDBManager.m ? 77.99% ?
    RCNConfigExperiment.m ? 92.91% ?
    RCNConfigFetch.m ? 81.70% ?
    RCNConfigSettings.m ? 70.14% ?
    RCNDevice.m ? 86.15% ?
    RCNPersonalization.m ? 91.30% ?
    RCNUserDefaultsManager.m ? 97.99% ?

Test Logs

@paulb777 paulb777 merged commit 7964564 into master Mar 15, 2021
@paulb777 paulb777 deleted the h-catalyst branch March 15, 2021 17:27
@jtaby
Copy link
Contributor

jtaby commented Mar 30, 2021

@paulb777 I believe this PR is incorrect, the preprocessor macro for catalyst is TARGET_OS_MACCATALYST, not TARGET_OS_CATALYST as this PR suggests.

This is validated by the system-provided TargetConditionals.h header which states:

 *  TARGET_OS_*
 *  These conditionals specify in which Operating System the generated code will
 *  run.  Indention is used to show which conditionals are evolutionary subclasses.
 *
 *  The MAC/WIN32/UNIX conditionals are mutually exclusive.
 *  The IOS/TV/WATCH conditionals are mutually exclusive.
 *
 *
 *      TARGET_OS_WIN32           - Generated code will run under 32-bit Windows
 *      TARGET_OS_UNIX            - Generated code will run under some Unix (not OSX)
 *      TARGET_OS_MAC             - Generated code will run under Mac OS X variant
 *         TARGET_OS_OSX          - Generated code will run under OS X devices
 *         TARGET_OS_IPHONE          - Generated code for firmware, devices, or simulator
 *            TARGET_OS_IOS             - Generated code will run under iOS
 *            TARGET_OS_TV              - Generated code will run under Apple TV OS
 *            TARGET_OS_WATCH           - Generated code will run under Apple Watch OS
 *            TARGET_OS_BRIDGE          - Generated code will run under Bridge devices
 *            TARGET_OS_MACCATALYST     - Generated code will run under macOS
 *         TARGET_OS_SIMULATOR      - Generated code will run under a simulator
 *
 *      TARGET_OS_EMBEDDED        - DEPRECATED: Use TARGET_OS_IPHONE and/or TARGET_OS_SIMULATOR instead
 *      TARGET_IPHONE_SIMULATOR   - DEPRECATED: Same as TARGET_OS_SIMULATOR
 *      TARGET_OS_NANO            - DEPRECATED: Same as TARGET_OS_WATCH
 *
 *    +---------------------------------------------------------------------+
 *    |                            TARGET_OS_MAC                            |
 *    | +---+ +-----------------------------------------------+ +---------+ |
 *    | |   | |               TARGET_OS_IPHONE                | |         | |
 *    | |   | | +---------------+ +----+ +-------+ +--------+ | |         | |
 *    | |   | | |      IOS      | |    | |       | |        | | |         | |
 *    | |OSX| | |+-------------+| | TV | | WATCH | | BRIDGE | | |DRIVERKIT| |
 *    | |   | | || MACCATALYST || |    | |       | |        | | |         | |
 *    | |   | | |+-------------+| |    | |       | |        | | |         | |
 *    | |   | | +---------------+ +----+ +-------+ +--------+ | |         | |
 *    | +---+ +-----------------------------------------------+ +---------+ |
 *    +---------------------------------------------------------------------+

jtaby added a commit to bergenco/firebase-ios-sdk that referenced this pull request Mar 30, 2021
@jtaby
Use correct preprocessor macro for Mac Catalyst
b8fc84a 
This was introduced in firebase#7719 to fix a warning that was generated when using Remote Config on Catalyst, however, the PR was not using the correct preprocessor macro for Mac Catalyst, as defined in TargetConditionals.h
@jtaby jtaby mentioned this pull request Mar 30, 2021
Closed
jtaby added a commit to bergenco/firebase-ios-sdk that referenced this pull request Mar 30, 2021
@jtaby
Use correct preprocessor macro for Mac Catalyst
53bb6f0 
This was introduced in firebase#7719 to fix a warning that was generated when using Remote Config on Catalyst, however, the PR was not using the correct preprocessor macro for Mac Catalyst, as defined in TargetConditionals.h
@jtaby jtaby mentioned this pull request Mar 30, 2021
Merged
paulb777 pushed a commit that referenced this pull request Mar 30, 2021
@jtaby
Use correct preprocessor macro for Mac Catalyst (#7812)
ec64786 
This was introduced in #7719 to fix a warning that was generated when using Remote Config on Catalyst, however, the PR was not using the correct preprocessor macro for Mac Catalyst, as defined in TargetConditionals.h

Co-authored-by: Majd Taby <[email protected]>
@firebase firebase locked and limited conversation to collaborators Apr 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ryanwilson ryanwilson ryanwilson approved these changes

@visumickey visumickey visumickey approved these changes

@karenyz karenyz karenyz approved these changes

Assignees
No one assigned
Labels
api: core api: remoteconfig cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Firebase Performance warns me to add Analytics, which is not available on Catalyst
6 participants
@paulb777 @google-oss-bot @jtaby @ryanwilson @visumickey @karenyz