Skip to content

Commit 1d7e661

Browse files
chrfalchclaude
andcommitted
fix(spm): address review — Majors, minors, CodeQL + local-build ergonomics
Group A of the SwiftPM review (code + unit-testable; SPM Release build fixes land separately). Majors: - generate-spm-xcodeproj.js: the generated Sync build phase ran `spm sync` then `RC=$?` under `set -euo pipefail`, so a non-zero exit aborted the phase and the RC branch was unreachable (every transient sync failure broke the build). Now `spm sync || RC=$?` captures the code so the warn-and-exit-0 branch works. - spm-utils.js: buildPerAppHeaderTree built into `outDir + '.tmp'`, which sits INSIDE build/generated/ios — a folded root — so foldDir folded the half-built farm into a spurious `ReactAppHeaders.tmp/` namespace duplicating every codegen header. Build into build/.react-app-headers.tmp (outside every folded root), then rename into place. Test asserts `.tmp/` is absent. - read-podspec.js: pod-ipc JSON extraction anchored on the first `{`/last `}` anywhere, so a pre-JSON diagnostic containing braces (`Building {module}`) shifted the slice and dropped SILENTLY to the weaker regex parser. Anchor on the line-boundary `{`…`}` pod ipc actually emits, and log when the authoritative parse is abandoned. Minors / CodeQL / ergonomics: - spm-pbxproj.js: array build-setting dedup was substring-based (`-ObjC` vs `-ObjCFoo`); now exact-token as the docstring promised. - expand-spm-dependencies.js: swiftName collision detection is case-insensitive (`worklets` vs `Worklets` collide on case-insensitive macOS filesystems). - generate-spm-autolinking.js: the mixed-language friendly diagnostic now also guards user spm.modules / synth wrappers, not just the community-dep path; and the `.podspec` scanners skip a crashed run's `.spm-scaffold-*` leftover. - download-spm-artifacts.js: registry `version` is validated against a safe charset before it flows into Maven URLs / tar filenames (clear error instead of a 404; satisfies the CodeQL command-line data-flow). Added `--deps-tarball` / RN_DEPS_TARBALL_PATH (symmetric with --core-tarball) — a local deps tarball auto-stages its headers sidecar via the existing companion path. - generate-spm-package.js: guard against a stale/wrong React.xcframework layout (missing Headers/React-umbrella.h or a nested Headers/React_Core/) — fail loudly instead of dying deep in the consumer's Clang module build. - scaffold-package-swift.js: correct the `exclude:` comment (none is emitted). CodeQL weak-crypto (spm-pbxproj generateUUID) is already sha256 — those alerts are stale pre-fix and clear on rescan. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 1a228e2 commit 1d7e661

13 files changed

Lines changed: 260 additions & 32 deletions

packages/react-native/scripts/spm/__tests__/download-spm-artifacts-test.js

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -527,6 +527,29 @@ describe('resolveRNDepsArtifact', () => {
527527
expect(result.url).toContain('react-native-artifacts/0.84.2/');
528528
});
529529

