Skip to content
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

fix: define separate module names for UIKitless configurations #4140

Merged
merged 27 commits into from
Jul 9, 2024

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented Jul 2, 2024

Define a different module name for the configurations that build without UIKit enabled. Ideally we split out a separate submodule for UIKit stuff a la #3219, but this is a workaround in the meantime.

Had to fix an issue with the test targets' configuration. They included Sentry.xcconfig and so inherited the module names. But, they never should have inherited from that config. It looks like they overrode everything anyways, except for the workaround using HEADER_SEARCH_PATHS.

fixes ##4086

Copy link

codecov bot commented Jul 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.348%. Comparing base (d26797f) to head (9ad35df).

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4140       +/-   ##
=============================================
- Coverage   91.357%   91.348%   -0.009%     
=============================================
  Files          604       604               
  Lines        48179     48179               
  Branches     17355     17352        -3     
=============================================
- Hits         44015     44011        -4     
- Misses        4071      4075        +4     
  Partials        93        93               

see 3 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d26797f...9ad35df. Read the comment docs.

- also configure product name and bundle ID
- rename Debug_without_UIKit and Release_without_UIKit to remove the underscores and capitalize "without"
- add a separate modulemap file for SentryWithoutUIKit
fixes this error:
Module 'SentryWithoutUIKit' in AST file '/Users/andrewmcknight/Library/Developer/Xcode/DerivedData/ModuleCache.noindex/22JF8W0XJRMRE/SentryWithoutUIKit-1Z1Q6V1DCHSHF.pcm' (imported by AST file '/Users/andrewmcknight/Library/Developer/Xcode/DerivedData/ModuleCache.noindex/22JF8W0XJRMRE/_SentryPrivate-23Z4ZY3NE0LI4.pcm') is not defined in any loaded module map file; maybe you need to load '/Users/andrewmcknight/Library/Developer/Xcode/DerivedData/Sentry-gnnqqmcfqpqmmhcjxfzwadgjajnt/Build/Products/DebugWithoutUIKit-iphonesimulator/Sentry.framework/Modules/module.modulemap'?

we probably need to rename this from module.modulemap to
    SentryWithoutUIKitPrivate.modulemap and create the parallel
    SentryPrivate.modulemap and set those as MODULEMAP_PRIVATE_FILE
    per config
@aaronsky
Copy link

aaronsky commented Jul 2, 2024

Try this @armcknight

asky-1.patch

@armcknight armcknight force-pushed the armcknight/fix/module-names branch from c02b560 to 9128272 Compare July 2, 2024 19:35
the error starts out as such:

ERROR: /Users/andrewmcknight/Code/organization/getsentry/repos/public/sentry-cocoa-bazel/file_provider_extension/BUILD:4:14: Compiling Swift module //file_provider_extension:file_provider_swift_sources failed: (Exit 1): worker failed: error executing SwiftCompile command (from target //file_provider_extension:file_provider_swift_sources) bazel-out/darwin_arm64-opt-exec-ST-13d3ddad9198/bin/external/build_bazel_rules_swift/tools/worker/worker swiftc ... (remaining 1 argument skipped)
error: emit-module command failed with exit code 1 (use -v to see invocation)
<module-includes>:2:9: note: in file included from <module-includes>:2:
        ^
vendor/Carthage/Build/SentryWithoutUIKit.xcframework/ios-arm64_x86_64-simulator/SentryWithoutUIKit.framework/Headers/SentryWithoutUIKit-Swift.h:324:29: error: cannot find protocol declaration for 'SentrySerializable'
@protocol SentryRRWebEvent <SentrySerializable>
                            ^
<module-includes>:2:9: note: in file included from <module-includes>:2:
        ^
vendor/Carthage/Build/SentryWithoutUIKit.xcframework/ios-arm64_x86_64-simulator/SentryWithoutUIKit.framework/Headers/SentryWithoutUIKit-Swift.h:640:32: error: cannot find interface declaration for 'SentryEvent', superclass of 'SentryReplayEvent'
@interface SentryReplayEvent : SentryEvent
                               ^
<module-includes>:1:9: note: in file included from <module-includes>:1:
        ^
/private/var/tmp/_bazel_andrewmcknight/1473e8b1ae382f4a9a68831aec4e0587/execroot/_main/vendor/Carthage/Build/SentryWithoutUIKit.xcframework/ios-arm64_x86_64-simulator/SentryWithoutUIKit.framework/Headers/SentryWithoutUIKit.h:50:1: warning: umbrella header for module 'SentryWithoutUIKit' does not include header 'SentryMeasurementUnit.h'

^
<module-includes>:1:9: note: in file included from <module-includes>:1:
        ^
/private/var/tmp/_bazel_andrewmcknight/1473e8b1ae382f4a9a68831aec4e0587/execroot/_main/vendor/Carthage/Build/SentryWithoutUIKit.xcframework/ios-arm64_x86_64-simulator/SentryWithoutUIKit.framework/Headers/SentryWithoutUIKit.h:50:1: warning: umbrella header for module 'SentryWithoutUIKit' does not include header 'SentryNSError.h'
@armcknight armcknight marked this pull request as ready for review July 8, 2024 22:17
Copy link

github-actions bot commented Jul 9, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1241.47 ms 1258.00 ms 16.53 ms
Size 21.58 KiB 682.40 KiB 660.82 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
e681215 6240.94 ms 6254.36 ms 13.42 ms
a71f5e2 1239.92 ms 1250.67 ms 10.76 ms
51307b7 1223.08 ms 1240.76 ms 17.68 ms
984eb2d 1220.62 ms 1235.24 ms 14.62 ms
3437454 1254.04 ms 1259.50 ms 5.46 ms
becc941 1221.90 ms 1240.37 ms 18.47 ms
7bc3c0d 1212.35 ms 1228.94 ms 16.59 ms
7bb0873 1215.65 ms 1235.00 ms 19.35 ms
f587451 1271.63 ms 1275.90 ms 4.27 ms
6e31b7c 1230.80 ms 1246.29 ms 15.49 ms

App size

Revision Plain With Sentry Diff
e681215 20.76 KiB 431.91 KiB 411.15 KiB
a71f5e2 21.58 KiB 424.34 KiB 402.76 KiB
51307b7 22.85 KiB 407.63 KiB 384.78 KiB
984eb2d 20.76 KiB 425.77 KiB 405.01 KiB
3437454 22.85 KiB 408.87 KiB 386.02 KiB
becc941 21.58 KiB 419.82 KiB 398.24 KiB
7bc3c0d 20.76 KiB 427.35 KiB 406.59 KiB
7bb0873 22.85 KiB 407.09 KiB 384.24 KiB
f587451 20.76 KiB 435.25 KiB 414.49 KiB
6e31b7c 21.58 KiB 614.65 KiB 593.07 KiB

Previous results on branch: armcknight/fix/module-names

Startup times

Revision Plain With Sentry Diff
1d5b5f7 1239.52 ms 1255.79 ms 16.27 ms
e1f6a1a 1218.40 ms 1231.88 ms 13.48 ms

App size

Revision Plain With Sentry Diff
1d5b5f7 21.58 KiB 682.39 KiB 660.81 KiB
e1f6a1a 21.58 KiB 682.12 KiB 660.54 KiB

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @armcknight !!

All looks good.

@armcknight armcknight merged commit 5230990 into main Jul 9, 2024
66 of 67 checks passed
@armcknight armcknight deleted the armcknight/fix/module-names branch July 9, 2024 19:15
This was referenced Jul 9, 2024
@kahest kahest linked an issue Jul 10, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Separately named modules for Sentry with and without UIKit
3 participants