-
-
Notifications
You must be signed in to change notification settings - Fork 352
meta: correct version setting #5139
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: armcknight/ci/ios-swift-cleanup
Are you sure you want to change the base?
meta: correct version setting #5139
Conversation
…a/correct-version-setting
…a/correct-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## armcknight/ci/ios-swift-cleanup #5139 +/- ##
=====================================================================
+ Coverage 92.686% 92.785% +0.098%
=====================================================================
Files 677 677
Lines 84410 84414 +4
Branches 29616 30700 +1084
=====================================================================
+ Hits 78237 78324 +87
+ Misses 6076 5988 -88
- Partials 97 102 +5
... and 23 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
7c14190 | 1223.51 ms | 1246.22 ms | 22.71 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
7c14190 | 22.31 KiB | 852.06 KiB | 829.75 KiB |
Previous results on branch: armcknight/meta/correct-version-setting
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
19f26cd | 1225.63 ms | 1249.29 ms | 23.66 ms |
92918ce | 1234.24 ms | 1251.21 ms | 16.96 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
19f26cd | 22.31 KiB | 852.05 KiB | 829.75 KiB |
92918ce | 22.31 KiB | 852.06 KiB | 829.75 KiB |
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
…orrect-version-setting
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.
LGTM, I just want to point out that with this PR we are not only changing the sample projects but also configurations of the SDK (SDK.xcconfig
).
I don't see this as an issue, it just doesn't match with internal communication of this refactor not touching the SDK projects yet.
I would appreciate a second review by @philipphofmann on this PR please.
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 approach looks good, but I don't think we can touch the version numbers manually. Only the release workflow should do this.
I'm not too much worried about the change of CURRENT_PROJECT_VERSION
in Sources/Configuration/SDK.xcconfig
because as already pointed out by @armcknight it should be an integer of a float, which it wasn't. So it was already broken.
@@ -1,2 +1,2 @@ | |||
CURRENT_PROJECT_VERSION = 1 | |||
MARKETING_VERSION = 8.49.1 | |||
MARKETING_VERSION = 8.49.2 |
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.
h
: Something is off here. Here we have 8.49.2
and sometimes we have 8.49.3
, but we already released 8.50.0. All versions should already be 8.50.0
.
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 haven't merged from main
since 8.50.0 was released, and I did a find/replace on some of these I think, so I'll get the versions straightened out here and for the other ones you mentioned.
url: "https://github.com/getsentry/sentry-cocoa/releases/download/8.49.3/Sentry.xcframework.zip", | ||
checksum: "85cf16b4fa2dd0ae58b1e5c62fd8c829c94e5dc92251c3a1de8e71b3fed195ef" //Sentry-Static |
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.
h
: If the URL to the zip changes, the checksum must also change. Otherwise, we could break SPM, when compiling from the main branch. Anyways this should be 8.50.0, and we shouldn't update this manually. Only the release workflow should do that.
@@ -1,6 +1,6 @@ | |||
Pod::Spec.new do |s| | |||
s.name = "Sentry" | |||
s.version = "8.49.2" | |||
s.version = "8.49.3" |
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.
h
: We shouldn't have to update this manually. Only the GitHub release workflow should do that.
I noticed something about how we version some things while going through the XcodeGen refactor: we were setting
CURRENT_PROJECT_VERSION = 8.49.1
for the SentrySDK and SentrySwiftUI framework products. However, that setting is supposed to only be an integer value. The semantic version is supposed to go into MARKETING_VERSION.We also had a couple disparate places where we would write the semver. So, this unifies them all into a single source of truth, and uses the correct build settings.
#skip-changelog; not specifically for #5165, but is currently interleaved with its changes