core-crypto icon indicating copy to clipboard operation
core-crypto copied to clipboard
verified publisher icon wireapp

fix: lower iOS deployment target to match the one of the iOS app

Open caldrian opened this issue 3 months ago • 4 comments

What's new in this PR

wireapp/wire-ios uses iOS 16.4 as minimum deployment target. Since wireapp/core-crypto specifies 16.6 a warning is displayed when integrating the framework.

This PR lowers the deployment target so that it matches the one from the iOS app.

PR Submission Checklist for internal contributors
  • The PR Title
    • [ ] conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • [ ] contains a reference JIRA issue number like SQPIT-764
    • [ ] answers the question: If merged, this PR will: ... ³
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

caldrian avatar Nov 24 '25 11:11 caldrian

🐰 Bencher Report

Branchfix/ios-deployment-target
Testbedubuntu-latest

⚠️ WARNING: No Threshold found!

Without a Threshold, no Alerts will ever be generated.

Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the --ci-only-thresholds flag.

Click to view all benchmark results
BenchmarkLatencymicroseconds (µs)
Commit add f(group size)/cs1/mem/1002📈 view plot
⚠️ NO THRESHOLD		15,280.00 µs
Commit add f(group size)/cs1/mem/2📈 view plot
⚠️ NO THRESHOLD		674.48 µs
Commit add f(group size)/cs1/mem/202📈 view plot
⚠️ NO THRESHOLD		3,642.30 µs
Commit add f(group size)/cs1/mem/402📈 view plot
⚠️ NO THRESHOLD		6,603.70 µs
Commit add f(group size)/cs1/mem/602📈 view plot
⚠️ NO THRESHOLD		9,862.10 µs
Commit add f(group size)/cs1/mem/802📈 view plot
⚠️ NO THRESHOLD		12,030.00 µs
Commit add f(number clients)/cs1/mem/1002📈 view plot
⚠️ NO THRESHOLD		753,200.00 µs
Commit add f(number clients)/cs1/mem/2📈 view plot
⚠️ NO THRESHOLD		667.01 µs
Commit add f(number clients)/cs1/mem/202📈 view plot
⚠️ NO THRESHOLD		64,454.00 µs
Commit add f(number clients)/cs1/mem/402📈 view plot
⚠️ NO THRESHOLD		165,990.00 µs
Commit add f(number clients)/cs1/mem/602📈 view plot
⚠️ NO THRESHOLD		328,870.00 µs
Commit add f(number clients)/cs1/mem/802📈 view plot
⚠️ NO THRESHOLD		508,940.00 µs
Commit pending proposals f(group size)/cs1/mem/1002📈 view plot
⚠️ NO THRESHOLD		84,499.00 µs
Commit pending proposals f(group size)/cs1/mem/2📈 view plot
⚠️ NO THRESHOLD		18,317.00 µs
Commit pending proposals f(group size)/cs1/mem/202📈 view plot
⚠️ NO THRESHOLD		31,878.00 µs
Commit pending proposals f(group size)/cs1/mem/402📈 view plot
⚠️ NO THRESHOLD		42,590.00 µs
Commit pending proposals f(group size)/cs1/mem/602📈 view plot
⚠️ NO THRESHOLD		56,504.00 µs
Commit pending proposals f(group size)/cs1/mem/802📈 view plot
⚠️ NO THRESHOLD		67,286.00 µs
Commit pending proposals f(pending size)/cs1/mem/1📈 view plot
⚠️ NO THRESHOLD		14,499.00 µs
Commit pending proposals f(pending size)/cs1/mem/101📈 view plot
⚠️ NO THRESHOLD		83,931.00 µs
Commit pending proposals f(pending size)/cs1/mem/21📈 view plot
⚠️ NO THRESHOLD		26,612.00 µs
Commit pending proposals f(pending size)/cs1/mem/41📈 view plot
⚠️ NO THRESHOLD		42,040.00 µs
Commit pending proposals f(pending size)/cs1/mem/61📈 view plot
⚠️ NO THRESHOLD		55,183.00 µs
Commit pending proposals f(pending size)/cs1/mem/81📈 view plot
⚠️ NO THRESHOLD		70,254.00 µs
Commit remove f(group size)/cs1/mem/1002📈 view plot
⚠️ NO THRESHOLD		9,182.20 µs
Commit remove f(group size)/cs1/mem/2📈 view plot
⚠️ NO THRESHOLD		527.93 µs
Commit remove f(group size)/cs1/mem/202📈 view plot
⚠️ NO THRESHOLD		1,995.60 µs
Commit remove f(group size)/cs1/mem/402📈 view plot
⚠️ NO THRESHOLD		3,567.10 µs
Commit remove f(group size)/cs1/mem/602📈 view plot
⚠️ NO THRESHOLD		5,398.70 µs
Commit remove f(group size)/cs1/mem/802📈 view plot
⚠️ NO THRESHOLD		6,919.20 µs
Commit remove f(number clients)/cs1/mem/1002📈 view plot
⚠️ NO THRESHOLD		11,786.00 µs
Commit remove f(number clients)/cs1/mem/2📈 view plot
⚠️ NO THRESHOLD		116,180.00 µs
Commit remove f(number clients)/cs1/mem/202📈 view plot
⚠️ NO THRESHOLD		95,097.00 µs
Commit remove f(number clients)/cs1/mem/402📈 view plot
⚠️ NO THRESHOLD		74,235.00 µs
Commit remove f(number clients)/cs1/mem/602📈 view plot
⚠️ NO THRESHOLD		53,407.00 µs
Commit remove f(number clients)/cs1/mem/802📈 view plot
⚠️ NO THRESHOLD		32,510.00 µs
Commit update f(group size)/cs1/mem/1002📈 view plot
⚠️ NO THRESHOLD		115,800.00 µs
Commit update f(group size)/cs1/mem/2📈 view plot
⚠️ NO THRESHOLD		693.74 µs
Commit update f(group size)/cs1/mem/202📈 view plot
⚠️ NO THRESHOLD		23,951.00 µs
Commit update f(group size)/cs1/mem/402📈 view plot
⚠️ NO THRESHOLD		46,971.00 µs
Commit update f(group size)/cs1/mem/602📈 view plot
⚠️ NO THRESHOLD		70,724.00 µs
Commit update f(group size)/cs1/mem/802📈 view plot
⚠️ NO THRESHOLD		92,937.00 µs
🐰 View full continuous benchmarking report in Bencher

