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

V6 rebuild project #670

Merged
merged 28 commits into from
Jun 5, 2020
Merged

V6 rebuild project #670

merged 28 commits into from
Jun 5, 2020

Conversation

robinmacharg
Copy link
Contributor

@robinmacharg robinmacharg commented Jun 3, 2020

Goal

Previous work to combine three separate Xcode projects didn't go far enough and left a folder structure that didn't follow Apple best-practice and, further, wasn't amenable to consistent install instructions. This work rebuilds the entire project structure to address these issues.

Design

  • A new Xcode iOS framework project was created.
  • Framework targets for each platform were added, along with tests and a static target.
  • Build settings from the previous project merge were copied across
  • Source files were moved from existing locations to preserve history, with additional directory structure added. Tests were also re-added. A single redundant Swift test was elided.

Commits were granular and show the progression towards a stable state. Pushes were frequent to ensure that CI issues were addressed. Builds were attempted frequently locally. Popular open source frameworks were consulted to ensure best (common) practice was followed as much as possible.

Changeset

  • Project file added at top level, source files moved; this generates a large changeset that may be better reviewed as a checkout. v6 changes were merged in to ensure that the two branches did not diverge too greatly.
  • Makefile adjusted for new source path

Tests

Manual install, Cocoapods, Carthage both local and remote.

@robinmacharg robinmacharg force-pushed the v6-rebuild-project branch 2 times, most recently from 55ec8c9 to 52f0705 Compare June 3, 2020 15:40
@robinmacharg robinmacharg marked this pull request as ready for review June 3, 2020 15:42
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.

Noticed a few things that differ compared to the previous project that I believe need to change:

  • The Link Binary with Libraries step appears to be missing the frameworks from the previous project
  • The BugsnagStatic target is missing a copy headers build phase, which means headers will not be available for React Native
  • The BugsnagStatic target is missing around half the required .m files from the Compile Sources Build phase
  • The test targets have an inconsistent number of files in their build phase

I've tested this by confirming that it works in the Obj-C example project - we would need to test against all our installation instructions before merging this.

@fractalwrench
Copy link
Contributor

Updated PR to address the review points raised above - I've tested out on an example app using Cocoapods only. @tomlongridge would be good to get another pair of eyes reviewing this.

Copy link
Contributor

@tomlongridge tomlongridge left a comment

Choose a reason for hiding this comment

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

Do we need Configurations/Config.xcconfig any more? I believe this was used to share config across projects (but was causing the manual install instructions to break), so I believe it is now unused.

@fractalwrench
Copy link
Contributor

Configurations/Config.xcconfig does not appear to be necessary - I've deleted this and rebased the PR to resolve conflicts from the deadlock fix merged into v6.

@fractalwrench fractalwrench merged commit f778143 into v6 Jun 5, 2020
@fractalwrench fractalwrench deleted the v6-rebuild-project branch June 5, 2020 09:48
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.

3 participants