-
-
Notifications
You must be signed in to change notification settings - Fork 215
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
Address Trim warnings and prevent future regressions #3841
Changes from all commits
f0c2233
1fcfee5
4e8c7d0
88d70bc
04bc9ec
2117208
b3f71a5
55ea8c0
81f2994
e371e7e
af59166
94370ad
cd1aafb
4128643
a49791c
a9de355
6102abb
6952ada
b520842
2aee802
fce29a9
daa1b71
d86dee2
531de39
0a3e014
0a44a1c
a4a025c
f67bd14
fbad609
bb9efa5
13caecc
5597967
634a616
9af1b39
7188b44
98173c5
1621887
587c3cb
46712a0
9e40b76
d1a92b7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,7 @@ jobs: | |
strategy: | ||
fail-fast: false | ||
matrix: | ||
# Pin macos to get the version of XCode that we need: https://github.com/actions/runner-images/issues/10703 | ||
# Pin macos to get the version of Xcode that we need: https://github.com/actions/runner-images/issues/10703 | ||
os: [ubuntu-latest, windows-latest, macos-15] | ||
|
||
steps: | ||
|
@@ -53,7 +53,7 @@ jobs: | |
strategy: | ||
fail-fast: false | ||
matrix: | ||
# Pin macos to get the version of XCode that we need: https://github.com/actions/runner-images/issues/10703 | ||
# Pin macos to get the version of Xcode that we need: https://github.com/actions/runner-images/issues/10703 | ||
os: [ubuntu-latest, windows-latest, macos-15] | ||
|
||
steps: | ||
|
@@ -154,7 +154,7 @@ jobs: | |
strategy: | ||
fail-fast: false | ||
matrix: | ||
# Pin macos to get the version of XCode that we need: https://github.com/actions/runner-images/issues/10703 | ||
# Pin macos to get the version of Xcode that we need: https://github.com/actions/runner-images/issues/10703 | ||
os: [ubuntu-latest, windows-latest, macos-15] | ||
|
||
steps: | ||
|
@@ -187,6 +187,52 @@ jobs: | |
with: | ||
path: integration-test | ||
|
||
|
||
trim-analysis: | ||
name: Trim analysis | ||
runs-on: macos-15 | ||
|
||
steps: | ||
- name: Checkout | ||
uses: actions/checkout@v4 | ||
with: | ||
submodules: recursive | ||
fetch-depth: 2 # default is 1 and codecov needs > 1 | ||
|
||
# We use macOS for the final publishing build so we get all the iOS/macCatalyst targets in the packages | ||
- name: Set Environment Variables | ||
run: echo "CI_PUBLISHING_BUILD=true" >> $GITHUB_ENV | ||
|
||
- name: Download sentry-native (macOS) | ||
uses: actions/cache/restore@v4 | ||
with: | ||
path: src/Sentry/Platforms/Native/sentry-native | ||
key: sentry-native-macOS-${{ hashFiles('scripts/build-sentry-native.ps1') }}-${{ hashFiles('.git/modules/modules/sentry-native/HEAD') }} | ||
fail-on-cache-miss: true | ||
|
||
- name: Setup Environment | ||
uses: ./.github/actions/environment | ||
|
||
- name: Build Native Dependencies | ||
uses: ./.github/actions/buildnative | ||
|
||
- name: Install Android SDKs | ||
if: runner.os == 'macOS' | ||
run: | | ||
dotnet build src/Sentry/Sentry.csproj -t:InstallAndroidDependencies -f:net8.0-android34.0 -p:AcceptAndroidSDKLicenses=True -p:AndroidSdkPath="/usr/local/lib/android/sdk/" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we need to set the specific android version here? I imagine it might go out of sync when we bump .NET for example There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What we're passing here is the target framework The main reason we're doing this is to ensure the android34.0 SDK gets installed as this is missing from the new GitHub runners. If we remove this, the build fails later on in the workflow. |
||
|
||
- name: Publish Test app (macOS) | ||
run: dotnet publish test/Sentry.TrimTest/Sentry.TrimTest.csproj -c Release -r osx-arm64 | ||
|
||
- name: Publish Test app (Android) | ||
run: dotnet publish test/Sentry.MauiTrimTest/Sentry.MauiTrimTest.csproj -c Release -f net9.0-android35.0 -r android-arm64 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we use Android 35 here and 34 above. Is that by default? We could set this as vars at the top of the workflow at least? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 34.0 is used abote with the So yeah, unfortunately we want to use two separate target frameworks for the two different build steps. |
||
|
||
# We can't publish iOS applications on CI yet. We'd need a valid bundle identifier and to install the relevant | ||
# certificates/profiles on CI (presumably certs associated with the Sentry org). | ||
# See https://youtrack.jetbrains.com/issue/RIDER-17115/Could-not-find-any-available-provisioning-profiles-for-iOS | ||
# - name: Publish Test app (iOS) | ||
# run: dotnet publish test/Sentry.MauiTrimTest/Sentry.MauiTrimTest.csproj -c Release -f net9.0-ios18.0 -r ios-arm64 | ||
|
||
test-solution-filters: | ||
runs-on: ubuntu-latest | ||
if: ${{ !startsWith(github.ref_name, 'release/') }} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,36 @@ | ||
using Sentry.Internal; | ||
|
||
namespace Sentry.Maui.Internal; | ||
|
||
internal static class PageNavigationExtensions | ||
{ | ||
private static readonly PropertyInfo? DestinationPageProperty = | ||
typeof(NavigatedFromEventArgs).GetProperty("DestinationPage", BindingFlags.Instance | BindingFlags.NonPublic); | ||
private static readonly PropertyInfo? DestinationPageProperty; | ||
private static readonly PropertyInfo? PreviousPageProperty; | ||
|
||
private static readonly PropertyInfo? PreviousPageProperty = | ||
typeof(NavigatedToEventArgs).GetProperty("PreviousPage", BindingFlags.Instance | BindingFlags.NonPublic); | ||
[UnconditionalSuppressMessage("Trimming", "IL2075: DynamicallyAccessedMembers", Justification = AotHelper.AvoidAtRuntime)] | ||
static PageNavigationExtensions() | ||
{ | ||
if (AotHelper.IsTrimmed) | ||
{ | ||
return; | ||
} | ||
DestinationPageProperty = typeof(NavigatedFromEventArgs) | ||
.GetProperty("DestinationPage", BindingFlags.Instance | BindingFlags.NonPublic); | ||
PreviousPageProperty = typeof(NavigatedToEventArgs) | ||
.GetProperty("PreviousPage", BindingFlags.Instance | BindingFlags.NonPublic); | ||
} | ||
|
||
/// <summary> | ||
/// Reads the (internal) NavigatedFromEventArgs.DestinationPage property via reflection. | ||
/// Note that this will return null if trimming is enabled. | ||
/// </summary> | ||
public static Page? GetDestinationPage(this NavigatedFromEventArgs eventArgs) => | ||
DestinationPageProperty?.GetValue(eventArgs) as Page; | ||
|
||
/// <summary> | ||
/// Reads the (internal) NavigatedFromEventArgs.PreviousPage property via reflection. | ||
/// Note that this will return null if trimming is enabled. | ||
/// </summary> | ||
public static Page? GetPreviousPage(this NavigatedToEventArgs eventArgs) => | ||
PreviousPageProperty?.GetValue(eventArgs) as Page; | ||
} |
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.
can this be
latest
or we need to pin?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 pin to get the correct version of Xcode. As of about a month ago, the GitHub macos runners are associated with specific major versions of Xcode.
I've basically started trying to pin everything (specific runner version, .NET workload versions via the ios18.0 or android35.0 monkiers etc.) since it gets us closer to deterministic builds. If we don't do this, too much changes arbitrarily between different runs against the same commit and CI fails arbitrarily.