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

fix(ios): ignore the mac build if it failed and exclude sim arm64 if thirdparty .framework is there #12093

Merged
merged 1 commit into from
Sep 22, 2020
Merged

fix(ios): ignore the mac build if it failed and exclude sim arm64 if thirdparty .framework is there #12093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
36 changes: 20 additions & 16 deletions iphone/cli/commands/_build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ const appc = require('node-appc'),
__n = i18n.__n,
parallel = appc.async.parallel,
series = appc.async.series,
plist = require('simple-plist'),
version = appc.version;
const platformsRegExp = new RegExp('^(' + ti.allPlatformNames.join('|') + ')$'); // eslint-disable-line security/detect-non-literal-regexp
const pemCertRegExp = /(^-----BEGIN CERTIFICATE-----)|(-----END CERTIFICATE-----.*$)|\n/g;
Expand Down Expand Up @@ -2225,29 +2226,32 @@ iOSBuilder.prototype.validate = function validate(logger, config, cli) {
module.libFile = path.join(module.modulePath, module.libName);
module.isFramework = true;

// TODO: read Info.plist to get the full scope of targets/arches supported!
let archDir = 'ios-arm64_i386_x86_64-simulator';
if (!fs.existsSync(path.join(module.libFile, archDir))) {
// Try XCode 11 dir w/o arm64 support
archDir = 'ios-i386_x86_64-simulator';
this.legacyModules.add(module.id);// Record that this won't support arm64 sim!
const xcFrameworkInfo = plist.readFileSync(path.join(module.libFile, 'Info.plist'));
for (const libInfo of xcFrameworkInfo.AvailableLibraries) {
if (libInfo.SupportedPlatformVariant === undefined) {
// Device library is used for hash calculation.
// TODO: Probably we want to add other varient's library as well.
nativeHashes.push(module.hash = this.hash(fs.readFileSync(path.join(module.libFile, libInfo.LibraryIdentifier, 'lib' + module.id.toLowerCase() + '.a'))));
} else if (libInfo.SupportedPlatformVariant === 'simulator' && !libInfo.SupportedArchitectures.includes('arm64')) {
this.legacyModules.add(module.id);// Record that this won't support arm64 sim!
}
}
// TODO: Change hash calculation
nativeHashes.push(module.hash = this.hash(fs.readFileSync(path.join(module.libFile, archDir, 'lib' + module.id.toLowerCase() + '.a'))));
} else if (fs.existsSync(path.join(module.modulePath, xcFrameworkOfFramework))) {
module.libName = xcFrameworkOfFramework;
module.libFile = path.join(module.modulePath, module.libName);
module.isFramework = true;

// TODO: read Info.plist to get the full scope of targets/arches supported!
let archDir = 'ios-arm64_i386_x86_64-simulator';
if (!fs.existsSync(path.join(module.libFile, archDir))) {
// Try XCode 11 dir w/o arm64 support
archDir = 'ios-i386_x86_64-simulator';
this.legacyModules.add(module.id);// Record that this won't support arm64 sim!
const xcFrameworkInfo = plist.readFileSync(path.join(module.libFile, 'Info.plist'));
const scrubbedModuleId = this.scrubbedModuleId(module.id);
for (const libInfo of xcFrameworkInfo.AvailableLibraries) {
if (libInfo.SupportedPlatformVariant === undefined) {
// Device library is used for hash calculation.
// TODO: Probably we want to add other varient's library as well.
nativeHashes.push(module.hash = this.hash(fs.readFileSync(path.join(module.libFile, libInfo.LibraryIdentifier, scrubbedModuleId + '.framework', scrubbedModuleId))));
} else if (libInfo.SupportedPlatformVariant === 'simulator' && !libInfo.SupportedArchitectures.includes('arm64')) {
this.legacyModules.add(module.id);// Record that this won't support arm64 sim!
}
}
// TODO: Change hash calculation
nativeHashes.push(module.hash = this.hash(fs.readFileSync(path.join(module.libFile, archDir, this.scrubbedModuleId(module.id) + '.framework', this.scrubbedModuleId(module.id)))));
} else {
this.logger.error(__('Module %s (%s) is missing library or framework file.', module.id.cyan, (module.manifest.version || 'latest').cyan) + '\n');
this.logger.error(__('Please validate that your module has been packaged correctly and try it again.'));
Expand Down
225 changes: 153 additions & 72 deletions iphone/cli/commands/_buildModule.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

'use strict';

const { exec, spawn } = require('child_process'); // eslint-disable-line security/detect-child-process
const { spawn } = require('child_process'); // eslint-disable-line security/detect-child-process

const appc = require('node-appc'),
archiver = require('archiver'),
Expand All @@ -29,7 +29,11 @@ const appc = require('node-appc'),
temp = require('temp'),
util = require('util'),
__ = appc.i18n(__dirname).__,
series = appc.async.series;
series = appc.async.series,
xcode = require('xcode');

const plist = require('simple-plist');
const parsePlist = util.promisify(plist.readFile);

function iOSModuleBuilder() {
Builder.apply(this, arguments);
Expand All @@ -47,6 +51,7 @@ iOSModuleBuilder.prototype.validate = function validate(logger, config, cli) {
this.moduleName = cli.manifest.name;
this.moduleIdAsIdentifier = this.scrubbedName(this.moduleId);
this.moduleVersion = cli.manifest.version;
this.isMacOSEnabled = this.detectMacOSTarget();
this.moduleGuid = cli.manifest.guid;
this.isFramework = fs.existsSync(path.join(this.projectDir, 'Info.plist')); // TODO: There MUST be a better way to determine if it's a framework (Swift)

Expand Down Expand Up @@ -86,6 +91,32 @@ iOSModuleBuilder.prototype.validate = function validate(logger, config, cli) {
}.bind(this);
};

iOSModuleBuilder.prototype.detectMacOSTarget = function detectMacOSTarget() {
if (this.manifest.mac !== undefined) {
return this.manifest.mac === 'true';
}
const pbxFilePath = path.join(this.projectDir, `${this.moduleName}.xcodeproj`, 'project.pbxproj');

if (!fs.existsSync(pbxFilePath)) {
this.logger.warn(__(`The Xcode project does not contain the default naming scheme. This is required to detect macOS targets in your module.\nPlease rename your Xcode project to "${this.moduleName}.xcodeproj"`));
return true;
}

let isMacOSEnabled = true;
const proj = xcode.project(pbxFilePath).parseSync();
const configurations = proj.hash.project.objects.XCBuildConfiguration;

for (const key of Object.keys(proj.hash.project.objects.XCBuildConfiguration)) {
const configuration = configurations[key];
if (typeof configuration === 'object' && configuration.buildSettings.SUPPORTS_MACCATALYST) {
isMacOSEnabled = configuration.buildSettings.SUPPORTS_MACCATALYST === 'YES';
break;
}
}

return isMacOSEnabled;
};

iOSModuleBuilder.prototype.run = function run(logger, config, cli, finished) {
Builder.prototype.run.apply(this, arguments);

Expand All @@ -107,7 +138,9 @@ iOSModuleBuilder.prototype.run = function run(logger, config, cli, finished) {
'compileJS',
'buildModule',
'createUniversalBinary',
'verifyBuildArch',
function (next) {
this.verifyBuildArch().then(next, next);
Copy link
Contributor

Choose a reason for hiding this comment

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

  • ⚠️ iphone/cli/commands/_buildModule.js line 142 – Avoid calling back inside of a promise. (promise/no-callback-in-promise)
  • ⚠️ iphone/cli/commands/_buildModule.js line 142 – Expected catch() or return (promise/catch-or-return)

},
'packageModule',
'runModule',

Expand Down Expand Up @@ -447,6 +480,9 @@ iOSModuleBuilder.prototype.buildModule = function buildModule(next) {
this.logger.error('[' + type + '] ' + line);
}, this);
this.logger.log();
if (type === 'xcode-macos') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like this should be handled by the caller (code at bottom of the file that launches these xcodebuilds). I'm fine with the change here since that code would get messy if we just bubbled up the exit code and had to do error handling there as it stands. But I think long-term we should move towards async/await in place of callbacks and do the handling there (so just bubble up the code as return value here, don't call process.exit here, etc and then it's a matter of try/catch down there)

this.logger.info(__('To exclude mac target, set/add mac: false in manifest file.'));
}
process.exit(1);
}

Expand Down Expand Up @@ -476,10 +512,43 @@ iOSModuleBuilder.prototype.buildModule = function buildModule(next) {
args.push('SUPPORTS_MACCATALYST=YES');
}

if (target === 'iphonesimulator') {
// Search for third party framewrok included in module. If found any, exclude arm64 from simulator build.
// Assumption is that simulator arm64 architecture can only be supported via .xcframework.
const frameworksPath = path.join(this.projectDir, 'platform');
const legacyFrameworks = new Set();

fs.readdirSync(frameworksPath).forEach(filename => {
if (filename.endsWith('.framework')) {
legacyFrameworks.add(filename);
}
});
if (legacyFrameworks.size > 0) {
const pbxFilePath = path.join(this.projectDir, `${this.moduleName}.xcodeproj`, 'project.pbxproj');
const proj = xcode.project(pbxFilePath).parseSync();
const configurations = proj.hash.project.objects.XCBuildConfiguration;
let excludeArchs = 'EXCLUDED_ARCHS=arm64 ';

// Merge with excluded archs in xcode setting
for (const key of Object.keys(proj.hash.project.objects.XCBuildConfiguration)) {
const configuration = configurations[key];
if (typeof configuration === 'object' && configuration.name === 'Release' && configuration.buildSettings.EXCLUDED_ARCHS) {
let archs = configuration.buildSettings.EXCLUDED_ARCHS; // e.g. "i386 arm64 x86_64"
archs = archs.replace(/["]/g, '');
excludeArchs = excludeArchs.concat(archs);
break;
}
}

args.push(excludeArchs);
this.logger.warn(__(`The module is using frameworks (${Array.from(legacyFrameworks)}) that do not support simulator arm64. Excluding simulator arm64. The app using this module may fail if you're on an arm64 Apple Silicon device.`));
}
}

return args;
}.bind(this);

this.cli.env.getOSInfo(function (osInfo) {
this.cli.env.getOSInfo(osInfo => {
const macOsVersion = osInfo.osver;

// 1. Create a build for the simulator
Expand All @@ -488,8 +557,13 @@ iOSModuleBuilder.prototype.buildModule = function buildModule(next) {
xcodebuildHook(xcBuild, xcodeBuildArgumentsForTarget('iphoneos'), opts, 'xcode-dist', () => {
const osVersionParts = macOsVersion.split('.').map(n => parseInt(n));
if (osVersionParts[0] > 10 || (osVersionParts[0] === 10 && osVersionParts[1] >= 15)) {
// 3. Create a build for the maccatalyst
xcodebuildHook(xcBuild, xcodeBuildArgumentsForTarget('macosx'), opts, 'xcode-macos', next);
// 3. Create a build for the mac-catalyst if enabled
if (this.isMacOSEnabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Kind of a dumb edge-case, but if they have no indication of Mac support in either a positive or negative value, what should we do? I think this will assume no macOS support, so will not try to build for it.

But if they haven't set SUPPORTS_MACCATALYST in Xcode and they have no mac key/value pair in the manifest, then technically they haven't indicated one way or the other. Would we try to build for Mac and just be ok with it failing in that case?

xcodebuildHook(xcBuild, xcodeBuildArgumentsForTarget('macosx'), opts, 'xcode-macos', next);
} else {
this.logger.info(__('macOS support disabled in Xcode project. Skipping …'));
next();
}
} else {
this.logger.warn(__('Ignoring build for mac as mac target is < 10.15'));
next();
Expand Down Expand Up @@ -558,15 +632,17 @@ iOSModuleBuilder.prototype.createUniversalBinary = function createUniversalBinar
args.push('-headers');
args.push(headerPath);
}
lib = findLib('macosx');
if (lib instanceof Error) {
this.logger.warn(__('The module is missing 64-bit support of macos. Ignoring mac target for this module...'));
} else {
args.push(buildType);
args.push(lib);
if (!this.isFramework) {
args.push('-headers');
args.push(headerPath);
if (this.isMacOSEnabled) {
lib = findLib('macosx');
if (lib instanceof Error) {
this.logger.warn(__('The module is missing macOS support. Ignoring mac target for this module...'));
} else {
args.push(buildType);
args.push(lib);
if (!this.isFramework) {
args.push('-headers');
args.push(headerPath);
}
}
}

Expand All @@ -592,84 +668,89 @@ iOSModuleBuilder.prototype.createUniversalBinary = function createUniversalBinar
}.bind(this));
};

iOSModuleBuilder.prototype.verifyBuildArch = function verifyBuildArch(next) {
iOSModuleBuilder.prototype.verifyBuildArch = async function verifyBuildArch() {
this.logger.info(__('Verifying universal library'));

// TODO: We want to verify the set of targets/arches built
// The manifest shouldn't matter any more?
// Can we just ensure it's an xcframework?

const findLib = function (dest) {
const moduleId = this.isFramework ? this.moduleIdAsIdentifier : this.moduleId;
const libName = this.isFramework ? this.moduleIdAsIdentifier + '.framework' : 'lib' + this.moduleId + '.a';
const lib = path.join(this.projectDir, 'build', moduleId + '.xcframework', dest, libName);
this.logger.info(__('Looking for ' + lib));
const moduleId = this.isFramework ? this.moduleIdAsIdentifier : this.moduleId;
const frameworkPath = path.join(this.projectDir, 'build', moduleId + '.xcframework');
// Make sure xcframework exists...
if (!(await fs.pathExists(frameworkPath))) {
throw new Error(__(`Unable to find "${moduleId}.xcframework"`));
}

if (!fs.existsSync(lib)) {
return new Error(__('Unable to find the built %s library', 'Release-' + dest));
// Confirm that the library/binary the Info.plist is pointing at actually exists on disk
const verifyLibraryExists = async (libInfo) => {
let libraryPath = path.join(frameworkPath, libInfo.LibraryIdentifier, libInfo.LibraryPath);
if (libraryPath.endsWith('.framework')) {
const frameworkName = path.basename(libraryPath, '.framework');
libraryPath = path.join(libraryPath, frameworkName);
}
return lib;
}.bind(this);
if (!(await fs.pathExists(libraryPath))) {
throw new Error(__('Unable to find the built library %s', libraryPath));
}
};

// Verify the architectures and platform variants
const buildArchs = new Set();
// TODO: Just read the Info.plist and take it's listing!
let lib = findLib('ios-arm64_armv7');
if (lib instanceof Error) {
// fallback to xcode 11 xcframework
lib = findLib('ios-armv7_arm64');
if (lib instanceof Error) {
this.logger.warn(__('The module is missing 64-bit support.'));
} else {
buildArchs.add('armv7');
buildArchs.add('arm64');
}
} else {
buildArchs.add('armv7');
buildArchs.add('arm64');

const xcFrameworkInfo = await parsePlist(path.join(frameworkPath, 'Info.plist'));

// Check iOS device
const iosDevice = xcFrameworkInfo.AvailableLibraries.find(l => l.SupportedPlatform === 'ios' && !l.SupportedPlatformVariant);
if (!iosDevice) {
throw new Error('The module is missing iOS device support.');
}
await verifyLibraryExists(iosDevice);
if (!iosDevice.SupportedArchitectures.includes('arm64')) {
this.logger.warn(__('The module is missing iOS device 64-bit support.'));
}
iosDevice.SupportedArchitectures.forEach(arch => buildArchs.add(arch));

lib = findLib('ios-arm64_i386_x86_64-simulator');
if (lib instanceof Error) {
// fall back to xcode 11 xcframework w/o arm64 sim support
lib = findLib('ios-i386_x86_64-simulator');
if (lib instanceof Error) {
// neither is available, so no sim support!
this.logger.warn(__('The module is missing ios simulator support.'));
} else {
this.logger.warn(__('The module is missing arm64 ios simulator support.'));
buildArchs.add('i386');
buildArchs.add('x86_64');
}
// Check iOS Simulator
const iosSim = xcFrameworkInfo.AvailableLibraries.find(l => l.SupportedPlatformVariant === 'simulator');
if (!iosSim) {
throw new Error(__('The module is missing iOS simulator support.'));
}
await verifyLibraryExists(iosSim);
if (!iosSim.SupportedArchitectures.includes('arm64')) {
this.logger.warn(__('The module is missing arm64 iOS simulator support.'));
}
iosSim.SupportedArchitectures.forEach(arch => buildArchs.add(arch));

// MacOS Catalyst support is optional
const macos = xcFrameworkInfo.AvailableLibraries.find(l => l.SupportedPlatformVariant === 'maccatalyst');
if (!macos) {
this.logger.warn(__('The module is missing macOS support.'));
} else {
buildArchs.add('i386');
buildArchs.add('x86_64');
// buildArchs.add('arm64'); // Don't include as we traditionally meant 'arm64' to be ios device arm64 (not sim!)
await verifyLibraryExists(macos);
if (!macos.SupportedArchitectures.includes('arm64')) {
this.logger.warn(__('The module is missing arm64 macOS support. This will not work on Apple Silicon devices (and is likley due to use of an Xcode that does not support arm64 maccatalyst).'));
}
}

lib = findLib('ios-arm64_x86_64-maccatalyst');
if (lib instanceof Error) {
// fall back to Xcode 12 GM variant (betas include arm64, but xcode 12 final does not)
lib = findLib('ios-x86_64-maccatalyst');
if (lib instanceof Error) {
this.logger.warn(__('The module is missing maccatalyst support.'));
// Spit out the platform variants and their architectures
for (const libInfo of xcFrameworkInfo.AvailableLibraries) {
let target;
if (libInfo.SupportedPlatformVariant === 'maccatalyst') {
target = 'Mac';
} else if (libInfo.SupportedPlatformVariant === 'simulator') {
target = 'Simulator';
} else {
this.logger.warn(__('The module is missing arm64 maccatalyst support. This will not work on Apple Silicon devices (and is likley due to use of an Xcode that does not support arm64 maccatalyst).'));
target = 'Device';
}
this.logger.info(__(`${target} has architectures: ${libInfo.SupportedArchitectures}`));
}

// Match against manifest
// TODO: Drop architectures from manifest altogether now?
const manifestArchs = new Set(this.manifest.architectures.split(' '));
if (buildArchs.size !== manifestArchs.size) {
this.logger.error(__('There is discrepancy between the architectures specified in module manifest and compiled binary.'));
this.logger.error(__('Architectures in manifest: %s', Array.from(manifestArchs)));
this.logger.error(__('Compiled binary architectures: %s', Array.from(buildArchs)));
this.logger.error(__('Please update manifest to match module binary architectures.') + '\n');
process.exit(1);
process.exit(1); // TODO: Just throw an Error!
}

if (!buildArchs.has('arm64')) {
this.logger.warn(__('The module is missing 64-bit support.'));
}
next();
};

iOSModuleBuilder.prototype.packageModule = function packageModule(next) {
Expand Down Expand Up @@ -894,7 +975,7 @@ iOSModuleBuilder.prototype.runModule = function runModule(next) {

// 5. unzip module to the tmp dir. Use native binary on macOS, as AdmZip doesn't support symlinks used in mac catalyst frameworks
const proc = spawn('unzip', [ '-o', this.moduleZipPath, '-d', tmpProjectDir ]);
proc.stdout.on('data', data => this.logger.info(data.toString().trimEnd()));
Copy link
Contributor

Choose a reason for hiding this comment

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

without these lines, the unzip never finished for me. We have to "consume" the stdout/stderr for it to finish.

proc.stdout.on('data', data => this.logger.trace(data.toString().trimEnd()));
proc.stderr.on('data', data => this.logger.error(data.toString().trimEnd()));
proc.once('error', err => cb(err));
proc.on('exit', () => cb());
Expand Down
1 change: 1 addition & 0 deletions iphone/templates/module/default/template/ios/manifest.ejs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
version: 1.0.0
apiversion: 2
architectures: armv7 arm64 i386 x86_64
mac: true
description: <%- moduleName %>
author: <%- author %>
license: Specify your license
Expand Down