[nfc] use _bazel.yml from the release.yml #3079
Conversation
|
I wonder if there's a way to trigger release build on PR to test it? |
| - name: Setup macOS | ||
| if: runner.os == 'macOS' | ||
| run: | | ||
| sudo xcode-select -s "/Applications/Xcode_15.1.app" |
There was a problem hiding this comment.
Test uses macos-15, where only Xcode 16 is available, so this won't work as-is – let's just land #3078 first so that they have the same OS version and the xcode-select command won't be needed
There was a problem hiding this comment.
Thank you for approving! I think ci-macOS-release will also need --config=macos-cross-x86_64 now.
| sudo xcode-select -s "/Applications/Xcode_15.1.app" | ||
| # Install lld and link it to /usr/local/bin. We overwrite any existing link, which may | ||
| # exist from an older pre-installed LLVM version on the runner image. | ||
| brew install lld |
There was a problem hiding this comment.
Can we keep this step release-only? Installing llvm + lld takes some time, which will make builds take longer, especially for builds when the bazel cache is warm. lld provides few advantages to the test build except for slightly faster link speeds which won't make up for this (the macOS system linker has become faster in recent releases).
There was a problem hiding this comment.
Can we keep this step release-only?
hard to do without introducing extra switches. Also arguably we should use the same version of xcode and linker both in release and in test configuration?
There was a problem hiding this comment.
We are using the same Xcode version now that #3078 has been merged. Apple ld and lld on macOS are high-quality linkers that have very good compatibility/interop, so I think we should only spend the 80s - 90s it takes to fetch lld/LLVM for release builds, where we care more about the binary size.
You can just do if: ${{ (if: runner.os == 'macOS') && inputs.upload_binary }}' right? Since the Xcode flag is no longer needed the setup macOS step only contains the lld step. Then we could rename upload_binary to is_release, making it more descriptive.
|
|
||
| # Enable lld identical code folding to significantly reduce binary size. |
There was a problem hiding this comment.
ICF is primarily useful for release builds – link time slightly more than doubles with this. You could make a case for it based on disk space usage and the time it takes to fetch binaries from cache, but based on the download speed that CI has this would be a net loss for test build time.
Edit: Also, ci-macOS-common step is not being added to ci-macOS-release. Let's just add --config=macos_lld_icf to that directly.
Temporarily enabling it with on: pull_request might work, worth trying for sure. Release is not an action that can be started manually (unlike the npm jobs that are marked as workflow_dispatch). This kind of change inherently comes with some risk of breakage, I'm happy to review any fixups following this if needed. Merging the release bazel configuration will make CI a lot easier to maintain in the long term. LGTM otherwise BTW. |
No description provided.