github-actions[bot] avatar Nov 24 '25 11:11 github-actions[bot]

Thanks @caldrian the CI error doesn't seem related to your change. We'll look into it.

typfel avatar Nov 24 '25 12:11 typfel

I think this is not complete. We have

Makefile:207:   IPHONEOS_DEPLOYMENT_TARGET=16.0 \
interop/src/clients/InteropClient/InteropClient.xcodeproj/project.pbxproj:245:                          IPHONEOS_DEPLOYMENT_TARGET = 18.0;
interop/src/clients/InteropClient/InteropClient.xcodeproj/project.pbxproj:302:                          IPHONEOS_DEPLOYMENT_TARGET = 18.0;
crypto-ffi/bindings/swift/WireCoreCryptoUniffi/WireCoreCryptoUniffi.xcodeproj/project.pbxproj:282:                              IPHONEOS_DEPLOYMENT_TARGET = 16.6;
crypto-ffi/bindings/swift/WireCoreCryptoUniffi/WireCoreCryptoUniffi.xcodeproj/project.pbxproj:324:                              IPHONEOS_DEPLOYMENT_TARGET = 16.6;
crypto-ffi/bindings/swift/WireCoreCrypto/WireCoreCrypto.xcodeproj/project.pbxproj:467:                          IPHONEOS_DEPLOYMENT_TARGET = 16.6;
crypto-ffi/bindings/swift/WireCoreCrypto/WireCoreCrypto.xcodeproj/project.pbxproj:527:                          IPHONEOS_DEPLOYMENT_TARGET = 16.6;

Note in particular the Makefile one. We should be consistent and use the same deployment target everywhere.

istankovic avatar Nov 25 '25 15:11 istankovic

I think this is not complete. We have

Makefile:207:   IPHONEOS_DEPLOYMENT_TARGET=16.0 \
interop/src/clients/InteropClient/InteropClient.xcodeproj/project.pbxproj:245:                          IPHONEOS_DEPLOYMENT_TARGET = 18.0;
interop/src/clients/InteropClient/InteropClient.xcodeproj/project.pbxproj:302:                          IPHONEOS_DEPLOYMENT_TARGET = 18.0;
crypto-ffi/bindings/swift/WireCoreCryptoUniffi/WireCoreCryptoUniffi.xcodeproj/project.pbxproj:282:                              IPHONEOS_DEPLOYMENT_TARGET = 16.6;
crypto-ffi/bindings/swift/WireCoreCryptoUniffi/WireCoreCryptoUniffi.xcodeproj/project.pbxproj:324:                              IPHONEOS_DEPLOYMENT_TARGET = 16.6;
crypto-ffi/bindings/swift/WireCoreCrypto/WireCoreCrypto.xcodeproj/project.pbxproj:467:                          IPHONEOS_DEPLOYMENT_TARGET = 16.6;
crypto-ffi/bindings/swift/WireCoreCrypto/WireCoreCrypto.xcodeproj/project.pbxproj:527:                          IPHONEOS_DEPLOYMENT_TARGET = 16.6;

