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

[NFC] Use angle-bracketed includes in public headers #12057

Closed
wants to merge 3 commits into from

Conversation

ncooke3
Copy link
Member

@ncooke3 ncooke3 commented Nov 6, 2023

No description provided.

Copy link
Contributor

github-actions bot commented Nov 6, 2023

Apple API Diff Report

Commit: 6930ece
Last updated: Tue Nov 7 06:25 PST 2023
View workflow logs & download artifacts


[BUILD ERROR] FirebaseFirestore


@google-oss-bot
Copy link

1 Warning
⚠️ Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

@google-oss-bot
Copy link

google-oss-bot commented Nov 6, 2023

Coverage Report 1

Affected Products

  • FirebaseFirestore-iOS-FirebaseFirestoreInternal.framework

    Overall coverage changed from 88.09% (a51b4d0) to 88.10% (6930ece) by +0.01%.

    FilenameBase (a51b4d0)Merge (6930ece)Diff
    leveldb_mutation_queue.cc92.42%93.56%+1.14%
    local_serializer.cc87.78%88.33%+0.56%
    ordered_code.cc94.39%93.90%-0.49%
  • FirebaseRemoteConfig-iOS-FirebaseRemoteConfig.framework

    Overall coverage changed from 72.24% (a51b4d0) to 72.27% (6930ece) by +0.04%.

    FilenameBase (a51b4d0)Merge (6930ece)Diff
    FIRRemoteConfig.m83.91%84.20%+0.29%

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/T8K9bejWc5.html

Comment on lines +17 to +18
#import <FirebaseCrashlytics/FIRExceptionModel.h>
//#import "Crashlytics/Crashlytics/Public/FirebaseCrashlytics/FIRExceptionModel.h"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When changing the imports from "..." → <...> in the public headers, this surfaced an issue when building the target's test specs (causing most/all of the pod-lib-lint failures on CI).

I also had to change imports of the public headers to the bracket notation to avoid the redefinition errors:
Screenshot 2023-11-07 at 9 23 37 AM

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the issue has to do with the public headers not forward declaring public types where they could be. It would be a breaking change to change that right now though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paulb777, I'm not too sure about this. Do you see anything troubling about changing all non-public imports of public headers from "..."<...>?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. And leaning towards punting since it's a large change on code freeze day.

@ncooke3
Copy link
Member Author

ncooke3 commented Nov 14, 2023

Closing. Will re-open when neeeded for Firebase 11.

@ncooke3 ncooke3 closed this Nov 14, 2023
@firebase firebase locked and limited conversation to collaborators Dec 14, 2023
@ncooke3
Copy link
Member Author

ncooke3 commented Jul 2, 2024

Linking to #11751

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants