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

fix: Solve the "Double-quoted include" warnings #2051

Closed
wants to merge 4 commits into from

Conversation

kevinrenskers
Copy link
Contributor

📜 Description

Fixes all the "Double-quoted include" warnings. You could easily trigger them by building the ObjC sample app.

💡 Motivation and Context

Closes #1072

💚 How did you test it?

Compiling the app and the sample apps still works, all the tests still work.

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • Review from the native team if needed
  • No breaking changes

🔮 Next steps

@kevinrenskers
Copy link
Contributor Author

Integration tests fail on #import <Sentry/SentryDefines.h>. Not sure why, as locally it all works, also when building the sample apps. @brustolin any ideas?

@brustolin
Copy link
Contributor

brustolin commented Aug 9, 2022

Integration tests fail on #import <Sentry/SentryDefines.h>. Not sure why, as locally it all works, also when building the sample apps. @brustolin any ideas?

Nope. I tried to do this too some time ago and SPM is not happy. And SPM is a must for us.

@kevinrenskers
Copy link
Contributor Author

Unless someone else has some insight into this I guess we close this and live with the warnings? But surely other SDKs don't generate the same kind of warnings so this should be possible to solve I would think.

@armcknight
Copy link
Member

If we're adding the Sentry SDK directly to one of the AlamoFire example projects, you could try integrating it via CocoaPods/Carthage/SPM instead, both to see if the resultant changes still match what is in our patchfile, and to see if it passes in CI also/anyways.

Our current approach of applying a patch file is more brittle than that so seems like it'd also leave things in a better state if we did that moving forwards.

@kevinrenskers
Copy link
Contributor Author

I've looked at integration-tests.yml and add-sentry-to-alamofire.patch, and I have a suspicion that it's already integrated with SPM? Since there's mention of a XCRemoteSwiftPackageReference with the URL https://github.com/getsentry/sentry-cocoa.. so I don't think we're adding the Sentry SDK directly as you said?

+/* Begin XCRemoteSwiftPackageReference section */
+		7B4C98B32743C1FE00B03EC9 /* XCRemoteSwiftPackageReference "sentry-cocoa" */ = {
+			isa = XCRemoteSwiftPackageReference;
+			repositoryURL = "https://github.com/getsentry/sentry-cocoa";
+			requirement = {
+				kind = revision;
+				revision = __GITHUB_REVISION_PLACEHOLDER__;
+			};
+		};
+/* End XCRemoteSwiftPackageReference section */
+
+/* Begin XCSwiftPackageProductDependency section */
+		7B4C98B42743C1FE00B03EC9 /* Sentry */ = {
+			isa = XCSwiftPackageProductDependency;
+			package = 7B4C98B32743C1FE00B03EC9 /* XCRemoteSwiftPackageReference "sentry-cocoa" */;
+			productName = Sentry;
+		};
+/* End XCSwiftPackageProductDependency section */

This looks similar to me as a test project I created and added sentry-cocoa via SPM.

@brustolin
Copy link
Contributor

brustolin commented Aug 10, 2022

and I have a suspicion that it's already integrated with SPM?

Yes. The problem is not the integration, the problem is the way SPM is build.
All you need to do is make sure swift build works which based on this action Build / Build with Swift (pull_request) is not.
I really think this worth an extra investigation, it would be nice to have this solved, but if this is taking too long we close the PR, this is definitely not a high priority.

@kevinrenskers kevinrenskers marked this pull request as draft August 12, 2022 09:52
@armcknight
Copy link
Member

Hm, it sounds like we might need something like #1726 first. If SPM links Sentry statically, then framework-style imports probably won't work.

@brustolin
Copy link
Contributor

Hm, it sounds like we might need something like #1726 first. If SPM links Sentry statically, then framework-style imports probably won't work.

But even then this won't work, I think, because the static lib options will still be available

@github-actions
Copy link

github-actions bot commented Sep 7, 2022

This pull request has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!


"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

@philipphofmann
Copy link
Member

#1726 was merged, @kevinrenskers can you please revisit this PR?

@kevinrenskers kevinrenskers reopened this Sep 15, 2022
@kevinrenskers
Copy link
Contributor Author

Will do

* master: (73 commits)
  ref: Fix typo in measurement (#2154)
  Fix typos in changelog (#2153)
  docs: Add a note on flaky tests to Contributing (#2161)
  docs: fix PR reference in CHANGELOG (#2157)
  test: generate graphs for benchmarks (#2158)
  release: 7.25.1
  fix: Prewarmed app start detection (#2151)
  ci: temporarily disable scheduled CI runs uploading profiles/symbols (#2152)
  Run tests on iOS 16 using Xcode 14.0 (#2147)
  ref: Fix Xcode 14 compile issues (#2145)
  Provide fallbacks for assertions in production (#2141)
  Fix compile errors in Xcode 14 (#2146)
  release: 7.25.0
  fix: setting SDK name through options[sdk][name] shouldn't clear version (#2139)
  fix: SentryTracer instances should be called "tracer", not "transaction" (#2137)
  ref: functions to convert to/from enum cases (#2108)
  fix: Crash with screenshot is reported twice (#2134)
  meta: Clarify PR rules in Contributing (#2133)
  meta: Fix Changelog (#2136)
  feat: Add support for dynamic library (#1726)
  ...
@kevinrenskers
Copy link
Contributor Author

We're still getting fatal errors like fatal error: 'Sentry/SentryDefines.h' file not found.

Anyone got an idea on how to fix this? I'm really not experienced with frameworks and static/dynamic libs and how that works with SPM etc etc.

But @brustolin did predict this:

But even then this won't work, I think, because the static lib options will still be available

So not sure if anything can be done.

@philipphofmann
Copy link
Member

I also don't know how to fix this right now, and sadly I don't have the bandwidth to look at this. We can also close this PR, if we can't figure it out right now.

@philipphofmann philipphofmann deleted the fix/1072-fix-double-quote-warnings branch March 4, 2024 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix all "Double-quoted include in framework header, expected angle-bracketed instead" warnings
4 participants