Skip to content

Commit 89b00ec

Browse files
Saadnajmiclaude
andcommitted
refactor: address PR review comments (round 2)
- Rename workflow to "Build SwiftPM" (tido64) - Replace shallow clone + fetch with actions/checkout for Hermes (tido64) - Use efficient git init + fetch pattern in buildFromHermesCommit (tido64) - Extract reusable build env object in hermes.js (tido64) - Move macOS version resolution to fork-only macosVersionResolver.js (tido64) - Replace platformLinkerSettings with #if os(macOS) in Package.swift (Saad) - Revert CMAKE_OSX_DEPLOYMENT_TARGET from build-apple-framework.sh (Saad) - Add // [macOS] tag to os require in hermes.js (tido64) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 233ee43 commit 89b00ec

File tree

6 files changed

+266
-235
lines changed

6 files changed

+266
-235
lines changed

.github/workflows/microsoft-build-spm.yml

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
name: Build SPM
1+
name: Build SwiftPM
22

33
on:
44
workflow_call:
@@ -68,11 +68,11 @@ jobs:
6868
run: sudo xcode-select --switch /Applications/Xcode_16.2.app
6969

7070
- name: Clone Hermes
71-
run: |
72-
git clone --depth 1 https://github.com/facebook/hermes.git /tmp/hermes
73-
cd /tmp/hermes
74-
git fetch --depth 1 origin ${{ needs.resolve-hermes.outputs.hermes-commit }}
75-
git checkout ${{ needs.resolve-hermes.outputs.hermes-commit }}
71+
uses: actions/checkout@v4
72+
with:
73+
repository: facebook/hermes
74+
ref: ${{ needs.resolve-hermes.outputs.hermes-commit }}
75+
path: /tmp/hermes
7676

7777
- name: Build hermesc
7878
working-directory: /tmp/hermes
@@ -118,11 +118,11 @@ jobs:
118118
sudo xcodebuild -runFirstLaunch
119119
120120
- name: Clone Hermes
121-
run: |
122-
git clone --depth 1 https://github.com/facebook/hermes.git /tmp/hermes
123-
cd /tmp/hermes
124-
git fetch --depth 1 origin ${{ needs.resolve-hermes.outputs.hermes-commit }}
125-
git checkout ${{ needs.resolve-hermes.outputs.hermes-commit }}
121+
uses: actions/checkout@v4
122+
with:
123+
repository: facebook/hermes
124+
ref: ${{ needs.resolve-hermes.outputs.hermes-commit }}
125+
path: /tmp/hermes
126126

127127
- name: Download hermesc
128128
uses: actions/download-artifact@v4

packages/react-native/Package.swift