Note in particular the Makefile one. We should be consistent and use the same deployment target everywhere.

That's only the interop client which we don't ship. All other places have been changed to 16.4.

I'd say this OK. It'll be nicer when we switch to xcodegen.

typfel avatar Nov 25 '25 16:11 typfel

I think this is not complete. We have

Makefile:207:   IPHONEOS_DEPLOYMENT_TARGET=16.0 \
interop/src/clients/InteropClient/InteropClient.xcodeproj/project.pbxproj:245:                          IPHONEOS_DEPLOYMENT_TARGET = 18.0;
interop/src/clients/InteropClient/InteropClient.xcodeproj/project.pbxproj:302:                          IPHONEOS_DEPLOYMENT_TARGET = 18.0;
crypto-ffi/bindings/swift/WireCoreCryptoUniffi/WireCoreCryptoUniffi.xcodeproj/project.pbxproj:282:                              IPHONEOS_DEPLOYMENT_TARGET = 16.6;
crypto-ffi/bindings/swift/WireCoreCryptoUniffi/WireCoreCryptoUniffi.xcodeproj/project.pbxproj:324:                              IPHONEOS_DEPLOYMENT_TARGET = 16.6;
crypto-ffi/bindings/swift/WireCoreCrypto/WireCoreCrypto.xcodeproj/project.pbxproj:467:                          IPHONEOS_DEPLOYMENT_TARGET = 16.6;
crypto-ffi/bindings/swift/WireCoreCrypto/WireCoreCrypto.xcodeproj/project.pbxproj:527:                          IPHONEOS_DEPLOYMENT_TARGET = 16.6;

Note in particular the Makefile one. We should be consistent and use the same deployment target everywhere.

That's only the interop client which we don't ship. All other places have been changed to 16.4.

I'd say this OK. It'll be nicer when we switch to xcodegen.

Ah missed the part where the Makefile was using 16.0, yes we should also align that.

typfel avatar Nov 26 '25 10:11 typfel

That's only the interop client which we don't ship.

We don't ship interop, true, but we use it for testing and I think it makes sense to test with the same configuration used by the artifacts we're shipping. Otherwise we increase the risk that the interop tests pass, while the shipped artifacts are somehow broken.

istankovic avatar Nov 26 '25 10:11 istankovic

@typfel @istankovic Thank you, I adjusted the deployment version on the other places, see https://github.com/wireapp/core-crypto/pull/1619/commits/d0604d8d94f0eca6c7e0f18ce69f3c16f765371a

I noticed that the Makefile has set 14 as minimum target for simulators. Not sure if that should be adjusted as well: https://github.com/wireapp/core-crypto/blob/main/Makefile#L222-L223

For us the only important thing is that your minimum deployment target is not hight than ours, which was the case until now (us 16.4, you: 16.6)

caldrian avatar Nov 27 '25 07:11 caldrian

@caldrian why do we have merge commit f4120281ea19783cf031255d2be75675e3a10ba2?

Please note that we don't use merges in general, and especially not from main into any of the PR branches. If a PR branch is not up-to-date with main, we simply rebase it on top of main.

istankovic avatar Nov 27 '25 08:11 istankovic

@caldrian why do we have merge commit f412028?

Please note that we don't use merges in general, and especially not from main into any of the PR branches. If a PR branch is not up-to-date with main, we simply rebase it on top of main.

@istankovic doesn't GitHub do this automatically?

caldrian avatar Nov 27 '25 16:11 caldrian

@caldrian why do we have merge commit f412028? Please note that we don't use merges in general, and especially not from main into any of the PR branches. If a PR branch is not up-to-date with main, we simply rebase it on top of main.

@istankovic doesn't GitHub do this automatically?

If you mean the rebase button in github, yes, but we're not using it as it completely breaks commit signatures. See https://github.com/orgs/community/discussions/4618.

istankovic avatar Nov 28 '25 09:11 istankovic