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

RUMM-671 DatadogObjC target is fixed for SPM #220

Merged
merged 1 commit into from
Aug 14, 2020

Conversation

buranmert
Copy link
Contributor

What and why?

DatadogObjC wasn't compiling with @objc attribute used without importing module 'Foundation' errors
This only happens in Swift Package Manager; other dependency managers work fine
For more info: #218

How?

I added import Foundation lines to source files.
I also made dep-manager-test/spm import DatadogObjC instead of Datadog, this should test both targets at once.
Note: I'm still investigating how come Xcode manages to build the project without those imports 🤔

Release info

This PR is branched off from 1.3.0 tag and to be merged into master
We will ship this change as a hotfix once it is merged

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference

@buranmert buranmert added this to the hotfix-1.3.1 milestone Aug 13, 2020
@buranmert buranmert requested a review from a team as a code owner August 13, 2020 16:56
@buranmert buranmert self-assigned this Aug 13, 2020
@buranmert buranmert changed the base branch from master to buranmert/RUMM-669-xcode11-3-1-fix August 13, 2020 16:57
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

Have you tried adding

linkerSettings: [
   .linkedFramework("Foundation")
]

to the DatadogObjc target in Package.swift? If it solves the problem, this should be IMO preferred solution. Otherwise, we could easily miss import Foundation in any new file added to the DatadogObjc and the issue will be back there.

@ncreated
Copy link
Member

Release info

This PR is branched off from 1.3.0 tag and to be merged into master
We will ship this change as a hotfix once it is merged

If we plan to include two patches in a hotfix, please create the hotfix/1.3.1 branch and point both PRs to it. This way we'll have two merge commits, keeping the git history right. Otherwise, both patches will be included in a single, RUMM-669 merge commit, which is not correct.

@buranmert
Copy link
Contributor Author

buranmert commented Aug 14, 2020

@ncreated .linkedFramework("Foundation") didn't work and produced the same errors.
also it's mostly for C++ based libraries and non-modular libraries according to docs and only very few projects use it
we cannot easily miss that because otherwise dependency-manager-tests-spm will fail ❌

it's weird that Xcode can compile the project, when i create new project it fails when import Foundation is absent 🤔

UPDATE: hotfix/1.3.1 is here and fixes have it as their target branch ✅

SPM Dep Manager test now imports DDObjC instead of DD
DDObjC links DD statically so it contains DD inside
@buranmert buranmert force-pushed the buranmert/RUMM-671-ObjC-broken-in-SPM branch from 896fa0a to 80cbf8b Compare August 14, 2020 08:03
@buranmert buranmert changed the base branch from buranmert/RUMM-669-xcode11-3-1-fix to hotfix/1.3.1 August 14, 2020 08:03
@buranmert buranmert requested a review from ncreated August 14, 2020 08:04
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

it's weird that Xcode can compile the project, when i create new project it fails when import Foundation is absent 🤔

I guess it may depend on the Xcode version used to create the project.

@buranmert buranmert merged commit f4ba8f1 into hotfix/1.3.1 Aug 14, 2020
@buranmert buranmert deleted the buranmert/RUMM-671-ObjC-broken-in-SPM branch August 14, 2020 10:18
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.

2 participants