-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RDART-962: Use xcode 15.4 #1548
Conversation
Pull Request Test Coverage Report for Build 9285323168Details
💛 - Coveralls |
19be2b2
to
17827e8
Compare
0fe31e9
to
3d02b62
Compare
@@ -3,18 +3,10 @@ name: Dart desktop tests | |||
on: | |||
workflow_call: | |||
inputs: | |||
os: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I'm a huge fan of the changes in this file. I understand that it removes some redundant pieces of information, but it complicates the setup somewhat and forces readers to familiarize themselves with the architectures of the various github-hosted runners.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous code had assumptions about the architecture, that didn't actually match the runners, implicitly assuming arch was x86_64
unless architecture
was set to arm
. That is not correct for macos-14, which is being rolled out as macos-latest
as of yesterday (github/roadmap#926).
One thing I regret is that I cannot show the arch in the job title, as we don't know what runner is being used yet, so it is only shown in the steps. And obviously I would have preferred for github action expressions to be expressive enough to lowercase a simple string 🙄
What if I reintroduce the architecture
argument, and make a step that explicitly test that it actually match runner.arch
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'd be fine with that.
.github/workflows/ci.yml
Outdated
submodules: 'recursive' | ||
submodules: "recursive" | ||
|
||
- name: Select XCode for MacOS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to increase the ulimit here it seems.
@@ -7,4 +7,4 @@ set -o pipefail | |||
cd "$(dirname "$0")/.." | |||
|
|||
cmake --preset macos | |||
cmake --build --preset macos --config MinSizeRel -- -destination "generic/platform=macOS" | |||
cmake --build --preset macos --config Release -- -destination "generic/platform=macOS" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MinSizeRel
becomes -Os
which apparently clang++
(from XCode 15.x) don't like, hence this change.
@@ -168,7 +168,7 @@ | |||
"configurePreset": "macos", | |||
"nativeToolOptions": [ | |||
"-destination", | |||
"platform=macOS" | |||
"generic/platform=macOS" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of XCode 15.3 it is apparently important to use generic/platform=macOS
during preset, to actually end up with a universal binary (ie. supporting both arm64
and x86_64
).
The downside of this change is that we also build for x86_64
on our dev machines, but given that we use ccache
it is perhaps a non-issue in practice.
1da63ba
to
f69615f
Compare
375ceae
to
ab64653
Compare
6fb86bd
to
0247de5
Compare
3c999f4
to
49aa8af
Compare
Update CHANGELOG Use Release instead of MinSIzeRel
- name: Select XCode for MacOS | ||
if: startsWith(inputs.runner, 'macos') | ||
run: | | ||
xcodes installed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't, but I find it useful to see what versions are available when looking at the output of the step
run: ulimit -n 10240 | ||
if: ${{ contains(inputs.os, 'macos') }} | ||
run: | | ||
ulimit -n |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To see the current limit in the output of the step before setting it.
Co-authored-by: Nikola Irinchev <irinchev@me.com>
* main: RDART-1020: Fix writeAsync behaviour (#1666) RDART-999: Fix flutter test dlopen (#1623) RDART-1045: Expose setrlimit ios (#1700) RDART-962: Use xcode 15.4 (#1548) # Conflicts: # packages/realm_dart/lib/src/cli/install/install_command.dart
* main: Add vNext Changelog header (#1717) [Release 3.0.0] (#1716) libraryVersion moved to realm_libary.dart (take 2) libraryVersion moved to realm_libary.dart Update CHANGELOG (#1715) Github composite action for setting up flutter on runner (#1710) RDART-866: kn/decimal128 web support (#1713) Reduce expected gain of memEquals for test stability Refresh after awaiting download to stabilize tests RDART-866: Minimal web support (#1699) RDART-1052: Update realm-core to v14.9.0 (#1704) RDART-1020: Fix writeAsync behaviour (#1666) RDART-999: Fix flutter test dlopen (#1623) RDART-1045: Expose setrlimit ios (#1700) RDART-962: Use xcode 15.4 (#1548) RDART-1039: Drop catalyst support. Flutter doesn't support it (#1696) # Conflicts: # packages/realm_dart/src/realm-core
Fixes: #1547