530+
it('uses a local deps tarball override when the file exists', async () => {
531+
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'spm-deps-local-'));
532+
try {
533+
const tarball = path.join(dir, 'deps.tar.gz');
534+
fs.writeFileSync(tarball, 'x');
535+
const result = await resolveRNDepsArtifact('0.85.0', 'debug', tarball);
536+
expect(result.url).toBe(tarball);
537+
expect(result.version).toBe('0.85.0-local');
538+
} finally {
539+
fs.rmSync(dir, {recursive: true, force: true});
540+
}
541+
});
542+
543+
it('throws when the local deps tarball override is missing', async () => {
544+
await expect(
545+
resolveRNDepsArtifact(
546+
'0.85.0',
547+
'debug',
548+
path.join(os.tmpdir(), 'spm-deps-nope', 'nope.tar.gz'),
549+
),
550+
).rejects.toThrow(/does not exist/);
551+
});
552+
530553
it('resolves RN_DEP_VERSION=nightly via the npm registry', async () => {
531554
process.env.RN_DEP_VERSION = 'nightly';
532555
globalThis.fetch = routerFetch({

packages/react-native/scripts/spm/__tests__/expand-spm-dependencies-test.js

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -326,6 +326,24 @@ describe('expandSpmDependencies', () => {
326326
).toThrow(/ReactNativeWorklets/);
327327
});
328328

329+
it('throws on a CASE-INSENSITIVE swiftName collision (worklets vs Worklets)', () => {
330+
// Distinct as exact strings, but collide as directories on the default
331+
// case-insensitive macOS filesystem.
332+
const direct = [
333+
{name: 'react-native-worklets', root: '/w', platforms: {ios: {}}},
334+
{name: 'other-worklets', root: '/o', platforms: {ios: {}}},
335+
];
336+
expect(() =>
337+
expandSpmDependencies(direct, {
338+
readConfig: makeReadConfig({
339+
'/w': {spm: {name: 'worklets'}},
340+
'/o': {spm: {name: 'Worklets'}},
341+
}),
342+
resolveDep: makeResolveDep({}),
343+
}),
344+
).toThrow(/case/i);
345+
});
346+
329347
it('rejects empty-string spm.name with a clear error citing the npm name', () => {
330348
const direct = [{name: 'a', root: '/a', platforms: {ios: {}}}];
331349
expect(() =>

packages/react-native/scripts/spm/__tests__/spm-pbxproj-test.js

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,36 @@ describe('addArrayStringValues', () => {
192192
expect(out).toContain('"-lz"');
193193
expect(out).toContain('"-ObjC"');
194194
});
195+
196+
it('dedups by EXACT token, not substring (adds "-ObjC" even when "-ObjCFoo" is present)', () => {
197+
const withArray = PLAIN_PBXPROJ.replace(
198+
'PRODUCT_NAME = "$(TARGET_NAME)";',
199+
'OTHER_LDFLAGS = ("-ObjCFoo", ); PRODUCT_NAME = "$(TARGET_NAME)";',
200+
);
201+
const out = addArrayStringValues(
202+
withArray,
203+
targetDebugDict(withArray),
204+
'OTHER_LDFLAGS',
205+
['"-ObjC"'],
206+
);
207+
// A substring check would have seen "-ObjC" inside "-ObjCFoo" and skipped it.
208+
expect(out).toContain('"-ObjC"');
209+
expect(out).toContain('"-ObjCFoo"');
210+
});
211+
212+
it('does not re-add an exact existing member', () => {
213+
const withArray = PLAIN_PBXPROJ.replace(
214+
'PRODUCT_NAME = "$(TARGET_NAME)";',
215+
'OTHER_LDFLAGS = ("-ObjC", ); PRODUCT_NAME = "$(TARGET_NAME)";',
216+
);
217+
const out = addArrayStringValues(
218+
withArray,
219+
targetDebugDict(withArray),
220+
'OTHER_LDFLAGS',
221+
['"-ObjC"'],
222+
);
223+
expect((out.match(/"-ObjC"/g) || []).length).toBe(1);
224+
});
195225
});
196226

197227
describe('ensureScalarField', () => {

packages/react-native/scripts/spm/__tests__/spm-utils-test.js

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -602,6 +602,20 @@ describe('per-app header farm (ReactAppHeaders SPM target)', () => {
602602
const second = buildPerAppHeaderTree(appRoot);
603603
// No nested ReactAppHeaders/ReactAppHeaders self-fold artifacts.
604604
expect(fs.existsSync(path.join(perAppDir, 'ReactAppHeaders'))).toBe(false);
605+
// ...and no ReactAppHeaders.tmp/ either: the temp build dir must NOT live
606+
// under a folded root (build/generated/ios), else foldDir walks the
607+
// half-built farm and creates a spurious `ReactAppHeaders.tmp/` namespace
608+
// duplicating every codegen header.
609+
expect(fs.existsSync(path.join(perAppDir, 'ReactAppHeaders.tmp'))).toBe(
610+
false,
611+
);
612+
expect(
613+
[...second.virtualPaths].some(p => p.includes('ReactAppHeaders.tmp')),
614+
).toBe(false);
615+
// The out-of-tree temp is consumed (renamed into place), not left behind.
616+
expect(
617+
fs.existsSync(path.join(appRoot, 'build', '.react-app-headers.tmp')),
618+
).toBe(false);
605619
expect(second.virtualPaths.has('MyLib/Provider.h')).toBe(true);
606620
});
607621
});

packages/react-native/scripts/spm/download-spm-artifacts.js

Lines changed: 48 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,11 @@ function parseArgs(argv /*: Array<string> */) /*: DownloadArgs */ {
9696
describe:
9797
'Local ReactNativeHeaders tarball to use instead of downloading. Env fallback: RN_HEADERS_TARBALL_PATH.',
9898
})
99+
.option('deps-tarball', {
100+
type: 'string',
101+
describe:
102+
'Local ReactNativeDependencies tarball to use instead of downloading (e.g. the deps prebuild output). Carries the headers sidecar beside the binary, so this subsumes --deps-headers-tarball for a local build. Env fallback: RN_DEPS_TARBALL_PATH.',
103+
})
99104
.option('deps-headers-tarball', {
100105
type: 'string',
101106
describe:
@@ -115,6 +120,8 @@ function parseArgs(argv /*: Array<string> */) /*: DownloadArgs */ {
115120
parsed['core-tarball'] ?? process.env.RN_CORE_TARBALL_PATH ?? null,
116121
headersTarball:
117122
parsed['headers-tarball'] ?? process.env.RN_HEADERS_TARBALL_PATH ?? null,
123+
depsTarball:
124+
parsed['deps-tarball'] ?? process.env.RN_DEPS_TARBALL_PATH ?? null,
118125
depsHeadersTarball:
119126
parsed['deps-headers-tarball'] ??
120127
process.env.RN_DEPS_HEADERS_TARBALL_PATH ??
@@ -241,11 +248,24 @@ async function resolveNightlyVersion(
241248
if (!res.ok) {
242249
throw new Error(`npm lookup failed for ${npmPackage}: ${res.status}`);
243250
}
244-
const ver = (await res.json()).version;
251+
const ver = (await res.json())?.version;
252+
assertSafeVersion(ver, `${npmPackage}/nightly`);
245253
log(` Resolved nightly: ${ver}`);
246254
return ver;
247255
}
248256

257+
// A version string flows into Maven URLs and local tarball filenames (which are
258+
// then passed to tar/cp via execFileSync). Constrain it to a safe charset so a
259+
// malformed/hostile registry response can't produce a surprising path or a
260+
// confusing 404 — and so static analysis sees an explicit sanitizer.
261+
function assertSafeVersion(ver /*: mixed */, source /*: string */) /*: void */ {
262+
if (typeof ver !== 'string' || !/^[A-Za-z0-9._-]+$/.test(ver)) {
263+
throw new Error(
264+
`npm response for ${source} has no usable "version" field (got: ${String(ver)})`,
265+
);
266+
}
267+
}
268+
249269
/**
250270
* Returns the cache-slot key for a given raw version label.
251271
*
@@ -281,7 +301,8 @@ async function resolveLatestV1Version() /*: Promise<string> */ {
281301
if (!res.ok) {
282302
throw new Error(`npm lookup failed: ${res.status}`);
283303
}
284-
const ver = (await res.json()).version;
304+
const ver = (await res.json())?.version;
305+
assertSafeVersion(ver, 'hermes-compiler/latest-v1');
285306
log(` Resolved latest-v1: ${ver}`);
286307
return ver;
287308
}
@@ -338,7 +359,23 @@ async function resolveRNCoreArtifact(
338359
async function resolveRNDepsArtifact(
339360
rnVersion /*: string */,
340361
flavor /*: string */,
362+
localTarball /*: ?string */,
341363
) /*: Promise<ResolvedArtifact> */ {
364+
// Local-tarball override (--deps-tarball / RN_DEPS_TARBALL_PATH): use a
365+
// locally built deps tarball (the deps prebuild output) instead of
366+
// downloading. It carries ReactNativeDependenciesHeaders.xcframework beside
367+
// the binary, so the existing companion-staging path (COMPANION_XCFRAMEWORKS
368+
// 'rndeps' -> ReactNativeDependenciesHeaders) supplies the sidecar too — no
369+
// separate --deps-headers-tarball needed for a local build.
370+
if (localTarball != null && localTarball !== '') {
371+
if (!fs.existsSync(localTarball)) {
372+
throw new Error(
373+
`deps tarball override is set to ${localTarball} but the file does not exist`,
374+
);
375+
}
376+
log(` Using LOCAL deps tarball: ${localTarball}`);
377+
return {url: localTarball, version: `${rnVersion}-local`};
378+
}
342379
let version = process.env.RN_DEP_VERSION ?? rnVersion;
343380
if (version === 'nightly') {
344381
version = await resolveNightlyVersion('react-native');
@@ -1066,9 +1103,15 @@ async function main(argv /*:: ?: Array<string> */) /*: Promise<void> */ {
10661103
{
10671104
label: 'rndeps',
10681105
name: 'ReactNativeDependencies',
1069-
resolve: () => resolveRNDepsArtifact(resolvedRnVersion, flavor),
1070-
sharedName: (v /*: string */) =>
1071-
`reactnative-dependencies-${v}-${flavor}.tar.gz`,
1106+
resolve: () =>
1107+
resolveRNDepsArtifact(resolvedRnVersion, flavor, args.depsTarball),
1108+
// Local override skips the shared cache (as with core) so a local deps
1109+
// build can't poison the canonical downloads.
1110+
sharedName:
1111+
args.depsTarball != null
1112+
? null
1113+
: (v /*: string */) =>
1114+
`reactnative-dependencies-${v}-${flavor}.tar.gz`,
10721115
},
10731116
{
10741117
label: 'hermes',

packages/react-native/scripts/spm/expand-spm-dependencies.js

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -149,19 +149,29 @@ function expandSpmDependencies(
149149
// package layout and the centralized headers tree. Surface it now with a
150150
// clear message instead of letting SPM emit a confusing duplicate-target
151151
// error later.
152-
const seen /*: Map<string, string> */ = new Map();
152+
// Key case-INSENSITIVELY: resolveSwiftName permits lowercase ('worklets')
153+
// while toSwiftName produces TitleCase ('Worklets') — an exact-equality check
154+
// passes but the two still collide as directories on the default
155+
// case-insensitive macOS filesystem (synth package layout + headers tree).
156+
const seen /*: Map<string, {name: string, swiftName: string}> */ = new Map();
153157
for (const dep of byName.values()) {
154158
const swiftName = dep.swiftName;
155159
if (swiftName == null) {
156160
continue;
157161
}
158-
const existing = seen.get(swiftName);
162+
const key = swiftName.toLowerCase();
163+
const existing = seen.get(key);
159164
if (existing != null) {
165+
const same = existing.swiftName === swiftName;
160166
throw new Error(
161-
`react-native autolinking: SPM Swift name collision: '${existing}' and '${dep.name}' both resolve to '${swiftName}'. Set a distinct 'spm.name' in one of their react-native.config.js files.`,
167+
`react-native autolinking: SPM Swift name collision: '${existing.name}' ('${existing.swiftName}') and '${dep.name}' ('${swiftName}') ` +
168+
(same
169+
? `both resolve to '${swiftName}'.`
170+
: `differ only in case, which collides on case-insensitive filesystems.`) +
171+
` Set a distinct 'spm.name' in one of their react-native.config.js files.`,
162172
);
163173
}
164-
seen.set(swiftName, dep.name);
174+
seen.set(key, {name: dep.name, swiftName});
165175
}
166176

167177
return Array.from(byName.values());

packages/react-native/scripts/spm/generate-spm-autolinking.js

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -384,7 +384,11 @@ function hasPodspec(absSource /*: string */) /*: boolean */ {
384384
for (const sub of ['', 'ios']) {
385385
const dir = sub === '' ? absSource : path.join(absSource, sub);
386386
try {
387-
if (fs.readdirSync(dir).some(e => e.endsWith('.podspec'))) {
387+
if (
388+
fs
389+
.readdirSync(dir)
390+
.some(e => e.endsWith('.podspec') && !e.startsWith('.spm-scaffold-'))
391+
) {
388392
return true;
389393
}
390394
} catch {
@@ -746,7 +750,10 @@ function extractPodspecHeaderSearchPaths(
746750
let podspecPath /*: ?string */ = null;
747751
try {
748752
const entries = fs.readdirSync(sourceDir);
749-
const candidate = entries.find(e => e.endsWith('.podspec'));
753+
// Skip a crashed run's leftover `.spm-scaffold-<pid>-<name>.podspec` copy.
754+
const candidate = entries.find(
755+
e => e.endsWith('.podspec') && !e.startsWith('.spm-scaffold-'),
756+
);
750757
if (candidate != null) {
751758
podspecPath = path.join(sourceDir, candidate);
752759
}
@@ -1437,7 +1444,19 @@ function main(argv /*:: ?: Array<string> */) /*: void */ {
14371444
continue;
14381445
}
14391446
// spmModule: synth wrapper is the legitimate mechanism (no podspec exists
1440-
// to scaffold from, and the app developer declared it explicitly).
1447+
// to scaffold from, and the app developer declared it explicitly). But a
1448+
// mixed-language module can't be wrapped either — SPM can't compile Swift +
1449+
// C-family sources in one target, and a synth wrapper would fail with a
1450+
// cryptic SPM resolve error. Surface the same friendly diagnostic the
1451+
// community-dep path uses instead of letting SPM emit the cryptic one.
1452+
if (hasMixedLanguageSources(absSource)) {
1453+
throw new Error(
1454+
`react-native autolinking: the spm.module "${target.name}" mixes Swift ` +
1455+
`and C-family (.m/.mm/.c/.cpp) sources, which SwiftPM cannot compile ` +
1456+
`in a single target. Split it into separate single-language modules, ` +
1457+
`or ship a hand-written Package.swift with multiple targets.`,
1458+
);
1459+
}
14411460
const wrapperDir = path.join(packagesDir, target.name);
14421461
wrapperDirs.set(target.name, wrapperDir);
14431462
fs.mkdirSync(wrapperDir, {recursive: true});

packages/react-native/scripts/spm/generate-spm-package.js

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -307,6 +307,38 @@ function main(argv /*:: ?: Array<string> */) /*: void */ {
307307
);
308308
}
309309

310+
// Guard against a stale/wrong React.xcframework layout. The modular
311+
// framework (post-#57285) ships a FLAT Headers/ with React-umbrella.h and
312+
// NO nested Headers/React_Core/. An older/experimental artifact carrying the
313+
// React_Core-per-pod nesting would otherwise fail deep inside the consumer's
314+
// Clang module build with a cryptic "'React/RCTDefines.h' file not found" —
315+
// surface it here with an actionable message instead.
316+
const reactXcfw = raw.React?.xcframeworkPath;
317+
if (reactXcfw != null && fs.existsSync(reactXcfw)) {
318+
const frameworkSlice = fs
319+
.readdirSync(reactXcfw)
320+
.map(d => path.join(reactXcfw, d, 'React.framework'))
321+
.find(f => fs.existsSync(f));
322+
if (frameworkSlice != null) {
323+
const headersDir = path.join(frameworkSlice, 'Headers');
324+
const hasUmbrella = fs.existsSync(
325+
path.join(headersDir, 'React-umbrella.h'),
326+
);
327+
const hasReactCoreNesting = fs.existsSync(
328+
path.join(headersDir, 'React_Core'),
329+
);
330+
if (!hasUmbrella || hasReactCoreNesting) {
331+
throw new Error(
332+
'React.xcframework has an unexpected header layout (missing ' +
333+
'Headers/React-umbrella.h and/or a stale nested Headers/React_Core/). ' +
334+
'This is a pre-#57285 or experimental artifact; a SwiftPM consumer ' +
335+
'cannot build the `React` Clang module from it. Use a post-#57285 ' +
336+
'React.xcframework (current nightly/release or a fresh ios-prebuild build).',
337+
);
338+
}
339+
}
340+
}
341+
310342
const names /*: Array<string> */ = [];
311343
const linkOne = (name /*: string */, target /*: string */) => {
312344
const linkPath = path.join(xcfwLinksDir, `${name}.xcframework`);

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -382,8 +382,11 @@ fi
382382
383383
cd "$SRCROOT"
384384
if command -v npx >/dev/null 2>&1; then
385-
npx react-native spm sync
386-
RC=$?
385+
# \`|| RC=$?\` so a non-zero exit is CAPTURED rather than aborting the phase
386+
# under \`set -e\` — the whole point is to branch on the code below (2 = fail
387+
# the build with a scaffold hint; other non-zero = warn but don't break).
388+
RC=0
389+
npx react-native spm sync || RC=$?
387390
if [ "$RC" -eq 2 ]; then
388391
# Exit 2 = an autolinked community dependency has no Package.swift. The
389392
# autolinker already printed an \`error:\` line per dep (so Xcode shows them

packages/react-native/scripts/spm/read-podspec.js

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -125,18 +125,33 @@ function runPodIpcSpec(podspecPath /*: string */) /*: RawSpec | null */ {
125125
return null;
126126
}
127127
// Some podspecs print diagnostics to stdout during evaluation (e.g. skia:
128-
// `-- SK_GRAPHITE: OFF ...`) before `pod ipc` emits the JSON. Parsing the
129-
// raw stdout then throws and we'd silently fall back to the (much weaker)
130-
// regex parser. Extract just the JSON object (first `{` … last `}`).
128+
// `-- SK_GRAPHITE: OFF ...`) before `pod ipc` emits the JSON, and a
129+
// diagnostic can itself contain braces (`Building {module}`). `pod ipc`
130+
// pretty-prints the spec as a JSON object whose outermost braces sit at
131+
// column 0, so ANCHOR on a line-boundary `{`…`}` rather than the first/last
132+
// brace anywhere — otherwise a brace inside a diagnostic shifts the slice and
133+
// JSON.parse throws. When the authoritative parse is abandoned we log (not
134+
// silently drop to the much weaker, subspec-blind regex parser).
131135
const stdout = result.stdout;
132-
const start = stdout.indexOf('{');
133-
const end = stdout.lastIndexOf('}');
136+
const startMatch = stdout.match(/^\{/m);
137+
const start = startMatch != null ? startMatch.index : -1;
138+
const lastLineBrace = stdout.lastIndexOf('\n}');
139+
const end =
140+
lastLineBrace >= 0 ? lastLineBrace + 2 : stdout.lastIndexOf('}') + 1;
134141
if (start < 0 || end <= start) {
142+
console.warn(
143+
'[read-podspec] pod ipc spec produced no line-anchored JSON object; ' +
144+
'falling back to the regex parser (subspecs/helpers may be missed).',
145+
);
135146
return null;
136147
}
137148
try {
138-
return JSON.parse(stdout.slice(start, end + 1));
139-
} catch {
149+
return JSON.parse(stdout.slice(start, end));
150+
} catch (e) {
151+
console.warn(
152+
`[read-podspec] pod ipc spec JSON parse failed (${e.message}); ` +
153+
'falling back to the regex parser (subspecs/helpers may be missed).',
154+
);
140155
return null;
141156
}
142157
}

0 commit comments

Comments
 (0)