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

deps: Update Flutter and packages #316

Merged
merged 2 commits into from
Oct 11, 2023
Merged

deps: Update Flutter and packages #316

merged 2 commits into from
Oct 11, 2023

Conversation

chrisbobbe
Copy link
Collaborator

No description provided.

@chrisbobbe
Copy link
Collaborator Author

CI failures. Very interesting! Glad to have #314 in, to help us detect what are probably real things to fix here. 🙂

output
Run TERM=dumb tools/check --all --verbose
Test command: tools/check --all --verbose
Time now: 2023-10-06 23:03:53 +0000
zulip-flutter a95ad12fb • 2023-10-06 23:02:35 +0000
flutter/flutter 39b9d8fd6 • 2023-10-06 22:28:08 +0000
Dart SDK version: 3.2.0-231.0.dev (dev) (Wed Oct [4](https://github.com/zulip/zulip-flutter/actions/runs/6437255497/job/17482093710?pr=316#step:6:5) 13:04:13 2023 -0700) on "linux_x64"
================================================================
Running analyze...
Analyzing zulip-flutter...                                      
No issues found! (ran in 2[5](https://github.com/zulip/zulip-flutter/actions/runs/6437255497/job/17482093710?pr=316#step:6:6).8s)
================================================================
Running test...

[tons of output; trimmed out for brevity]

🎉 424 tests passed, 1 skipped.
================================================================
Running build_runner...
[INFO] Generating build script...
[INFO] Generating build script completed, took 661ms

[INFO] Precompiling build script......
[INFO] Precompiling build script... completed, took 15.0s

[INFO] Initializing inputs
[INFO] Building new asset graph...
[INFO] Building new asset graph completed, took 2.3s

[INFO] Checking for unexpected pre-existing outputs....
[INFO] Deleting 10 declared outputs which already existed on disk.
[INFO] Checking for unexpected pre-existing outputs. completed, took 6ms

[INFO] Running build...
[INFO] Generating SDK summary...
[INFO] 3.9s elapsed, 0/16 actions completed.
[INFO] [7](https://github.com/zulip/zulip-flutter/actions/runs/6437255497/job/17482093710?pr=316#step:6:8).1s elapsed, 0/16 actions completed.
[INFO] [8](https://github.com/zulip/zulip-flutter/actions/runs/6437255497/job/17482093710?pr=316#step:6:9).3s elapsed, 0/16 actions completed.
[INFO] Generating SDK summary completed, took 8.3s

[INFO] [9](https://github.com/zulip/zulip-flutter/actions/runs/6437255497/job/17482093710?pr=316#step:6:10).4s elapsed, 11/47 actions completed.
[INFO] 10.7s elapsed, 16/52 actions completed.
[INFO] 11.9s elapsed, 30/60 actions completed.
[INFO] 12.9s elapsed, 44/68 actions completed.
[INFO] 13.9s elapsed, 48/73 actions completed.
[INFO] 15.5s elapsed, 56/85 actions completed.
[INFO] 18.6s elapsed, 62/88 actions completed.
[INFO] 24.0s elapsed, 67/90 actions completed.
[INFO] 25.1s elapsed, 77/95 actions completed.
[INFO] 32.8s elapsed, 84/100 actions completed.
[INFO] 33.9s elapsed, 99/114 actions completed.
[INFO] 35.2s elapsed, 130/150 actions completed.
[INFO] 36.2s elapsed, 159/176 actions completed.
[INFO] 37.7s elapsed, 171/190 actions completed.
[INFO] 38.8s elapsed, 193/203 actions completed.
[INFO] 39.8s elapsed, 319/335 actions completed.
[INFO] Running build completed, took 40.7s

[INFO] Caching finalized dependency graph...
[INFO] Caching finalized dependency graph completed, took 289ms

[INFO] Succeeded after 41.0s with 226 outputs (618 actions)

Error: there were updates to *.g.dart files:
 M lib/model/database.g.dart
================================================================
Running drift...
Wrote to test/model/schemas/drift_schema_v2.json
Wrote 3 files into test/model/schemas
Error: there were schema updates:
 M test/model/schemas/drift_schema_v2.json
 M test/model/schemas/schema_v1.dart
 M test/model/schemas/schema_v2.dart
================================================================
Running icons...

added [16](https://github.com/zulip/zulip-flutter/actions/runs/6437255497/job/17482093710?pr=316#step:6:17)2 packages in [36](https://github.com/zulip/zulip-flutter/actions/runs/6437255497/job/17482093710?pr=316#step:6:37)s
================================================================
Running shellcheck...
ShellCheck - shell script analysis tool
version: 0.8.0
license: GNU General Public License, version 3
website: https://www.shellcheck.net
================================================================

FAILED: build_runner drift

To rerun the suites that failed, run:
  $ tools/check build_runner drift
Error: Process completed with exit code 1.

@gnprice
Copy link
Member

gnprice commented Oct 6, 2023

Cool!

Locally, tools/check --fix build_runner drift should be a convenient way to get those changes.

@gnprice
Copy link
Member

gnprice commented Oct 6, 2023

Also we should probably have tools/check --verbose print the diffs when there are diffs, instead of the one-line-per-file status. That way we'd have the diffs in the CI output.

@chrisbobbe
Copy link
Collaborator Author

chrisbobbe commented Oct 6, 2023

Locally, tools/check --fix build_runner drift should be a convenient way to get those changes.

Cool, I'll try that. The one thing I tried before that is:

$ dart run build_runner build --delete-conflicting-outputs

and that made these changes:

diff --git lib/model/database.g.dart lib/model/database.g.dart
index a7964f85a..b7f008675 100644
--- lib/model/database.g.dart
+++ lib/model/database.g.dart
@@ -78,9 +78,10 @@ class $AccountsTable extends Accounts with TableInfo<$AccountsTable, Account> {
         ackedPushToken
       ];
   @override
-  String get aliasedName => _alias ?? 'accounts';
+  String get aliasedName => _alias ?? actualTableName;
   @override
-  String get actualTableName => 'accounts';
+  String get actualTableName => $name;
+  static const String $name = 'accounts';
   @override
   VerificationContext validateIntegrity(Insertable<Account> instance,
       {bool isInserting = false}) {

I'll reset and try that command with --fix which does sound nice and convenient.

@gnprice
Copy link
Member

gnprice commented Oct 6, 2023

Cool. Yeah, I expect that's the same thing that the --fix will do ­— the latter is just a handy wrapper.

@chrisbobbe
Copy link
Collaborator Author

chrisbobbe commented Oct 6, 2023

Oh hmm, I think my run of build_runner above did something to a cache, and now when I run it (or that command with --fix) the changes it made the first time aren't getting applied. I've run into things like this before.

@gnprice
Copy link
Member

gnprice commented Oct 6, 2023

Oh. Yeah, I ran into that too when testing tools/check. Seems like build_runner doesn't notice when an output file was modified out of band, which is unfortunate.

One workaround is to change the input file, rerun, then change it back and rerun.

A mitigating factor is that I think this sequence isn't likely outside of scenarios like testing the different workflows, as you're doing and I was doing. Still seems like not great behavior for build_runner to have, though.

@chrisbobbe
Copy link
Collaborator Author

Cool. Yeah, I recovered by running dart run build_runner clean && dart run build_runner build but sounds like your solution would take less time.

Pushed a revision, but I don't yet understand the changes to the generated files and I think I should find out and write about it in the commit message :)

@chrisbobbe chrisbobbe marked this pull request as draft October 6, 2023 23:37
@chrisbobbe
Copy link
Collaborator Author

I don't yet understand the changes to the generated files and I think I should find out and write about it in the commit message :)

Hmm, yeah, I don't get it yet. At least the changes to the Dart files look NFC.

With an online JSON diff checker I found that just one part of test/model/schemas/drift_schema_v2.json changed: the JSON object's _meta.version changed from 1.0.0 to 1.1.0. But why that change, and why not a similar one in test/model/schemas/drift_schema_v1.json?

@gnprice
Copy link
Member

gnprice commented Oct 10, 2023

Here's a command line for that diff BTW:

$ diff -u <(git show @~:test/model/schemas/drift_schema_v2.json | jq .) \
    <(<test/model/schemas/drift_schema_v2.json jq .)
--- /dev/fd/63	2023-10-10 11:21:58.358941948 -0700
+++ /dev/fd/62	2023-10-10 11:21:58.358941948 -0700
@@ -1,7 +1,7 @@
 {
   "_meta": {
     "description": "This file contains a serialized version of schema entities for drift.",
-    "version": "1.0.0"
+    "version": "1.1.0"
   },
   "options": {
     "store_date_time_values_as_text": false

But why that change,

The change is a bit puzzling; I'd have expected it to be mentioned in the release notes:
https://github.com/simolus3/drift/releases/tag/drift-2.12.0

But presumably its omission there is just an oversight, and the change is because the new version of Drift changed the format of these schema files.

and why not a similar one in test/model/schemas/drift_schema_v1.json?

This one I think I do understand: it's because the code generator only generates the latest version of the schema. Which is because that's the only version it can generate, because that's the one it has the source code for, in lib/model/database.dart. The purpose of these schema JSON files (and the neighboring Dart files that are in turn generated from them) is to be a record of the old schema versions that we've deployed, and so want to test our migrations from, but aren't otherwise reflected in the current code.

This does suggest that strictly there's no need to update drift_schema_v2.json either — its purpose is to be a historical record from when we deployed schema v2, and it already is that. But it is valuable that we check that that record is accurate, when first introducing any change. And it seems like the most practical way to do that is to just regenerate the file and check it matches, which in turn means we need to update it when the generator changes like this.

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Oct 10, 2023
There are currently no upgrades that `--major-versions` would add
beyond these.

This includes the results of doing a build for iOS and for macOS;
the iOS Podfile.lock takes an update.

For the changes in lib/ and test/, see Greg's analysis here:
  zulip#316 (comment)
@chrisbobbe chrisbobbe marked this pull request as ready for review October 10, 2023 19:29
@chrisbobbe
Copy link
Collaborator Author

Thanks! Revision pushed, with the commit message pointing to your comment above: #316 (comment)

And update Flutter's supporting libraries to match.
There are currently no upgrades that `--major-versions` would add
beyond these.

This includes the results of doing a build for iOS and for macOS;
the iOS Podfile.lock takes an update.

For the changes in lib/ and test/, see Greg's analysis here:
  zulip#316 (comment)
@gnprice
Copy link
Member

gnprice commented Oct 11, 2023

Thanks! Looks good; merging.

@gnprice gnprice merged commit 5dbf1e6 into zulip:main Oct 11, 2023
1 check passed
@chrisbobbe chrisbobbe deleted the pr-deps branch October 11, 2023 22:56
@gnprice gnprice mentioned this pull request Oct 13, 2023
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.

2 participants