Lines changed: 24 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -243,18 +243,23 @@ let reactJsErrorHandler = RNTarget(
243243

244244
/// React-graphicsApple
245245
/// This represents the React-graphicsApple BUCK module
246+
// [macOS: UIKit on iOS/visionOS, AppKit on macOS
247+
#if os(macOS)
246248
let reactGraphicsApple = RNTarget(
247249
name: .reactGraphicsApple,
248250
path: "ReactCommon/react/renderer/graphics/platform/ios",
249-
linkedFrameworks: ["CoreGraphics"],
250-
// [macOS: UIKit on iOS/visionOS, AppKit on macOS
251-
platformLinkerSettings: [
252-
.linkedFramework("UIKit", .when(platforms: [.iOS, .visionOS])),
253-
.linkedFramework("AppKit", .when(platforms: [.macOS])),
254-
],
255-
// macOS]
251+
linkedFrameworks: ["CoreGraphics", "AppKit"],
252+
dependencies: [.reactDebug, .jsi, .reactUtils, .reactNativeDependencies]
253+
)
254+
#else
255+
let reactGraphicsApple = RNTarget(
256+
name: .reactGraphicsApple,
257+
path: "ReactCommon/react/renderer/graphics/platform/ios",
258+
linkedFrameworks: ["CoreGraphics", "UIKit"],
256259
dependencies: [.reactDebug, .jsi, .reactUtils, .reactNativeDependencies]
257260
)
261+
#endif
262+
// macOS]
258263

259264
/// React-graphics.podspec
260265
let reactGraphics = RNTarget(
@@ -541,17 +546,21 @@ let reactSettings = RNTarget(
541546
// [macOS
542547
/// React-RCTUIKit.podspec
543548
/// UIKit/AppKit compatibility layer for React Native macOS.
549+
#if os(macOS)
544550
let reactRCTUIKit = RNTarget(
545551
name: .reactRCTUIKit,
546552
path: "React/RCTUIKit",
547-
// [macOS: UIKit on iOS/visionOS, AppKit on macOS
548-
platformLinkerSettings: [
549-
.linkedFramework("UIKit", .when(platforms: [.iOS, .visionOS])),
550-
.linkedFramework("AppKit", .when(platforms: [.macOS])),
551-
],
552-
// macOS]
553+
linkedFrameworks: ["AppKit"],
553554
excludedPaths: ["README.md"]
554555
)
556+
#else
557+
let reactRCTUIKit = RNTarget(
558+
name: .reactRCTUIKit,
559+
path: "React/RCTUIKit",
560+
linkedFrameworks: ["UIKit"],
561+
excludedPaths: ["README.md"]
562+
)
563+
#endif
555564
// macOS]
556565

557566
// MARK: Target list
@@ -659,16 +668,14 @@ class BinaryTarget: BaseTarget {
659668

660669
class RNTarget: BaseTarget {
661670
let linkedFrameworks: [String]
662-
let platformLinkerSettings: [LinkerSetting] // [macOS] Platform-conditional framework linking (e.g. UIKit vs AppKit)
663671
let excludedPaths: [String]
664672
let dependencies: [String]
665673
let sources: [String]?
666674
let publicHeadersPath: String?
667675
let defines: [CXXSetting]
668676

669-
init(name: String, path: String, searchPaths: [String] = [], linkedFrameworks: [String] = [], platformLinkerSettings: [LinkerSetting] = [], excludedPaths: [String] = [], dependencies: [String] = [], sources: [String]? = nil, publicHeadersPath: String? = ".", defines: [CXXSetting] = []) {
677+
init(name: String, path: String, searchPaths: [String] = [], linkedFrameworks: [String] = [], excludedPaths: [String] = [], dependencies: [String] = [], sources: [String]? = nil, publicHeadersPath: String? = ".", defines: [CXXSetting] = []) {
670678
self.linkedFrameworks = linkedFrameworks
671-
self.platformLinkerSettings = platformLinkerSettings
672679
self.excludedPaths = excludedPaths
673680
self.dependencies = dependencies
674681
self.sources = sources
@@ -704,7 +711,7 @@ class RNTarget: BaseTarget {
704711
override func target(targets: [BaseTarget]) -> Target {
705712
let searchPaths: [String] = self.headerSearchPaths(targets: targets)
706713

707-
let linkerSettings = self.linkedFrameworks.reduce([]) { $0 + [LinkerSetting.linkedFramework($1)] } + self.platformLinkerSettings // [macOS]
714+
let linkerSettings = self.linkedFrameworks.reduce([]) { $0 + [LinkerSetting.linkedFramework($1)] }
708715

709716
return Target.reactNativeTarget(
710717
name: self.name,

packages/react-native/scripts/ios-prebuild/hermes.js

Lines changed: 30 additions & 148 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,11 @@
88
* @format
99
*/
1010

11+
const {findMatchingHermesVersion, hermesCommitAtMergeBase} = require('./macosVersionResolver'); // [macOS]
1112
const {computeNightlyTarballURL, createLogger} = require('./utils');
1213
const {execSync} = require('child_process');
1314
const fs = require('fs');
14-
const os = require('os');
15+
const os = require('os'); // [macOS]
1516
const path = require('path');
1617
const stream = require('stream');
1718
const {promisify} = require('util');
@@ -23,124 +24,6 @@ const hermesLog = createLogger('Hermes');
2324
import type {BuildFlavor, Destination, Platform} from './types';
2425
*/
2526

26-
// [macOS
27-
/**
28-
* For react-native-macos stable branches, maps the macOS package version
29-
* to the upstream react-native version using peerDependencies.
30-
* Returns null for version 1000.0.0 (main branch dev version).
31-
*
32-
* This is the JavaScript equivalent of the Ruby `findMatchingHermesVersion`
33-
* in sdks/hermes-engine/hermes-utils.rb.
34-
*/
35-
function findMatchingHermesVersion(
36-
packageJsonPath /*: string */,
37-
) /*: ?string */ {
38-
const pkg = JSON.parse(fs.readFileSync(packageJsonPath, 'utf8'));
39-
40-
if (pkg.version === '1000.0.0') {
41-
hermesLog(
42-
'Main branch detected (1000.0.0), no matching upstream Hermes version',
43-
);
44-
return null;
45-
}
46-
47-
if (pkg.peerDependencies && pkg.peerDependencies['react-native']) {
48-
const upstreamVersion = pkg.peerDependencies['react-native'];
49-
hermesLog(
50-
`Mapped macOS version ${pkg.version} to upstream RN version: ${upstreamVersion}`,
51-
);
52-
return upstreamVersion;
53-
}
54-
55-
hermesLog(
56-
'No matching Hermes version found in peerDependencies. Defaulting to package version.',
57-
);
58-
return null;
59-
}
60-
61-
/**
62-
* Finds the Hermes commit at the merge base with facebook/react-native.
63-
* Used on the main branch (1000.0.0) where no prebuilt artifacts exist.
64-
*
65-
* Since react-native-macos lags slightly behind facebook/react-native, we can't always use
66-
* the latest Hermes commit because Hermes and JSI don't always guarantee backwards compatibility.
67-
* Instead, we take the commit hash of Hermes at the time of the merge base with facebook/react-native.
68-
*
69-
* This is the JavaScript equivalent of the Ruby `hermes_commit_at_merge_base`
70-
* in sdks/hermes-engine/hermes-utils.rb.
71-
*/
72-
function hermesCommitAtMergeBase() /*: {| commit: string, timestamp: string |} */ {
73-
const HERMES_GITHUB_URL = 'https://github.com/facebook/hermes.git';
74-
75-
// Fetch upstream react-native
76-
hermesLog('Fetching facebook/react-native to find merge base...');
77-
try {
78-
execSync('git fetch -q https://github.com/facebook/react-native.git', {
79-
stdio: 'pipe',
80-
});
81-
} catch (e) {
82-
abort(
83-
'[Hermes] Failed to fetch facebook/react-native into the local repository.',
84-
);
85-
}
86-
87-
// Find merge base between our HEAD and upstream's HEAD
88-
const mergeBase = execSync('git merge-base FETCH_HEAD HEAD', {
89-
encoding: 'utf8',
90-
}).trim();
91-
if (!mergeBase) {
92-
abort(
93-
"[Hermes] Unable to find the merge base between our HEAD and upstream's HEAD.",
94-
);
95-
}
96-
97-
// Get timestamp of merge base
98-
const timestamp = execSync(`git show -s --format=%ci ${mergeBase}`, {
99-
encoding: 'utf8',
100-
}).trim();
101-
if (!timestamp) {
102-
abort(
103-
`[Hermes] Unable to extract the timestamp for the merge base (${mergeBase}).`,
104-
);
105-
}
106-
107-
// Clone Hermes bare (minimal) into a temp directory and find the commit
108-
hermesLog(
109-
`Merge base timestamp: ${timestamp}. Cloning Hermes to find matching commit...`,
110-
);
111-
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'hermes-'));
112-
const hermesGitDir = path.join(tmpDir, 'hermes.git');
113-
114-
try {
115-
// Explicitly use Hermes 'main' branch since the default branch changed to 'static_h' (Hermes V1)
116-
execSync(
117-
`git clone -q --bare --filter=blob:none --single-branch --branch main ${HERMES_GITHUB_URL} "${hermesGitDir}"`,
118-
{stdio: 'pipe', timeout: 120000},
119-
);
120-
121-
// Find the Hermes commit at the time of the merge base on branch 'main'
122-
const commit = execSync(
123-
`git --git-dir="${hermesGitDir}" rev-list -1 --before="${timestamp}" refs/heads/main`,
124-
{encoding: 'utf8'},
125-
).trim();
126-
127-
if (!commit) {
128-
abort(
129-
`[Hermes] Unable to find the Hermes commit hash at time ${timestamp} on branch 'main'.`,
130-
);
131-
}
132-
133-
hermesLog(
134-
`Using Hermes commit from the merge base with facebook/react-native: ${commit} (timestamp: ${timestamp})`,
135-
);
136-
return {commit, timestamp};
137-
} finally {
138-
// Clean up temp directory
139-
fs.rmSync(tmpDir, {recursive: true, force: true});
140-
}
141-
}
142-
// macOS]
143-
14427
/**
14528
* Downloads hermes artifacts from the specified version and build type. If you want to specify a specific
14629
* version of hermes, use the HERMES_VERSION environment variable. The path to the artifacts will be inside
@@ -556,23 +439,20 @@ async function buildFromHermesCommit(
556439
const hermesDir = path.join(tmpDir, 'hermes');
557440

558441
try {
559-
// Clone Hermes at the identified commit
442+
// Clone Hermes at the identified commit using the most efficient
443+
// single-fetch pattern (see https://github.com/actions/checkout)
560444
hermesLog(`Cloning Hermes at commit ${commit}...`);
561-
execSync(`git clone --depth 1 ${HERMES_GITHUB_URL} "${hermesDir}"`, {
562-
stdio: 'inherit',
563-
timeout: 300000,
564-
});
565-
execSync(`git -C "${hermesDir}" fetch --depth 1 origin ${commit}`, {
566-
stdio: 'inherit',
567-
timeout: 120000,
568-
});
569-
execSync(`git -C "${hermesDir}" checkout ${commit}`, {
570-
stdio: 'inherit',
571-
});
445+
execSync(`git init "${hermesDir}"`, {stdio: 'inherit'});
446+
execSync(
447+
`git -C "${hermesDir}" remote add origin ${HERMES_GITHUB_URL}`,
448+
{stdio: 'inherit'},
449+
);
450+
execSync(
451+
`git -C "${hermesDir}" fetch --no-tags --depth 1 origin +${commit}:refs/remotes/origin/main`,
452+
{stdio: 'inherit', timeout: 300000},
453+
);
454+
execSync(`git -C "${hermesDir}" checkout main`, {stdio: 'inherit'});
572455

573-
// The build-ios-framework.sh script runs from the hermes directory.
574-
// It sources build-apple-framework.sh which sets HERMES_PATH relative to itself,
575-
// but we override it to point to the cloned Hermes repo.
576456
const reactNativeRoot = path.resolve(__dirname, '..', '..');
577457
const buildScript = path.join(
578458
reactNativeRoot,
@@ -582,23 +462,25 @@ async function buildFromHermesCommit(
582462
'build-ios-framework.sh',
583463
);
584464

465+
const buildEnv = {
466+
...process.env,
467+
BUILD_TYPE: buildType,
468+
HERMES_PATH: hermesDir,
469+
JSI_PATH: path.join(hermesDir, 'API', 'jsi'),
470+
REACT_NATIVE_PATH: reactNativeRoot,
471+
// Deployment targets matching react-native-macos minimums
472+
IOS_DEPLOYMENT_TARGET: '15.1',
473+
MAC_DEPLOYMENT_TARGET: '14.0',
474+
XROS_DEPLOYMENT_TARGET: '1.0',
475+
RELEASE_VERSION: version,
476+
};
477+
585478
hermesLog(`Building Hermes frameworks (${buildType})...`);
586479
execSync(`bash "${buildScript}"`, {
587480
cwd: hermesDir,
588481
stdio: 'inherit',
589482
timeout: 3600000, // 60 minutes
590-
env: {
591-
...process.env,
592-
BUILD_TYPE: buildType,
593-
HERMES_PATH: hermesDir,
594-
JSI_PATH: path.join(hermesDir, 'API', 'jsi'),
595-
REACT_NATIVE_PATH: reactNativeRoot,
596-
// Deployment targets matching react-native-macos minimums
597-
IOS_DEPLOYMENT_TARGET: '15.1',
598-
MAC_DEPLOYMENT_TARGET: '14.0',
599-
XROS_DEPLOYMENT_TARGET: '1.0',
600-
RELEASE_VERSION: version,
601-
},
483+
env: buildEnv,
602484
});
603485

604486
// Create tarball from the destroot (same structure as Maven artifacts)
@@ -651,6 +533,6 @@ function abort(message /*: string */) {
651533

652534
module.exports = {
653535
prepareHermesArtifactsAsync,
654-
findMatchingHermesVersion, // [macOS]
655-
hermesCommitAtMergeBase, // [macOS]
536+
findMatchingHermesVersion, // [macOS] re-exported from macosVersionResolver.js
537+
hermesCommitAtMergeBase, // [macOS] re-exported from macosVersionResolver.js
656538
};

0 commit comments

Comments
 (0)