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

Bump macos deployment target; stop holding back firebase_core; run tools/upgrade pub #1184

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

chrisbobbe
Copy link
Collaborator

Fixes #1116.

@chrisbobbe chrisbobbe requested a review from gnprice December 19, 2024 22:30
@chrisbobbe
Copy link
Collaborator Author

chrisbobbe commented Dec 19, 2024

Is the CI failure a new flavor of #1177?

0s
Run flutter precache --universal
fatal: Not a valid object name origin/master
Error: Process completed with exit code [1](https://github.com/zulip/zulip-flutter/actions/runs/12421852640/job/34682388039?pr=1184#step:6:1)28.

@gnprice
Copy link
Member

gnprice commented Dec 19, 2024

Is the CI failure a new flavor of #1177?

Yeah, I think so.

@gnprice
Copy link
Member

gnprice commented Dec 19, 2024

(I'll send a fix for that)

@chrisbobbe
Copy link
Collaborator Author

chrisbobbe commented Dec 19, 2024

Thanks. I also saw this go by (thanks to #mobile-github > zulip-flutter!): d90c5a0#commitcomment-150583217

@gnprice
Copy link
Member

gnprice commented Dec 19, 2024

Thanks for taking care of this! These changes all look great.

Since we're upgrading firebase_messaging, and particularly as the underlying SDK seems to be upgraded by a major version (FirebaseMessaging (11.0.0) in the Podfile.lock), we should probably repeat our manual testing that notifications still work. (It'll be great to someday have automated tests for that: #757, #758.) Would you do that?

That testing can be batched with further upgrading Firebase to the latest: if you upgrade share_plus and share_plus_platform_interface, then all the Firebase packages can be upgraded to latest. No need to manually test on the intermediate upgraded versions, except in the unlikely case that things don't work after fully upgrading.

Xcode made the project.pbxproj changes when I changed the "macOS
Deployment Target" setting in the GUI from 10.14 to 10.15 and back
again. The Runner.xcscheme changes happened when I built the app to
run.

A quick test confirmed that 7649477 explains the change from
"zulip.app" to "Zulip.app" (I tried a different string "asdfjkl" and
saw that it propagated in the same way).

Not sure what explains removing the "name = Pods;" line, but there's
a "path = Pods;" line right under it, and anyway the app continues
to build.
…m ios/

We brought this to ios/Podfile from zulip-mobile in a7ed31e;
helpful to have it on the macOS side too.
Fixes zulip#1116.

Done by changing the line in the Podfile, and following the
(recently added) comment there by changing a setting in the Xcode
GUI.

The Podfile change seems to be the only thing missing when Greg
attempted this bump:

(from zulip#1116)
> Naturally we're happy to bump up our minimum macOS deployment
> target — 10.15 is from 2019, and even increasing to last year's
> macOS 14 would be perfectly fine. But I spent a few minutes just
> now attempting to do so — editing the `MACOSX_DEPLOYMENT_TARGET =
> 10.14;` lines in the `project.pbxproj` file so they say 10.15 (or
> 13.0) instead, then also tried setting it in Xcode which added
> some new lines of that form — and the error persisted. (Perhaps
> there's somewhere *else* that `pod update` is looking? Perhaps it
> has a cache of some kind that it fails to update?)
As Greg said in zulip#1116:

> 10.15 is from 2019, and even increasing to last year's macOS 14
> would be perfectly fine.
Done by editing pubspec.yaml and running `tools/upgrade pub` twice.

This should mean that all the Firebase packages are upgraded to latest:
  zulip#1184 (comment)

This seems to have unlocked upgrading `drift` too. Some followups
were needed:

- ran `tools/check drift --all --fix`

- ran `tools/check build_runner --all --fix`

- followed an instruction from the analyzer:

  info • 'package:drift_dev/api/migrations.dart' is deprecated and
    shouldn't be used. Import `package:drift/migrations_native.dart`
    instead • test/model/database_test.dart:4:1 •
    deprecated_member_use
@chrisbobbe chrisbobbe force-pushed the pr-macos-deployment-target-and-deps branch from 09c653e to f620229 Compare December 20, 2024 16:38
@chrisbobbe
Copy link
Collaborator Author

chrisbobbe commented Dec 20, 2024

Mm, good idea. OK to test on Android only, do you think, since on iOS it takes more setup? Then for iOS I can make sure to take a look in the new version from TestFlight when we publish that?

(Android device charging right now)

@chrisbobbe
Copy link
Collaborator Author

OK, and checked that notifications are still being presented on the office Android device running Android 9.

We'll plan to test iOS notifications when making the next beta release, because Apple makes it complicated to test client-side notification code in dev: https://github.com/zulip/zulip-mobile/blob/main/docs/howto/push-notifications.md#testing-client-side-changes-on-ios

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks! The changes all look good. Some comments/questions about the commit message on the last commit.

{"_meta":{"description":"This file contains a serialized version of schema entities for drift.","version":"1.2.0"},"options":{"store_date_time_values_as_text":false},"entities":[{"id":0,"references":[],"type":"table","data":{"name":"accounts","was_declared_in_moor":false,"columns":[{"name":"id","getter_name":"id","moor_type":"int","nullable":false,"customConstraints":null,"defaultConstraints":"PRIMARY KEY AUTOINCREMENT","dialectAwareDefaultConstraints":{"sqlite":"PRIMARY KEY AUTOINCREMENT"},"default_dart":null,"default_client_dart":null,"dsl_features":["auto-increment"]},{"name":"realm_url","getter_name":"realmUrl","moor_type":"string","nullable":false,"customConstraints":null,"default_dart":null,"default_client_dart":null,"dsl_features":[],"type_converter":{"dart_expr":"const UriConverter()","dart_type_name":"Uri"}},{"name":"user_id","getter_name":"userId","moor_type":"int","nullable":false,"customConstraints":null,"default_dart":null,"default_client_dart":null,"dsl_features":[]},{"name":"email","getter_name":"email","moor_type":"string","nullable":false,"customConstraints":null,"default_dart":null,"default_client_dart":null,"dsl_features":[]},{"name":"api_key","getter_name":"apiKey","moor_type":"string","nullable":false,"customConstraints":null,"default_dart":null,"default_client_dart":null,"dsl_features":[]},{"name":"zulip_version","getter_name":"zulipVersion","moor_type":"string","nullable":false,"customConstraints":null,"default_dart":null,"default_client_dart":null,"dsl_features":[]},{"name":"zulip_merge_base","getter_name":"zulipMergeBase","moor_type":"string","nullable":true,"customConstraints":null,"default_dart":null,"default_client_dart":null,"dsl_features":[]},{"name":"zulip_feature_level","getter_name":"zulipFeatureLevel","moor_type":"int","nullable":false,"customConstraints":null,"default_dart":null,"default_client_dart":null,"dsl_features":[]},{"name":"acked_push_token","getter_name":"ackedPushToken","moor_type":"string","nullable":true,"customConstraints":null,"default_dart":null,"default_client_dart":null,"dsl_features":[]}],"is_virtual":false,"without_rowid":false,"constraints":[],"unique_keys":[["realm_url","user_id"],["realm_url","email"]]}}]}
Copy link
Member

Choose a reason for hiding this comment

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

Huh, a bit awkward that this changes, as its purpose is to be a historical record of how things were when we introduced v2 of the schema.

So I got a diff of what changed:

$ diff -U1 \
    <(git show @~:test/model/schemas/drift_schema_v2.json | jq .) \
    <(<test/model/schemas/drift_schema_v2.json jq .)
--- /dev/fd/63	2024-12-20 14:31:03.565983750 -0800
+++ /dev/fd/62	2024-12-20 14:31:03.565983750 -0800
@@ -3,3 +3,3 @@
     "description": "This file contains a serialized version of schema entities for drift.",
-    "version": "1.1.0"
+    "version": "1.2.0"
   },
@@ -24,2 +24,5 @@
             "defaultConstraints": "PRIMARY KEY AUTOINCREMENT",
+            "dialectAwareDefaultConstraints": {
+              "sqlite": "PRIMARY KEY AUTOINCREMENT"
+            },
             "default_dart": null,

Seems like no substantive change, so it's fine.

Comment on lines +1 to 4
// dart format width=80
// GENERATED CODE, DO NOT EDIT BY HAND.
// ignore_for_file: type=lint
//@dart=2.12
import 'package:drift/drift.dart';
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, awkward that these schema_vN.dart files change, but again the change is a no-op (and this time just git log -p is enough to show that).

Comment on lines -57 to +58
share_plus: ^9.0.0
share_plus_platform_interface: ^4.0.0
share_plus: ^10.0.0
share_plus_platform_interface: ^5.0.0
Copy link
Member

Choose a reason for hiding this comment

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

Done by editing pubspec.yaml and running `tools/upgrade pub` twice.

Huh, why'd you have to run tools/upgrade twice? I'd expect it to be idempotent — for the first time to go all the way that a second time would take you. (Modulo new package versions getting uploaded to the package repo in between.)

Comment on lines -393 to 394
version: "15.0.4"
version: "15.1.6"
firebase_messaging_platform_interface:
Copy link
Member

Choose a reason for hiding this comment

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

This should mean that all the Firebase packages are upgraded to latest:
  https://github.com/zulip/zulip-flutter/pull/1184#issuecomment-2555964673

You can confirm this yourself using flutter pub outdated. Here's the output I see at the tip of the branch:

$ flutter pub outdated
Showing outdated packages.
[*] indicates versions that are not the latest available.

Package Name              Current              Upgradable           Resolvable           Latest   

direct dependencies:     
collection                *1.19.0              *1.19.0              *1.19.0              1.19.1   
device_info_plus          *10.1.2              *10.1.2              11.2.0               11.2.0   
flutter_color_models      *1.4.0 (overridden)  *1.4.0 (overridden)  *1.4.0 (overridden)  1.3.3+2  
intl                      *0.19.0              *0.19.0              *0.19.0              0.20.1   
mime                      *1.0.6               *1.0.6               2.0.0                2.0.0    
wakelock_plus             *1.2.8               1.2.9                1.2.9                1.2.9    

dev_dependencies:        
flutter_lints             *4.0.0               *4.0.0               5.0.0                5.0.0    
json_serializable         *6.9.0               *6.9.0               *6.9.0               6.9.2    
pigeon                    *20.0.2              *20.0.2              22.7.0               22.7.0   
stack_trace               *1.12.0              *1.12.0              *1.12.0              1.12.1   

transitive dependencies: 
characters                *1.3.0               *1.3.0               *1.3.0               1.4.0    
matcher                   *0.12.16+1           *0.12.16+1           *0.12.16+1           0.12.17  
material_color_utilities  *0.11.1              *0.11.1              *0.11.1              0.12.0   
meta                      *1.15.0              *1.15.0              *1.15.0              1.16.0   
stream_channel            *2.1.2               *2.1.2               *2.1.2               2.1.3    
string_scanner            *1.4.0               *1.4.0               *1.4.0               1.4.1    
term_glyph                *1.2.1               *1.2.1               *1.2.1               1.2.2    
vm_service                *14.3.1              *14.3.1              *14.3.1              15.0.0   
win32_registry            *1.1.5               *1.1.5               *1.1.5               2.0.1    

transitive dev_dependencies:
_fe_analyzer_shared       *76.0.0              *76.0.0              *76.0.0              78.0.0   
analyzer                  *6.11.0              *6.11.0              *6.11.0              7.1.0    
analyzer_plugin           *0.11.3              *0.11.3              *0.11.3              0.12.0   
dart_style                *2.3.7               *2.3.7               *2.3.7               3.0.1    
lints                     *4.0.0               *4.0.0               5.1.1                5.1.1    
pubspec_parse             *1.3.0               1.4.0                1.4.0                1.4.0    
source_gen                *1.5.0               *1.5.0               *1.5.0               2.0.0    
yaml                      *3.1.2               3.1.3                3.1.3                3.1.3    

3 upgradable dependencies are locked (in pubspec.lock) to older versions.
To update these dependencies, use `flutter pub upgrade`.

5  dependencies are constrained to versions that are older than a resolvable version.
To update these dependencies, edit pubspec.yaml, or run `flutter pub upgrade --major-versions`.

I think most of those are probably from the Flutter version being a couple of weeks old. In any case, no Firebase packages in the list.

In the terminal it comes in helpful color, too. Here's a screenshot:
image

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.

Can't upgrade firebase_core to 3.4.0, because macOS deployment target
2 participants