Refactor PR workflow into separate test jobs #6746
Conversation
kraenhansen
added
T-Internal
no-changelog
no-jira-ticket
| - name: Get NPM cache directory | ||
| id: npm-cache-dir | ||
| shell: bash | ||
| run: echo "dir=$(npm config get cache)" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
Not needed as this is already handled by actions/setup-node@v4.
| @@ -131,23 +138,9 @@ jobs: | |||
| - name: Setup node version | |||
| uses: actions/setup-node@v4 | |||
| with: | |||
| node-version: 20.11.1 | |||
There was a problem hiding this comment.
This was for a workaround in prebuild which is not released and this PR includes an upgrade to get the fix: prebuild/prebuild#325
| # On CI, file timestamps change with every run so instead rely on file content hashing | ||
| # https://reactnative.dev/docs/build-speed#using-this-approach-on-a-ci | ||
| - name: Configure ccache | ||
| run: ccache --set-config="compiler_check=content" |
There was a problem hiding this comment.
This is already handled by hendrikmuhs/ccache-action@v1.
|
|
||
| - name: Create Mocha Remote Context | ||
| id: mocha-env | ||
| run: echo "context=syncLogLevel=warn,longTimeoutMs=${{ env.LONG_TIMEOUT }},baseUrl=${{ steps.baas.outputs.baas-url }}" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
Little annoying that this is now duplicated in the workspace code 🤔
| # Because the Xcode project injects its own CCACHE_CONFIGPATH, | ||
| # the configuration set by the ccache action doesn't propagate. | ||
| - name: Expose Ccache config to Xcode | ||
| run: | | ||
| echo "CCACHE_DIR=`ccache --get-config cache_dir`" >> $GITHUB_ENV | ||
| echo "CCACHE_MAXSIZE=`ccache --get-config max_size`" >> $GITHUB_ENV | ||
| echo "CCACHE_COMPILERCHECK=`ccache --get-config compiler_check`" >> $GITHUB_ENV |
There was a problem hiding this comment.
This was the fix for the main issue.
- The React Native ccache scripts use
CCACHE_CONFIGPATH: https://github.com/facebook/react-native/blob/f7fe68861a52a0428698a9352d39a6e39afefe42/packages/react-native/scripts/xcode/ccache-clang.sh#L12 - This makes ccache disregard the system-wide configuration, specifically the
cache_dirwhich is unexpected to thehendrikmuhs/ccache-action@v1action. - We'll instead read the relevant configs set by the action and export these via environment variables for the React Native ccache scripts to pick up during compilation via Xcode.
| @@ -109,7 +109,7 @@ | |||
| } | |||
| }, | |||
| "pod-install:ci": { | |||
| "command": "pod-install", | |||
| "command": "cd ios && pod install --verbose", | |||
There was a problem hiding this comment.
This helped me debug the progress and just gives a bit more output.
| @@ -336,7 +336,7 @@ | |||
| "cross-env": "^7.0.3", | |||
| "glob": "^10.3.12", | |||
| "mocha": "^10.1.0", | |||
| "prebuild": "^12.1.0", | |||
| "prebuild": "^13.0.1", | |||
There was a problem hiding this comment.
See the note above on upgrading "node".
| # Hermes doesn't work with Cocoapods 1.15.0 | ||
| # https://forums.developer.apple.com/forums/thread/745518 | ||
| - name: Install older Cocoapods version | ||
| if: ${{ matrix.variant.os == 'ios' }} | ||
| uses: maxim-lobanov/setup-cocoapods@v1 | ||
| with: | ||
| version: 1.14.3 |
There was a problem hiding this comment.
This got fixed in 1.15.2 which is now installed on the runner and there's no need for the workaround anymore.
kraenhansen
force-pushed
the
kh/separate-pr-workflow
branch
from
954c576
to
5f935a1
Compare
.github/workflows/pr-realm-js.yml
Outdated
| node-electron-tests: | ||
| name: Test ${{ matrix.environment }} on ${{ matrix.runner }} (${{matrix.script_name}}) | ||
| needs: | ||
| - generate-jsi |
There was a problem hiding this comment.
Why is JSI needed for node? Shouldn't it be node-prebuild?
There was a problem hiding this comment.
Great catch - generate-jsi is not needed for the Node / Electron tests 👍
What, How & Why?
My work on #6742 turned into a refactor of the PR workflow separating the test job into three, each depending only on the jobs producing the artifacts they need. Taking us one step closer towards #6530.