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

CB-14192: (ios) Add project settings to explicitly use legacy build s… #378

Closed
wants to merge 2 commits into from

Conversation

sebastiangrail
Copy link

This fixes the build in Xcode 10 and is backwards compatible with Xcode 9.4.
Note that Xcode 10 automatically adds a workspace to the xcodeproj.

See CB-14192

Platforms affected

iOS

What does this PR do?

Fixes the build using Xcode 10 without breaking Xcode 9 builds

What testing has been done on this change?

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.

…ystem

This fixes the build in Xcode 10 and is backwards compatible with Xcode 9.4.
Note that Xcode 10 automatically adds a workspace to the xcodeproj.
@dpogue
Copy link
Member

dpogue commented Aug 1, 2018

@shazron @sebastiangrail Do we have any idea what would be involved in making it compatible with the new build system? Is it likely that the new build system would cause problems for plugins or CocoaPods?

Generally I'm in favour of keeping working configurations working, but this is Apple and they like to deprecate things and add new requirements to App Store submissions and I have little trust in them to not drop the legacy build system in a point release of Xcode.

@janpio
Copy link
Member

janpio commented Sep 2, 2018

@sebastiangrail Could you maybe rebase this on master? This should fix the (unrelated) failing tests as we removed node4 from the test roster.

@codecov-io
Copy link

codecov-io commented Sep 3, 2018

Codecov Report

Merging #378 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #378   +/-   ##
=======================================
  Coverage   74.29%   74.29%           
=======================================
  Files          12       12           
  Lines        1564     1564           
=======================================
  Hits         1162     1162           
  Misses        402      402

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad96ef0...25f8bba. Read the comment docs.

@sgoldberg-sfdc
Copy link
Contributor

@dpogue I find if I restructure the project for the framework target such that its not wrapping the static lib, but instead compiling the source directly as part of the target, I do not see the failures with the new build system.

In addition, it is also odd that the failure occurs while building the static lib as a dependency of the framework target, yet if we build the static lib directly , it has no issue.

@mlegenhausen
Copy link
Contributor

mlegenhausen commented Sep 14, 2018

The Xcode 10 GM is avaibable.

The provided pull request does not work with the GM release, but I found another solution. You can disable the modern buildsystem with xcodebuild -UseModernBuildSystem=NO ....

After you added the switch get another error that "your project path" is not a valid command. It seems that in the case of archive it is not possible anymore to supply the project folder as last parameter. So I needed to make -workspace and -archivePath an absolute path. The options for the isDevice case need to be changed to

options = [
    '-UseModernBuildSystem=NO',
    '-xcconfig', customArgs.xcconfig || path.join(__dirname, '..', 'build-' + configuration.toLowerCase() + '.xcconfig'),
    '-workspace', customArgs.workspace || path.join(projectPath, projectName + '.xcworkspace'),
    '-scheme', customArgs.scheme || projectName,
    '-configuration', customArgs.configuration || configuration,
    '-destination', customArgs.destination || 'generic/platform=iOS',
    '-archivePath', customArgs.archivePath || path.join(projectPath, projectName + '.xcarchive')
];

Finally I removed the projectPath parameter from spawn('xcodebuild', xcodebuildArgs).The build should now work as expected.

Are you planing to which with cordova-ios to the new build systems? If not I can provide my fix as pull request.

@sgoldberg-sfdc
Copy link
Contributor

@mlegenhausen unfortunately that won't work with Carthage since you cannot provide parameters to the build command. If the workspace settings no longer work with the GM, we need to fix the root build issue.

@janpio
Copy link
Member

janpio commented Sep 14, 2018

@mlegenhausen If you have a working solution (even if it's just for you) please do create a PR for others to look at and use please.

@sgoldberg-sfdc
Copy link
Contributor

@shazron and @dpogue , Here is the issue with the New Build System.

The framework target is a wrapper on the static library. The static library has a copy headers build phase that is before the compilation build phase. If you move the copy headers phase to after the compilation phase, the new build system will successfully build the framework target one time (with a clean derived data folder). Unfortunately, subsequent builds will fail because the new build system is getting confused by the header files being in two places in the search path (project folder and CONFIGURATION_BUILD_DIR folder). There are several hacky solutions, but my recommendation is make the framework target a first class citizen, sharing the source files and compiling them as part of the target, rather than depending on the static library target. I can submit a PR on Monday.

@dpogue
Copy link
Member

dpogue commented Sep 14, 2018

@sgoldberg-sfdc I poked around very briefly this afternoon, but you clearly understand the details of the problem better than I do.

After telling Xcode to bring both the app project and the CordovaLib project to the recommended build settings, the new build system seemed to work fine. I see that it did change some stuff around Cordova.framework, but I admit to not totally understanding what that entails. The only changes were to xcodeproj files, no changes to source files.

@dpogue
Copy link
Member

dpogue commented Dec 17, 2018

We've merged support for the new build system in preparation for the next major version of cordova-ios, so I'm going to close this. Thanks for your contribution!

@dpogue dpogue closed this Dec 17, 2018
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.

6 participants