Address review: follow #include chains, exact include line, default flavor, embed script input, safe deinit

Author: chrfalch

packages/react-native/scripts/spm/__tests__/generate-spm-xcodeproj-test.js

@@ -627,6 +627,7 @@ OTHER_LDFLAGS = $(inherited) -ObjC "$(RN_SPM_REACT_BINARY)"
 FRAMEWORK_SEARCH_PATHS = $(inherited) "$(RN_SPM_REACT_SEARCH_PATH)"
 LD_RUNPATH_SEARCH_PATHS = $(inherited) @executable_path/Frameworks
 
+RN_SPM_FLAVOR = release
 RN_SPM_FLAVOR[config=Debug] = debug
 SWIFT_ACTIVE_COMPILATION_CONDITIONS[config=Debug] = $(inherited) DEBUG
 RN_SPM_FLAVOR[config=Release] = release
@@ -661,6 +662,27 @@ RN_SPM_REACT_SEARCH_PATH[sdk=iphoneos*] = $(SRCROOT)/build/xcframeworks/$(RN_SPM
     expect(xcconfig).not.toMatch(/\[config=(Debug|Release)\]/);
   });
 
+  // A configuration duplicated in Xcode keeps the base reference but has no
+  // line of its own until the next sync, and an unset flavor would resolve
+  // every framework path to nothing.
+  it('falls back to the release flavor for a configuration it does not list', () => {
+    const lines = generateReactNativeXcconfig(
+      ['Debug', 'Release'],
+      '../node_modules/react-native',
+      [FRAMEWORK],
+    ).split('\n');
+    const fallback = lines.indexOf('RN_SPM_FLAVOR = release');
+    expect(fallback).toBeGreaterThan(-1);
+    expect(fallback).toBeLessThan(
+      lines.findIndex(line => line.startsWith('RN_SPM_FLAVOR[config=')),
+    );
+    expect(
+      lines.some(line =>
+        line.startsWith('SWIFT_ACTIVE_COMPILATION_CONDITIONS ='),
+      ),
+    ).toBe(false);
+  });
+
   it('writes no plist quoting around conditional keys or path values', () => {
     const xcconfig = generateReactNativeXcconfig(
       ['Debug'],

packages/react-native/scripts/spm/__tests__/inject-spm-xcodeproj-test.js

@@ -20,6 +20,7 @@ const {
 const {findField, findObjectByUuid, quoteIfNeeded} = require('../spm-pbxproj');
 const {isBalanced} = require('./pbxproj-oracles');
 const fs = require('node:fs');
+const os = require('node:os');
 const path = require('node:path');
 
 const PLAIN = fs.readFileSync(
@@ -270,6 +271,171 @@ describe('planInjection', () => {
   });
 });
 
+// FOREIGN_XCCONFIG with the reference inside a `Config` group, the way Xcode
+// files an xcconfig dragged into a folder of the navigator.
+const FOREIGN_XCCONFIG_IN_GROUP = FOREIGN_XCCONFIG.replace(
+  'path = Config/App.xcconfig;',
+  'path = App.xcconfig;',
+)
+  .replace(
+    '\t\t\t\tAA00000000000000000000F1 /* Products */,\n',
+    '\t\t\t\tAA00000000000000000000F1 /* Products */,\n\t\t\t\tCC0000000000000000000002 /* Config */,\n',
+  )
+  .replace(
+    '/* End PBXGroup section */',
+    '\t\tCC0000000000000000000002 /* Config */ = {\n\t\t\tisa = PBXGroup;\n\t\t\tchildren = (\n\t\t\t\tCC0000000000000000000001 /* App.xcconfig */,\n\t\t\t);\n\t\t\tpath = Config;\n\t\t\tsourceTree = "<group>";\n\t\t};\n/* End PBXGroup section */',
+  );
+
+describe("planInjection — a configuration based on the user's own xcconfig", () => {
+  let roots = [];
+  afterEach(() => {
+    for (const root of roots) {
+      fs.rmSync(root, {recursive: true, force: true});
+    }
+    roots = [];
+  });
+
+  // An app root (== SRCROOT) holding `files`, app-root-relative path → content.
+  function appRootWith(files) {
+    const root = fs.mkdtempSync(path.join(os.tmpdir(), 'spm-include-'));
+    roots.push(root);
+    for (const [relPath, content] of Object.entries(files)) {
+      fs.mkdirSync(path.dirname(path.join(root, relPath)), {recursive: true});
+      fs.writeFileSync(path.join(root, relPath), content, 'utf8');
+    }
+    return root;
+  }
+
+  function plan(files, text = FOREIGN_XCCONFIG) {
+    const root = appRootWith(files);
+    return planInjection(text, {appRoot: root, srcRoot: root});
+  }
+
+  const INCLUDES_GENERATED = `#include "../${XCCONFIG_PATH}"\n`;
+
+  it('accepts it when the xcconfig #includes the generated one', () => {
+    const result = plan({'Config/App.xcconfig': INCLUDES_GENERATED});
+    expect(result.ok).toBe(true);
+    expect(result.includedByConfigUuids).toEqual([APP_DEBUG_CONFIG]);
+  });
+
+  it('includes no configuration it bases on the generated xcconfig itself', () => {
+    expect(planInjection(PLAIN, {}).includedByConfigUuids).toEqual([]);
+  });
+
+  it('finds an xcconfig filed under a navigator group', () => {
+    expect(
+      plan(
+        {'Config/App.xcconfig': INCLUDES_GENERATED},
+        FOREIGN_XCCONFIG_IN_GROUP,
+      ).ok,
+    ).toBe(true);
+  });
+
+  it('follows #include? chains, each path relative to the file that holds it', () => {
+    expect(
+      plan({
+        'Config/App.xcconfig':
+          '// Shared settings\n#include? "Shared/Base.xcconfig"\n',
+        'Config/Shared/Base.xcconfig': `#include "../../${XCCONFIG_PATH}"\n`,
+      }).ok,
+    ).toBe(true);
+  });
+
+  it.each([
+    [
+      'a path relative to the project directory',
+      `#include "${XCCONFIG_PATH}"\n`,
+    ],
+    ['a commented-out include', `// ${INCLUDES_GENERATED}`],
+    ['an include of a file that is missing', '#include? "Missing.xcconfig"\n'],
+  ])('refuses %s', (_label, content) => {
+    expect(plan({'Config/App.xcconfig': content}).ok).toBe(false);
+  });
+
+  it('refuses an include cycle that never reaches the generated xcconfig', () => {
+    const result = plan({
+      'Config/App.xcconfig': '#include "Base.xcconfig"\n',
+      'Config/Base.xcconfig': '#include "App.xcconfig"\n',
+    });
+    expect(result.ok).toBe(false);
+  });
+
+  it('still refuses a list setting without $(inherited) on it', () => {
+    const result = plan(
+      {'Config/App.xcconfig': INCLUDES_GENERATED},
+      FOREIGN_XCCONFIG.replaceAll(
+        'PRODUCT_BUNDLE_IDENTIFIER = com.example.MyApp;',
+        'HEADER_SEARCH_PATHS = /vendor;\n\t\t\t\tPRODUCT_BUNDLE_IDENTIFIER = com.example.MyApp;',
+      ),
+    );
+    expect(result.ok).toBe(false);
+    expect(result.reason).toContain('HEADER_SEARCH_PATHS (Debug, Release)');
+  });
+
+  it('names the exact line to add, relative to the xcconfig that needs it', () => {
+    const result = plan({'Config/App.xcconfig': 'MY_SETTING = 1\n'});
+    expect(result.ok).toBe(false);
+    expect(result.reason).toContain(
+      `#include "../${XCCONFIG_PATH}" to Config/App.xcconfig`,
+    );
+    expect(result.reason).not.toContain('project directory');
+  });
+
+  it('says the path is relative to the xcconfig when it cannot find the file', () => {
+    const result = plan({});
+    expect(result.ok).toBe(false);
+    expect(result.reason).toContain(
+      `#include ${XCCONFIG_PATH} from Config/App.xcconfig`,
+    );
+    expect(result.reason).toContain(
+      'relative to the folder that holds Config/App.xcconfig',
+    );
+    expect(result.reason).not.toContain('project directory');
+  });
+
+  it("keeps the user's base and records the configuration as including the generated xcconfig", () => {
+    const root = appRootWith({'Config/App.xcconfig': INCLUDES_GENERATED});
+    const accepted = planInjection(FOREIGN_XCCONFIG, {
+      appRoot: root,
+      srcRoot: root,
+    });
+    const {text, xcconfig, generatedFiles} = injectSpmIntoPbxproj(
+      FOREIGN_XCCONFIG,
+      {
+        rootUuid: accepted.rootUuid,
+        targetUuid: accepted.target.uuid,
+        configUuids: accepted.configUuids,
+        includedByConfigUuids: accepted.includedByConfigUuids,
+        frameworksPhaseUuid: accepted.frameworksPhaseUuid,
+        sourcesPhaseUuid: accepted.sourcesPhaseUuid,
+      },
+      RN_PATHS,
+      null,
+      [],
+      TEST_FRAMEWORKS,
+    );
+    const baseOf = configUuid =>
+      findField(
+        text,
+        findObjectByUuid(text, configUuid),
+        'baseConfigurationReference',
+      )?.value;
+    expect(baseOf(APP_DEBUG_CONFIG)).toBe(
+      'CC0000000000000000000001 /* App.xcconfig */',
+    );
+    expect(baseOf(APP_RELEASE_CONFIG)).toBe(
+      `${xcconfig.fileRefUuid} /* ReactNativeSPM.xcconfig */`,
+    );
+    expect(xcconfig.configUuids).toEqual([APP_RELEASE_CONFIG]);
+    expect(xcconfig.includedByConfigUuids).toEqual([APP_DEBUG_CONFIG]);
+    expect(text).toContain(`path = ${XCCONFIG_PATH};`);
+    expect(generatedFiles[XCCONFIG_PATH]).toContain(
+      'RN_SPM_FLAVOR[config=Debug] = debug\n',
+    );
+  });
+});
+
 describe('injectSpmIntoPbxproj — Tier 1 (SPM graph)', () => {
   it('adds the local package references and product dependencies', () => {
     const {text} = inject(PLAIN);
@@ -564,6 +730,19 @@ describe('injectSpmIntoPbxproj — Tier 2 (build settings + phase)', () => {
       '$(TARGET_BUILD_DIR)/$(FRAMEWORKS_FOLDER_PATH)/React.framework',
     );
   });
+
+  // The phase only runs the script file, so the file has to be an input for
+  // Xcode to re-run the phase when the file alone changes.
+  it('lists its script file among the embed phase inputs', () => {
+    const {text} = inject(PLAIN);
+    const uuid =
+      /([0-9A-F]{24}) \/\* Embed React Native Flavored Frameworks \*\/ = \{/.exec(
+        text,
+      )[1];
+    expect(
+      findField(text, findObjectByUuid(text, uuid), 'inputPaths').value,
+    ).toContain(`${quoteIfNeeded(`$(SRCROOT)/${EMBED_SCRIPT_PATH}`)},`);
+  });
 });
 
 describe('injectSpmIntoPbxproj — Tier 3 (plugin generated sources)', () => {

packages/react-native/scripts/spm/__tests__/remove-spm-injection-test.js

@@ -186,6 +186,42 @@ function generatedFileContents(appRoot) {
   );
 }
 
+const APP_DEBUG_CONFIG = 'AA0000000000000000000901';
+
+// The app target's Debug configuration based on Config/App.xcconfig, an
+// xcconfig of the user's own.
+const FOREIGN_XCCONFIG = PLAIN.replace(
+  `${APP_DEBUG_CONFIG} /* Debug */ = {\n\t\t\tisa = XCBuildConfiguration;\n`,
+  `${APP_DEBUG_CONFIG} /* Debug */ = {\n\t\t\tisa = XCBuildConfiguration;\n\t\t\tbaseConfigurationReference = CC0000000000000000000001 /* App.xcconfig */;\n`,
+).replace(
+  '/* End PBXFileReference section */',
+  '\t\tCC0000000000000000000001 /* App.xcconfig */ = {isa = PBXFileReference; lastKnownFileType = text.xcconfig; path = Config/App.xcconfig; sourceTree = "<group>"; };\n/* End PBXFileReference section */',
+);
+
+function writeUserXcconfig(appRoot, content) {
+  fs.mkdirSync(path.join(appRoot, 'Config'), {recursive: true});
+  fs.writeFileSync(path.join(appRoot, 'Config', 'App.xcconfig'), content);
+}
+
+// What re-basing Debug on the user's own xcconfig in Xcode does after `add`.
+function rebaseDebugOnUserXcconfig(xcodeprojPath) {
+  const text = pbxprojOf(xcodeprojPath);
+  const start = text.indexOf(`${APP_DEBUG_CONFIG} /* Debug */ = {`);
+  const end = text.indexOf('buildSettings', start);
+  fs.writeFileSync(
+    path.join(xcodeprojPath, 'project.pbxproj'),
+    text.slice(0, start) +
+      text
+        .slice(start, end)
+        .replace(
+          /baseConfigurationReference = [0-9A-F]{24} \/\* ReactNativeSPM\.xcconfig \*\//,
+          'baseConfigurationReference = CC0000000000000000000001 /* App.xcconfig */',
+        ) +
+      text.slice(end),
+    'utf8',
+  );
+}
+
 function schemePathOf(xcodeprojPath) {
   return path.join(
     xcodeprojPath,
@@ -535,6 +571,95 @@ describe('removeSpmInjection — the surgical inverse of add', () => {
     expect(fs.existsSync(path.join(appRoot, XCCONFIG_REL))).toBe(true);
   });
 
+  it('keeps the xcconfig while one configuration is re-based on a user xcconfig, removing the scripts', () => {
+    const {appRoot, xcodeprojPath, rnRoot} = scaffoldApp();
+    injectSpmIntoExistingXcodeproj({
+      appRoot,
+      reactNativeRoot: rnRoot,
+      xcodeprojPath,
+    });
+    rebaseDebugOnUserXcconfig(xcodeprojPath);
+    const logSpy = jest.spyOn(console, 'log').mockImplementation(() => {});
+
+    removeSpmInjection({appRoot, xcodeprojPath});
+
+    const logged = logSpy.mock.calls.map(([line]) => line).join('\n');
+    logSpy.mockRestore();
+    expect(pbxprojOf(xcodeprojPath)).toContain(
+      'baseConfigurationReference = CC0000000000000000000001 /* App.xcconfig */;',
+    );
+    expect(pbxprojOf(xcodeprojPath)).not.toContain('ReactNativeSPM.xcconfig');
+    expect(
+      generatedFileContents(appRoot).map(content => content != null),
+    ).toEqual([true, false, false]);
+    expect(logged).toContain(`Kept ${XCCONFIG_REL}`);
+  });
+
+  it('accepts a user xcconfig that #includes the generated one, across syncs and deinit', () => {
+    const {appRoot, xcodeprojPath, rnRoot} = scaffoldApp(FOREIGN_XCCONFIG);
+    writeUserXcconfig(appRoot, `#include "../${XCCONFIG_REL}"\n`);
+    const sync = () =>
+      injectSpmIntoExistingXcodeproj({
+        appRoot,
+        reactNativeRoot: rnRoot,
+        xcodeprojPath,
+      });
+
+    expect(sync().status).toBe('injected');
+    const injected = pbxprojOf(xcodeprojPath);
+    expect(readMarker(xcodeprojPath).xcconfig.includedByConfigUuids).toEqual([
+      APP_DEBUG_CONFIG,
+    ]);
+    expect(sync().status).toBe('injected');
+    expect(pbxprojOf(xcodeprojPath)).toBe(injected);
+
+    removeSpmInjection({appRoot, xcodeprojPath});
+    expect(pbxprojOf(xcodeprojPath)).toBe(FOREIGN_XCCONFIG);
+    expect(fs.existsSync(path.join(appRoot, XCCONFIG_REL))).toBe(true);
+  });
+
+  it('keeps the xcconfig a recorded configuration included, even once its base is gone', () => {
+    const {appRoot, xcodeprojPath, rnRoot} = scaffoldApp(FOREIGN_XCCONFIG);
+    writeUserXcconfig(appRoot, `#include "../${XCCONFIG_REL}"\n`);
+    injectSpmIntoExistingXcodeproj({
+      appRoot,
+      reactNativeRoot: rnRoot,
+      xcodeprojPath,
+    });
+    fs.writeFileSync(
+      path.join(xcodeprojPath, 'project.pbxproj'),
+      pbxprojOf(xcodeprojPath).replace(
+        '\t\t\tbaseConfigurationReference = CC0000000000000000000001 /* App.xcconfig */;\n',
+        '',
+      ),
+      'utf8',
+    );
+
+    removeSpmInjection({appRoot, xcodeprojPath});
+    expect(fs.existsSync(path.join(appRoot, XCCONFIG_REL))).toBe(true);
+  });
+
+  it('deletes the xcconfig for a marker written before includes were recorded', () => {
+    const {appRoot, xcodeprojPath, rnRoot} = scaffoldApp();
+    injectSpmIntoExistingXcodeproj({
+      appRoot,
+      reactNativeRoot: rnRoot,
+      xcodeprojPath,
+    });
+    const marker = readMarker(xcodeprojPath);
+    delete marker.xcconfig.includedByConfigUuids;
+    fs.writeFileSync(
+      path.join(xcodeprojPath, SPM_INJECTED_MARKER),
+      JSON.stringify(marker, null, 2) + '\n',
+    );
+
+    removeSpmInjection({appRoot, xcodeprojPath});
+    expect(pbxprojOf(xcodeprojPath)).toBe(PLAIN);
+    expect(generatedFileContents(appRoot)).toEqual(
+      GENERATED_FILES.map(() => null),
+    );
+  });
+
   // A ${PODS_ROOT}-anchored value resolves empty without CocoaPods, so putting
   // it back would only restore a broken setting.
   it('does not restore the ${PODS_ROOT}-anchored REACT_NATIVE_PATH it removed', () => {