Skip to content

Commit f6714d5

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 f6714d5

File tree

6 files changed

+288
-257
lines changed

6 files changed

+288
-257
lines changed

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

Lines changed: 33 additions & 33 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:
@@ -43,14 +43,14 @@ jobs:
4343
uses: actions/cache/restore@v4
4444
with:
4545
key: hermes-v1-${{ steps.resolve.outputs.hermes-commit }}-Debug
46-
path: /tmp/hermes-destroot
46+
path: hermes-destroot
4747

4848
- name: Upload cached Hermes artifacts
4949
if: steps.cache.outputs.cache-hit == 'true'
5050
uses: actions/upload-artifact@v4
5151
with:
5252
name: hermes-artifacts
53-
path: /tmp/hermes-destroot
53+
path: hermes-destroot
5454
retention-days: 1
5555

5656
build-hermesc:
@@ -68,17 +68,17 @@ 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: hermes
7676

7777
- name: Build hermesc
78-
working-directory: /tmp/hermes
78+
working-directory: hermes
7979
env:
80-
HERMES_PATH: /tmp/hermes
81-
JSI_PATH: /tmp/hermes/API/jsi
80+
HERMES_PATH: ${{ github.workspace }}/hermes
81+
JSI_PATH: ${{ github.workspace }}/hermes/API/jsi
8282
MAC_DEPLOYMENT_TARGET: '14.0'
8383
run: |
8484
source $GITHUB_WORKSPACE/packages/react-native/sdks/hermes-engine/utils/build-apple-framework.sh
@@ -88,7 +88,7 @@ jobs:
8888
uses: actions/upload-artifact@v4
8989
with:
9090
name: hermesc
91-
path: /tmp/hermes/build_host_hermesc
91+
path: hermes/build_host_hermesc
9292
retention-days: 1
9393

9494
build-hermes-slice:
@@ -118,27 +118,27 @@ 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: hermes
126126

127127
- name: Download hermesc
128128
uses: actions/download-artifact@v4
129129
with:
130130
name: hermesc
131-
path: /tmp/hermes/build_host_hermesc
131+
path: hermes/build_host_hermesc
132132

133133
- name: Restore hermesc permissions
134-
run: chmod +x /tmp/hermes/build_host_hermesc/bin/hermesc
134+
run: chmod +x ${{ github.workspace }}/hermes/build_host_hermesc/bin/hermesc
135135

136136
- name: Build Hermes slice (${{ matrix.slice }})
137-
working-directory: /tmp/hermes
137+
working-directory: hermes
138138
env:
139139
BUILD_TYPE: Debug
140-
HERMES_PATH: /tmp/hermes
141-
JSI_PATH: /tmp/hermes/API/jsi
140+
HERMES_PATH: ${{ github.workspace }}/hermes
141+
JSI_PATH: ${{ github.workspace }}/hermes/API/jsi
142142
IOS_DEPLOYMENT_TARGET: '15.1'
143143
MAC_DEPLOYMENT_TARGET: '14.0'
144144
XROS_DEPLOYMENT_TARGET: '1.0'
@@ -150,7 +150,7 @@ jobs:
150150
uses: actions/upload-artifact@v4
151151
with:
152152
name: hermes-slice-${{ matrix.slice }}
153-
path: /tmp/hermes/destroot
153+
path: hermes/destroot
154154
retention-days: 1
155155

156156
assemble-hermes:
@@ -172,26 +172,26 @@ jobs:
172172

173173
- name: Assemble destroot from slices
174174
run: |
175-
mkdir -p /tmp/hermes/destroot/Library/Frameworks
175+
mkdir -p ${{ github.workspace }}/hermes/destroot/Library/Frameworks
176176
for slice_dir in /tmp/slices/hermes-slice-*; do
177177
slice_name=$(basename "$slice_dir" | sed 's/hermes-slice-//')
178178
echo "Copying slice: $slice_name"
179-
cp -R "$slice_dir/Library/Frameworks/$slice_name" /tmp/hermes/destroot/Library/Frameworks/
179+
cp -R "$slice_dir/Library/Frameworks/$slice_name" ${{ github.workspace }}/hermes/destroot/Library/Frameworks/
180180
# Copy include and bin directories (identical across slices, only need one copy)
181-
if [ -d "$slice_dir/include" ] && [ ! -d /tmp/hermes/destroot/include ]; then
182-
cp -R "$slice_dir/include" /tmp/hermes/destroot/
181+
if [ -d "$slice_dir/include" ] && [ ! -d ${{ github.workspace }}/hermes/destroot/include ]; then
182+
cp -R "$slice_dir/include" ${{ github.workspace }}/hermes/destroot/
183183
fi
184184
if [ -d "$slice_dir/bin" ]; then
185-
cp -R "$slice_dir/bin" /tmp/hermes/destroot/
185+
cp -R "$slice_dir/bin" ${{ github.workspace }}/hermes/destroot/
186186
fi
187187
done
188188
echo "Assembled destroot contents:"
189-
ls -la /tmp/hermes/destroot/Library/Frameworks/
189+
ls -la ${{ github.workspace }}/hermes/destroot/Library/Frameworks/
190190
191191
- name: Create universal xcframework
192-
working-directory: /tmp/hermes
192+
working-directory: hermes
193193
env:
194-
HERMES_PATH: /tmp/hermes
194+
HERMES_PATH: ${{ github.workspace }}/hermes
195195
run: |
196196
source $GITHUB_WORKSPACE/packages/react-native/sdks/hermes-engine/utils/build-apple-framework.sh
197197
create_universal_framework "iphoneos" "iphonesimulator" "macosx" "xros" "xrsimulator"
@@ -200,13 +200,13 @@ jobs:
200200
uses: actions/cache/save@v4
201201
with:
202202
key: hermes-v1-${{ needs.resolve-hermes.outputs.hermes-commit }}-Debug
203-
path: /tmp/hermes/destroot
203+
path: hermes/destroot
204204

205205
- name: Upload Hermes artifacts
206206
uses: actions/upload-artifact@v4
207207
with:
208208
name: hermes-artifacts
209-
path: /tmp/hermes/destroot
209+
path: hermes/destroot
210210
retention-days: 1
211211

212212
build-spm:

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,

0 commit comments

Comments
 (0)