diff --git a/lib/services/ios-project-service.ts b/lib/services/ios-project-service.ts index 4950bb9637..1c536a0a62 100644 --- a/lib/services/ios-project-service.ts +++ b/lib/services/ios-project-service.ts @@ -1834,6 +1834,23 @@ export class IOSProjectService this.$fs.deleteFile(pluginsXcconfigFilePath); } + // mergeFiles keeps whichever value is already present, so the app's + // xcconfig is merged before any plugin's to make it authoritative: a + // plugin must not be able to dictate a setting the app has chosen. + const appResourcesXcconfigPath = path.join( + projectData.appResourcesDirectoryPath, + this.getPlatformData(projectData).normalizedPlatformName, + BUILD_XCCONFIG_FILE_NAME, + ); + if (this.$fs.exists(appResourcesXcconfigPath)) { + for (const pluginsXcconfigFilePath of pluginsXcconfigFilePaths) { + await this.$xcconfigService.mergeFiles( + appResourcesXcconfigPath, + pluginsXcconfigFilePath, + ); + } + } + const allPlugins: IPluginData[] = this.getAllProductionPlugins(projectData); for (const plugin of allPlugins) { const pluginPlatformsFolderPath = plugin.pluginPlatformsFolderPath( @@ -1853,20 +1870,6 @@ export class IOSProjectService } } - const appResourcesXcconfigPath = path.join( - projectData.appResourcesDirectoryPath, - this.getPlatformData(projectData).normalizedPlatformName, - BUILD_XCCONFIG_FILE_NAME, - ); - if (this.$fs.exists(appResourcesXcconfigPath)) { - for (const pluginsXcconfigFilePath of pluginsXcconfigFilePaths) { - await this.$xcconfigService.mergeFiles( - appResourcesXcconfigPath, - pluginsXcconfigFilePath, - ); - } - } - for (const pluginsXcconfigFilePath of pluginsXcconfigFilePaths) { if (!this.$fs.exists(pluginsXcconfigFilePath)) { // We need the pluginsXcconfig file to exist in platforms dir as it is required in the native template: diff --git a/lib/services/xcconfig-service.ts b/lib/services/xcconfig-service.ts index 1b99ffa962..a85e34851e 100644 --- a/lib/services/xcconfig-service.ts +++ b/lib/services/xcconfig-service.ts @@ -10,16 +10,20 @@ import * as _ from "lodash"; import { injector } from "../common/yok"; export class XcconfigService implements IXcconfigService { - constructor(private $childProcess: IChildProcess, private $fs: IFileSystem) {} + private static readonly CONFLICT_MARKER = "NS_XCCONFIG_CONFLICTS:"; + + constructor( + private $childProcess: IChildProcess, + private $fs: IFileSystem, + private $logger: ILogger, + ) {} public getPluginsXcconfigFilePaths(projectRoot: string): IStringDictionary { return { - [Configurations.Debug.toLowerCase()]: this.getPluginsDebugXcconfigFilePath( - projectRoot - ), - [Configurations.Release.toLowerCase()]: this.getPluginsReleaseXcconfigFilePath( - projectRoot - ), + [Configurations.Debug.toLowerCase()]: + this.getPluginsDebugXcconfigFilePath(projectRoot), + [Configurations.Release.toLowerCase()]: + this.getPluginsReleaseXcconfigFilePath(projectRoot), }; } @@ -33,28 +37,79 @@ export class XcconfigService implements IXcconfigService { public async mergeFiles( sourceFile: string, - destinationFile: string + destinationFile: string, ): Promise { if (!this.$fs.exists(destinationFile)) { this.$fs.writeFile(destinationFile, ""); } - const escapedDestinationFile = destinationFile.replace(/'/g, "\\'"); - const escapedSourceFile = sourceFile.replace(/'/g, "\\'"); - - const mergeScript = `require 'xcodeproj'; - userConfig = Xcodeproj::Config.new('${escapedDestinationFile}') - existingConfig = Xcodeproj::Config.new('${escapedSourceFile}') - userConfig.attributes.each do |key,| - existingConfig.attributes.delete(key) if (userConfig.attributes.key?(key) && existingConfig.attributes.key?(key)) + // A key already present in the destination wins, so the incoming one is + // dropped. Report the drops whose values actually differ: a silently + // discarded setting is otherwise indistinguishable from one that was + // never written, which makes a plugin pinning e.g. + // CLANG_CXX_LANGUAGE_STANDARD very hard to track down. + // + // The paths are passed as argv rather than interpolated: they come from + // the project and node_modules layout, and a shell-interpolated command + // would execute anything a directory name expands to. + const mergeScript = `require 'xcodeproj' + require 'json' + destination, source = ARGV + userConfig = Xcodeproj::Config.new(destination) + existingConfig = Xcodeproj::Config.new(source) + conflicts = [] + userConfig.attributes.each do |key, kept| + if existingConfig.attributes.key?(key) + ignored = existingConfig.attributes[key] + conflicts << { 'key' => key, 'kept' => kept.to_s, 'ignored' => ignored.to_s } if ignored.to_s != kept.to_s + existingConfig.attributes.delete(key) + end end - userConfig.merge(existingConfig).save_as(Pathname.new('${escapedDestinationFile}'))`; - await this.$childProcess.exec(`ruby -e "${mergeScript}"`); + userConfig.merge(existingConfig).save_as(Pathname.new(destination)) + print '${XcconfigService.CONFLICT_MARKER}' + JSON.generate(conflicts)`; + const output = await this.$childProcess.execFile("ruby", [ + "-e", + mergeScript, + destinationFile, + sourceFile, + ]); + this.warnAboutConflicts(sourceFile, output); + } + + private warnAboutConflicts(sourceFile: string, output: any): void { + const text: string = + output === null || output === undefined ? "" : `${output}`; + const markerIndex = text.lastIndexOf(XcconfigService.CONFLICT_MARKER); + if (markerIndex === -1) { + return; + } + + let conflicts: { key: string; kept: string; ignored: string }[]; + try { + conflicts = JSON.parse( + text.substring(markerIndex + XcconfigService.CONFLICT_MARKER.length), + ); + } catch (err) { + // Never let a reporting problem fail the merge itself. + this.$logger.trace( + `Unable to read xcconfig conflicts for ${sourceFile}: ${err}`, + ); + return; + } + + for (const conflict of conflicts || []) { + this.$logger.warn( + `Ignoring ${conflict.key} = ${conflict.ignored} from ${sourceFile}: ` + + `already set to ${conflict.kept} by a higher precedence xcconfig. ` + + `The app's App_Resources xcconfig is applied first, then each ` + + `plugin's in dependency order.`, + ); + } } public readPropertyValue( xcconfigFilePath: string, - propertyName: string + propertyName: string, ): string { if (this.$fs.exists(xcconfigFilePath)) { const text = this.$fs.readText(xcconfigFilePath); diff --git a/test/ios-project-service.ts b/test/ios-project-service.ts index 20decc96d1..5aa80c57cb 100644 --- a/test/ios-project-service.ts +++ b/test/ios-project-service.ts @@ -1201,6 +1201,45 @@ describe("Merge Project XCConfig files", () => { } }); + it("The app's build.xcconfig wins over a plugin's", async () => { + fs.writeFile( + appResourcesXcconfigPath, + `CLANG_CXX_LANGUAGE_STANDARD = c++20${EOL}`, + ); + + const pluginPlatformsFolderPath = join(projectPath, "somePlugin", "ios"); + fs.writeFile( + join(pluginPlatformsFolderPath, BUILD_XCCONFIG_FILE_NAME), + `CLANG_CXX_LANGUAGE_STANDARD = c++17${EOL}GCC_C_LANGUAGE_STANDARD = gnu17${EOL}`, + ); + + const pluginsService = testInjector.resolve("pluginsService"); + pluginsService.getAllProductionPlugins = () => [ + { + name: "somePlugin", + pluginPlatformsFolderPath: () => pluginPlatformsFolderPath, + }, + ]; + + await (iOSProjectService).mergeProjectXcconfigFiles(projectData); + + _.each( + xcconfigService.getPluginsXcconfigFilePaths(projectRoot), + (destinationFilePath) => { + assertPropertyValues( + { + // The app pinned this, so the plugin's c++17 must not win. + CLANG_CXX_LANGUAGE_STANDARD: "c++20", + // Keys the app says nothing about still come from the plugin. + GCC_C_LANGUAGE_STANDARD: "gnu17", + }, + destinationFilePath, + testInjector, + ); + }, + ); + }); + it("Adds the entitlements property if not set by the user", async () => { for (const release in [true, false]) { const realExistsFunction = testInjector.resolve("fs").exists; diff --git a/test/xcconfig-service.ts b/test/xcconfig-service.ts index a43291f73d..20d9c20ce0 100644 --- a/test/xcconfig-service.ts +++ b/test/xcconfig-service.ts @@ -5,6 +5,7 @@ import * as yok from "../lib/common/yok"; import { IXcconfigService } from "../lib/declarations"; import { IInjector } from "../lib/common/definitions/yok"; import { IReadFileOptions } from "../lib/common/declarations"; +import { LoggerStub } from "./stubs"; // start tracking temporary folders/files @@ -18,6 +19,7 @@ describe("XCConfig Service Tests", () => { }); testInjector.register("childProcess", {}); testInjector.register("xcprojService", {}); + testInjector.register("logger", LoggerStub); testInjector.register("xcconfigService", XcconfigService);