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

Converge all three platforms' projects #633

Merged
merged 18 commits into from
May 29, 2020
Merged

Conversation

robinmacharg
Copy link
Contributor

@robinmacharg robinmacharg commented May 27, 2020

Goal

The Bugsnag library has, for some time, been supplied as three separate projects due to a historic issue with multi-platform Carthage support. The Carthage issue has been resolved and there is no reason that the project structure cannot be simplified back to one XCode project that supports all three platforms (iOS, macOS, tvOS)

Design

iOS was used as the base project. Additional targets were created for tvOS and macOS, along with test targets. Side by side comparison with existing tvOS and macOS (OSX) projects were made while populating the new targets. Comparison with other established cross-platform open source frameworks also guided work. iOS was renamed to Project. Rebased onto v6 during development to simplify final merging.

Changeset

  • Added new build and test targets to the (formerly) iOS project.
  • tvOS and OSX projects removed.
  • .cpp file specifier removed from podspec. This supplants this PR.

Tests

  • Frequent local and remote testing of both Carthage and Cocoapods setups using the example apps.
  • Building from-scratch projects pointing at the specific remote branch.

Notes

  • Documentation will need to be updated.
  • Changelog not added since this does not materially affect the end-users' experience of the framework.
  • CI may require updating if it makes any assumptions around directory names and/or specific build instructions.

…l unit tests

- Removed .cpp source file specifier from podspec
- Unshared schemes
- Remove tvOS and OSX projects
- Renamed iOS project to Project
…l unit tests

- Removed .cpp source file specifier from podspec
- Unshared schemes
- Remove tvOS and OSX projects
- Renamed iOS project to Project
Robin Macharg and others added 2 commits May 28, 2020 09:13
…l unit tests

- Removed .cpp source file specifier from podspec
- Unshared schemes
- Remove tvOS and OSX projects
- Renamed iOS project to Project
@robinmacharg robinmacharg changed the title Converge all three platforms to what was iOS, building and passing al… Converge all three platforms May 28, 2020
@robinmacharg robinmacharg changed the title Converge all three platforms Converge all three platforms' projects May 28, 2020
…cocoa into v6-merge-xcode-projects

# Conflicts:
#	Project/Bugsnag copy-Info.plist
@robinmacharg robinmacharg marked this pull request as ready for review May 28, 2020 15:45
Copy link
Contributor

@fractalwrench fractalwrench left a comment

Choose a reason for hiding this comment

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

Good to finally address this 🎉

As this doesn't lend itself well to inline comments, I've left queries here:

  • Frameworks and libraries are different between targets, for tvOS in particular - I assume this is expected behaviour
  • Deployment targets do not match supported versions
  • Why are the C/C++ language dialect build settings different for iOS/macOS/tvOS/BugsnagStatic?
  • macOS target has an empty Run Script in its build phases
  • Generally speaking, it'd be nice to set default values for Build Settings in the Bugsnag Project, so that each target inherits the value and doesn't need to override anything.
  • The test targets each have a different number of test files in the Compile Sources
  • The copy files phase for BugsnagStatic does not copy the same number of headers as the iOS target has
  • Would suggest moving a few more files into groups for logical organisation. E.g. BugsnagApiClient could go in the Delivery group etc

There are also a couple of follow on tasks/questions:

  • Review Apple Clang/Static analysis flags, there's probably at least a few new ones that are worth enabling
  • When switching between targets and building, I ocassionally encountered an error with Derived Data where it failed to create a directory to put the headers in. Cleaning the build folder tended to work but it seems like this would be worth keeping an eye on
  • Are MessageUi + UIKit required for the BugsnagStatic target? They're not present in the regular iOS target

And finally, there's a list of things that could use some manual testing to verify they're working if this hasn't already been done:

  • Upgrading from previous installation
  • Carthage
  • React Native
  • Archiving iOS app and installing on real device

@robinmacharg
Copy link
Contributor Author

robinmacharg commented May 29, 2020

Thanks for the comments; a good list to work through. Let me address some of these in turn, Updating this comment as I go.

  • Frameworks and libraries are different between targets, for tvOS in particular - I assume this is expected behaviour - ✅ Fixed
  • Deployment targets do not match supported versions - ✅
  • Why are the C/C++ language dialect build settings different for iOS/macOS/tvOS/BugsnagStatic? - ✅
  • macOS target has an empty Run Script in its build phases ✅
  • Generally speaking, it'd be nice to set default values for Build Settings in the Bugsnag Project, so that each target inherits the value and doesn't need to override anything. - ✅ Mostly done. There are a couple that require further scrutiny; left as they are to be on the safe side
  • The test targets each have a different number of test files in the Compile Sources - Added missing KSCrash tests to tv and mac targets. Omitted Swift tests from tvOS due to Swift version incompatibilities on CI. There may be a way to specify the SWIFT_VERSION in the Makefile but that was deemed non-critical at this point.
  • The copy files phase for BugsnagStatic does not copy the same number of headers as the iOS target has - ✅
  • Would suggest moving a few more files into groups for logical organisation. E.g. BugsnagApiClient could go in the Delivery group etc ✅ I'm sure there are more.

There are also a couple of follow on tasks/questions:

  • Review Apple Clang/Static analysis flags, there's probably at least a few new ones that are worth enabling - There's a separate ticket for this; I believe it can be addressed at a separate time.
  • When switching between targets and building, I ocassionally encountered an error with Derived Data where it failed to create a directory to put the headers in. Cleaning the build folder tended to work but it seems like this would be worth keeping an eye on - Noted and confirmed. I think this is due to using a common build directory; I've not seen it recently, but will keep an eye out.
  • Are MessageUi + UIKit required for the BugsnagStatic target? They're not present in the regular iOS target - I don't believe so, but will remove and confirm that the static library functionas as expected.

And finally, there's a list of things that could use some manual testing to verify they're working if this hasn't already been done:

  • Upgrading from previous installation
  • Carthage ✅
  • React Native
  • Archiving iOS app and installing on real device ✅

Copy link
Contributor

@fractalwrench fractalwrench left a comment

Choose a reason for hiding this comment

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

LGTM pending CI.

@robinmacharg robinmacharg merged commit e107014 into v6 May 29, 2020
@robinmacharg robinmacharg deleted the v6-merge-xcode-projects branch May 29, 2020 16:42
This was referenced May 29, 2020
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