Skip to content

Commit e8296ed

Browse files
edusperoniedusperoni
committed
fix(ios): let the app's xcconfig win over a plugin's
XcconfigService.mergeFiles keeps whichever value is already in the destination and drops the incoming one, and mergeProjectXcconfigFiles merged every plugin before the app's App_Resources/iOS/build.xcconfig. So for any key a plugin set, the app's value was discarded outright -- there was no way to override a plugin from the app, and among plugins the winner was whichever came first in dependency resolution order. Merging the app's file first makes it authoritative while leaving plugins to supply everything the app says nothing about.
1 parent 354903e commit e8296ed

4 files changed

Lines changed: 111 additions & 19 deletions

File tree

‎lib/services/ios-project-service.ts‎

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1834,6 +1834,23 @@ export class IOSProjectService
18341834
this.$fs.deleteFile(pluginsXcconfigFilePath);
18351835
}
18361836

1837+
// mergeFiles keeps whichever value is already present, so the app's
1838+
// xcconfig is merged before any plugin's to make it authoritative: a
1839+
// plugin must not be able to dictate a setting the app has chosen.
1840+
const appResourcesXcconfigPath = path.join(
1841+
projectData.appResourcesDirectoryPath,
1842+
this.getPlatformData(projectData).normalizedPlatformName,
1843+
BUILD_XCCONFIG_FILE_NAME,
1844+
);
1845+
if (this.$fs.exists(appResourcesXcconfigPath)) {
1846+
for (const pluginsXcconfigFilePath of pluginsXcconfigFilePaths) {
1847+
await this.$xcconfigService.mergeFiles(
1848+
appResourcesXcconfigPath,
1849+
pluginsXcconfigFilePath,
1850+
);
1851+
}
1852+
}
1853+
18371854
const allPlugins: IPluginData[] = this.getAllProductionPlugins(projectData);
18381855
for (const plugin of allPlugins) {
18391856
const pluginPlatformsFolderPath = plugin.pluginPlatformsFolderPath(
@@ -1853,20 +1870,6 @@ export class IOSProjectService
18531870
}
18541871
}
18551872

