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.

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

RUMM-669 Xcode 11.3.1 compilation fix #217

Merged
merged 1 commit into from
Aug 14, 2020
Merged

RUMM-669 Xcode 11.3.1 compilation fix #217

merged 1 commit into from
Aug 14, 2020

Conversation

buranmert
Copy link
Contributor

@buranmert buranmert commented Aug 13, 2020 •
edited
Loading

What and why?

Xcode 11.3.1 doesn't have iOS 13.4 as base SDK
therefore #available(iOS 13.4) closures don't compile

For more: #214

How?

We added compiler version macros as workaround

#if compiler(>=5.2)
  if #available(iOS 13.4, *) { newMethod() }
  else { legacy() }
#else
legacy()
#endif

Note: It's not trivial to run our tests in different Xcode versions with our CI provider

Screenshot 2020-08-13 at 14 19 59

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 requested a review from a team as a code owner August 13, 2020 12:23
@buranmert buranmert self-assigned this Aug 13, 2020
@buranmert buranmert force-pushed the buranmert/RUMM-669-xcode11-3-1-fix branch 3 times, most recently from b1425c4 to 0c6a12a Compare August 13, 2020 12:55
@buranmert buranmert added this to the hotfix-1.3.1 milestone Aug 13, 2020
ncreated
ncreated reviewed Aug 14, 2020
View reviewed changes
Sources/Datadog/Core/Persistence/Files/File.swift
Comment on lines +98 to +126
let data = fileHandle.readDataToEndOfFile()
try? objcExceptionHandler.rethrowToSwift {
fileHandle.closeFile()
}
return data
Copy link
Member

@ncreated ncreated Aug 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

nit: we can keep its original order

Suggested change
let data = fileHandle.readDataToEndOfFile()
try? objcExceptionHandler.rethrowToSwift {
fileHandle.closeFile()
}
return data
defer {
try? objcExceptionHandler.rethrowToSwift {
fileHandle.closeFile()
}
}
fileHandle.readDataToEndOfFile()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

previous code was closing fileHandle in defer block, this is the original order as readDataToEndOfFile doesn't throw and closeFile is called after reading

Copy link
Member

Choose a reason for hiding this comment

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

You're right. I updated my suggestion. Anyway, it does the same as the code in PR 👍.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it does the same thing, i go with shorter code 🩳

ncreated
ncreated requested changes Aug 14, 2020
View reviewed changes
Copy link
Member

@ncreated ncreated left a comment •
edited by buranmert
Loading

Choose a reason for hiding this comment

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

Why do we need all these changes in swizzling code?

@buranmert buranmert changed the base branch from master to hotfix/1.3.1 August 14, 2020 08:03
@buranmert
Copy link
Contributor Author

buranmert commented Aug 14, 2020 •
edited
Loading

@ncreated i changed the order of parameters in MethodSwizzler because apparently Xcode 11.3.1 has a problem with parameter ordering. it looks like we need to put the block parameter (instead of default value parameter) at the end

other Xcode versions don't have this issue

@buranmert buranmert requested a review from ncreated August 14, 2020 08:14
@buranmert
RUMM-669 Xcode 11.3.1 compilation fix
4402e4f 
Xcode 11.3.1 doesn't have iOS 13.4 as base SDK
We added compiler version macros as workaround
@buranmert buranmert force-pushed the buranmert/RUMM-669-xcode11-3-1-fix branch from 0c6a12a to 4402e4f Compare August 14, 2020 10:30
@buranmert buranmert merged commit ee29c6a into hotfix/1.3.1 Aug 14, 2020
@buranmert buranmert deleted the buranmert/RUMM-669-xcode11-3-1-fix branch August 14, 2020 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nachoBonafonte nachoBonafonte nachoBonafonte approved these changes

@ncreated ncreated ncreated approved these changes

Assignees

@buranmert buranmert

Labels
None yet
Projects
None yet
Milestone
hotfix-1.3.1
Development

Successfully merging this pull request may close these issues.
3 participants
@buranmert @ncreated @nachoBonafonte