Skip to content

Commit 1b5cabd

Browse files
edusperoniedusperoni
authored
fix(ios): let the app's xcconfig win over a plugin's, and warn on discarded settings (#6091)
1 parent 513415a commit 1b5cabd

4 files changed

Lines changed: 132 additions & 33 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: 74 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -10,16 +10,20 @@ 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 {
17-
[Configurations.Debug.toLowerCase()]: this.getPluginsDebugXcconfigFilePath(
18-
projectRoot
19-
),
20-
[Configurations.Release.toLowerCase()]: this.getPluginsReleaseXcconfigFilePath(
21-
projectRoot
22-
),
23+
[Configurations.Debug.toLowerCase()]:
24+
this.getPluginsDebugXcconfigFilePath(projectRoot),
25+
[Configurations.Release.toLowerCase()]:
26+
this.getPluginsReleaseXcconfigFilePath(projectRoot),
2327
};
2428
}
2529

@@ -33,28 +37,79 @@ export class XcconfigService implements IXcconfigService {
3337

3438
public async mergeFiles(
3539
sourceFile: string,
36-
destinationFile: string
40+
destinationFile: string,
3741
): Promise<void> {
3842
if (!this.$fs.exists(destinationFile)) {
3943
this.$fs.writeFile(destinationFile, "");
4044
}
4145

42-
const escapedDestinationFile = destinationFile.replace(/'/g, "\\'");
43-
const escapedSourceFile = sourceFile.replace(/'/g, "\\'");
44-
45-
const mergeScript = `require 'xcodeproj';
46-
userConfig = Xcodeproj::Config.new('${escapedDestinationFile}')
47-
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))
46+
// A key already present in the destination wins, so the incoming one is
47+
// dropped. Report the drops whose values actually differ: a silently
48+
// discarded setting is otherwise indistinguishable from one that was
49+
// never written, which makes a plugin pinning e.g.
50+
// CLANG_CXX_LANGUAGE_STANDARD very hard to track down.
51+
//
52+
// The paths are passed as argv rather than interpolated: they come from
53+
// the project and node_modules layout, and a shell-interpolated command
54+
// would execute anything a directory name expands to.
55+
const mergeScript = `require 'xcodeproj'
56+
require 'json'
57+
destination, source = ARGV
58+
userConfig = Xcodeproj::Config.new(destination)
59+
existingConfig = Xcodeproj::Config.new(source)
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(destination))
69+
print '${XcconfigService.CONFLICT_MARKER}' + JSON.generate(conflicts)`;
70+
const output = await this.$childProcess.execFile("ruby", [
71+
"-e",
72+
mergeScript,
73+
destinationFile,
74+
sourceFile,
75+
]);
76+
this.warnAboutConflicts(sourceFile, output);
77+
}
78+
79+
private warnAboutConflicts(sourceFile: string, output: any): void {
80+
const text: string =
81+
output === null || output === undefined ? "" : `${output}`;
82+
const markerIndex = text.lastIndexOf(XcconfigService.CONFLICT_MARKER);
83+
if (markerIndex === -1) {
84+
return;
85+
}
86+
87+
let conflicts: { key: string; kept: string; ignored: string }[];
88+
try {
89+
conflicts = JSON.parse(
90+
text.substring(markerIndex + XcconfigService.CONFLICT_MARKER.length),
91+
);
92+
} catch (err) {
93+
// Never let a reporting problem fail the merge itself.
94+
this.$logger.trace(
95+
`Unable to read xcconfig conflicts for ${sourceFile}: ${err}`,
96+
);
97+
return;
98+
}
99+
100+
for (const conflict of conflicts || []) {
101+
this.$logger.warn(
102+
`Ignoring ${conflict.key} = ${conflict.ignored} from ${sourceFile}: ` +
103+
`already set to ${conflict.kept} by a higher precedence xcconfig. ` +
104+
`The app's App_Resources xcconfig is applied first, then each ` +
105+
`plugin's in dependency order.`,
106+
);
107+
}
53108
}
54109

55110
public readPropertyValue(
56111
xcconfigFilePath: string,
57-
propertyName: string
112+
propertyName: string,
58113
): string {
59114
if (this.$fs.exists(xcconfigFilePath)) {
60115
const text = this.$fs.readText(xcconfigFilePath);

‎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)