1856-
const appResourcesXcconfigPath = path.join(
1857-
projectData.appResourcesDirectoryPath,
1858-
this.getPlatformData(projectData).normalizedPlatformName,
1859-
BUILD_XCCONFIG_FILE_NAME,
1860-
);
1861-
if (this.$fs.exists(appResourcesXcconfigPath)) {
1862-
for (const pluginsXcconfigFilePath of pluginsXcconfigFilePaths) {
1863-
await this.$xcconfigService.mergeFiles(
1864-
appResourcesXcconfigPath,
1865-
pluginsXcconfigFilePath,
1866-
);
1867-
}
1868-
}
1869-
18701873
for (const pluginsXcconfigFilePath of pluginsXcconfigFilePaths) {
18711874
if (!this.$fs.exists(pluginsXcconfigFilePath)) {
18721875
// We need the pluginsXcconfig file to exist in platforms dir as it is required in the native template:

‎lib/services/xcconfig-service.ts‎

Lines changed: 53 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,13 @@ import * as _ from "lodash";
1010
import { injector } from "../common/yok";
1111

1212
export class XcconfigService implements IXcconfigService {
13-
constructor(private $childProcess: IChildProcess, private $fs: IFileSystem) {}
13+
private static readonly CONFLICT_MARKER = "NS_XCCONFIG_CONFLICTS:";
14+
15+
constructor(
16+
private $childProcess: IChildProcess,
17+
private $fs: IFileSystem,
18+
private $logger: ILogger
19+
) {}
1420

1521
public getPluginsXcconfigFilePaths(projectRoot: string): IStringDictionary {
1622
return {
@@ -42,14 +48,56 @@ export class XcconfigService implements IXcconfigService {
4248
const escapedDestinationFile = destinationFile.replace(/'/g, "\\'");
4349
const escapedSourceFile = sourceFile.replace(/'/g, "\\'");
4450

51+
// A key already present in the destination wins, so the incoming one is
52+
// dropped. Report the drops whose values actually differ: a silently
53+
// discarded setting is otherwise indistinguishable from one that was
54+
// never written, which makes a plugin pinning e.g.
55+
// CLANG_CXX_LANGUAGE_STANDARD very hard to track down.
4556
const mergeScript = `require 'xcodeproj';
57+
require 'json';
4658
userConfig = Xcodeproj::Config.new('${escapedDestinationFile}')
4759
existingConfig = Xcodeproj::Config.new('${escapedSourceFile}')
48-
userConfig.attributes.each do |key,|
49-
existingConfig.attributes.delete(key) if (userConfig.attributes.key?(key) && existingConfig.attributes.key?(key))
60+
conflicts = []
61+
userConfig.attributes.each do |key, kept|
62+
if existingConfig.attributes.key?(key)
63+
ignored = existingConfig.attributes[key]
64+
conflicts << { 'key' => key, 'kept' => kept.to_s, 'ignored' => ignored.to_s } if ignored.to_s != kept.to_s
65+
existingConfig.attributes.delete(key)
66+
end
5067
end
51-
userConfig.merge(existingConfig).save_as(Pathname.new('${escapedDestinationFile}'))`;
52-
await this.$childProcess.exec(`ruby -e "${mergeScript}"`);
68+
userConfig.merge(existingConfig).save_as(Pathname.new('${escapedDestinationFile}'))
69+
print '${XcconfigService.CONFLICT_MARKER}' + JSON.generate(conflicts)`;
70+
const output = await this.$childProcess.exec(`ruby -e "${mergeScript}"`);
71+
this.warnAboutConflicts(sourceFile, output);
72+
}
73+
74+
private warnAboutConflicts(sourceFile: string, output: any): void {
75+
const text: string = output === null || output === undefined ? "" : `${output}`;
76+
const markerIndex = text.lastIndexOf(XcconfigService.CONFLICT_MARKER);
77+
if (markerIndex === -1) {
78+
return;
79+
}
80+
81+
let conflicts: { key: string; kept: string; ignored: string }[];
82+
try {
83+
conflicts = JSON.parse(
84+
text.substring(markerIndex + XcconfigService.CONFLICT_MARKER.length)
85+
);
86+
} catch (err) {
87+
// Never let a reporting problem fail the merge itself.
88+
this.$logger.trace(
89+
`Unable to read xcconfig conflicts for ${sourceFile}: ${err}`
90+
);
91+
return;
92+
}
93+
94+
for (const conflict of conflicts || []) {
95+
this.$logger.warn(
96+
`Ignoring ${conflict.key} = ${conflict.ignored} from ${sourceFile}: ` +
97+
`already set to ${conflict.kept} by a higher precedence xcconfig. ` +
98+
`The app's App_Resources xcconfig wins over any plugin's.`
99+
);
100+
}
53101
}
54102

55103
public readPropertyValue(

‎test/ios-project-service.ts‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1201,6 +1201,45 @@ describe("Merge Project XCConfig files", () => {
12011201
}
12021202
});
12031203

1204+
it("The app's build.xcconfig wins over a plugin's", async () => {
1205+
fs.writeFile(
1206+
appResourcesXcconfigPath,
1207+
`CLANG_CXX_LANGUAGE_STANDARD = c++20${EOL}`,
1208+
);
1209+
1210+
const pluginPlatformsFolderPath = join(projectPath, "somePlugin", "ios");
1211+
fs.writeFile(
1212+
join(pluginPlatformsFolderPath, BUILD_XCCONFIG_FILE_NAME),
1213+
`CLANG_CXX_LANGUAGE_STANDARD = c++17${EOL}GCC_C_LANGUAGE_STANDARD = gnu17${EOL}`,
1214+
);
1215+
1216+
const pluginsService = testInjector.resolve("pluginsService");
1217+
pluginsService.getAllProductionPlugins = () => [
1218+
{
1219+
name: "somePlugin",
1220+
pluginPlatformsFolderPath: () => pluginPlatformsFolderPath,
1221+
},
1222+
];
1223+
1224+
await (<any>iOSProjectService).mergeProjectXcconfigFiles(projectData);
1225+
1226+
_.each(
1227+
xcconfigService.getPluginsXcconfigFilePaths(projectRoot),
1228+
(destinationFilePath) => {
1229+
assertPropertyValues(
1230+
{
1231+
// The app pinned this, so the plugin's c++17 must not win.
1232+
CLANG_CXX_LANGUAGE_STANDARD: "c++20",
1233+
// Keys the app says nothing about still come from the plugin.
1234+
GCC_C_LANGUAGE_STANDARD: "gnu17",
1235+
},
1236+
destinationFilePath,
1237+
testInjector,
1238+
);
1239+
},
1240+
);
1241+
});
1242+
12041243
it("Adds the entitlements property if not set by the user", async () => {
12051244
for (const release in [true, false]) {
12061245
const realExistsFunction = testInjector.resolve("fs").exists;

‎test/xcconfig-service.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import * as yok from "../lib/common/yok";
55
import { IXcconfigService } from "../lib/declarations";
66
import { IInjector } from "../lib/common/definitions/yok";
77
import { IReadFileOptions } from "../lib/common/declarations";
8+
import { LoggerStub } from "./stubs";
89

910
// start tracking temporary folders/files
1011

@@ -18,6 +19,7 @@ describe("XCConfig Service Tests", () => {
1819
});
1920
testInjector.register("childProcess", {});
2021
testInjector.register("xcprojService", {});
22+
testInjector.register("logger", LoggerStub);
2123

2224
testInjector.register("xcconfigService", XcconfigService);
2325

0 commit comments

Comments
 (0)