ci: Add fatal-infos flag to dart and flutter analyze

[Nidal-Bakir]

Pull Request

• Report security issues confidentially.
• Any contribution is under this license.
• Link this pull request to an issue.

Issue

Closes: #905

Approach

see fatal-infos docs

[parse-github-assistant]

Thanks for opening this pull request!

[Nidal-Bakir]

Flutter SDK

Run flutter analyze packages/flutter --fatal-infos
Analyzing flutter...

info • Use 'const' with the constructor to improve performance • packages/flutter/example/lib/live_list/main.dart:52:23 • prefer_const_constructors
info • Use 'const' literals as arguments to constructors of '@immutable' classes • packages/flutter/example/lib/live_list/main.dart:54:35 • prefer_const_literals_to_create_immutables
info • Use 'const' with the constructor to improve performance • packages/flutter/example/lib/live_list/main.dart:55:27 • prefer_const_constructors
info • Use 'const' with the constructor to improve performance • packages/flutter/example/lib/live_list/main.dart:56:27 • prefer_const_constructors

4 issues found. (ran in 15.9s)

---

Dart SDK

Run dart analyze packages/dart --fatal-infos
Analyzing dart...

info - example\main.dart:1:8 - Can't use a relative path to import a library in 'lib'. Try fixing the relative path or changing the import to a 'package:' import. - avoid_relative_lib_imports

---

No good. We need to fix this. No wonder the publishing is failing

[mtrezza]

But these don't seem to contain the error that was logged when publishing failed?

[Nidal-Bakir]

I updated the comment.

If no one opens a pull request to fix this, I'll fix it tomorrow. I have an exam in the morning, so I'm currently studying.

[mtrezza]

Great that you found that out and analyzed this so far! I think this is one of the last hurdles since we've been revamping our CI, so we're on a good path. Maybe @mbfakourii has time to take a look as well and give a hand.

[mbfakourii]

Flutter SDK

Run flutter analyze packages/flutter --fatal-infos
Analyzing flutter...
info • Use 'const' with the constructor to improve performance • packages/flutter/example/lib/live_list/main.dart:52:23 • prefer_const_constructors
info • Use 'const' literals as arguments to constructors of '@immutable' classes • packages/flutter/example/lib/live_list/main.dart:54:35 • prefer_const_literals_to_create_immutables
info • Use 'const' with the constructor to improve performance • packages/flutter/example/lib/live_list/main.dart:55:27 • prefer_const_constructors
info • Use 'const' with the constructor to improve performance • packages/flutter/example/lib/live_list/main.dart:56:27 • prefer_const_constructors
4 issues found. (ran in 15.9s)

Dart SDK

Run dart analyze packages/dart --fatal-infos
Analyzing dart...
info - example\main.dart:1:8 - Can't use a relative path to import a library in 'lib'. Try fixing the relative path or changing the import to a 'package:' import. - avoid_relative_lib_imports

No good. We need to fix this. No wonder the publishing is failing

@Nidal-Bakir Have a good exam ❤️❤️

Regarding the first warnings, I fixed these bugs in the PR, but there was a problem on Flutter 3.3.x and 3.7.x, that's why it was left like this to be supported in this version.

Regarding the second warning, it is better to use it in this way because it is linked to the library in a local form

These are all warnings, I don't think they are the reason for not releasing the version

[mtrezza]

These are all warnings, I don't think they are the reason for not releasing the version

I've added the missing "Issue" reference to the PR description. As you can see there, publishing is failing because we are running the analzyer as part of the release workflow with dart analyze --fatal-infos. Do you mean we should remove the fatal-info and always publish releases regardless of whether they have errors or warnings on the info level?

[mbfakourii]

These are all warnings, I don't think they are the reason for not releasing the version

I've added the missing "Issue" reference to the PR description. As you can see there, publishing is failing because we are running the analzyer as part of the release workflow with dart analyze --fatal-infos. Do you mean we should remove the fatal-info and always publish releases regardless of whether they have errors or warnings on the info level?

No, in my opinion, warnings should be completely resolved

In this PR, I tried to solve the second warning, probably now we don't have any more publishing problems

[mtrezza]

Should such info level warnings prevent a PR from being merged and/or a release from being published? Or can they be ignored for PRs and releases and be solved over time?

My concern is that if we don't make them fatal in the CI (what this PR intends) we may miss minor issues when reviewing PRs. But if the issues logged on the info level won't cause any trouble and can be ignored, we may as well not add it to the CI and remove it from the publishing workflow. What do you suggest?

[mbfakourii]

Should such info level warnings prevent a PR from being merged and/or a release from being published? Or can they be ignored for PRs and releases and be solved over time?

My concern is that if we don't make them fatal in the CI (what this PR intends) we may miss minor issues when reviewing PRs. But if the issues logged on the info level won't cause any trouble and can be ignored, we may as well not add it to the CI and remove it from the publishing workflow. What do you suggest?

In my opinion, we should find out and solve these issues in ci, but these warnings do not cause any problems during the release

[mtrezza]

We can either add the flag or remove it from both, CI and CD. If the flag is removed, anyone is free to look into the CI / CD logs and fix the warnings if they like, but they won't be considered (or even looked at) when merging a PR or publishing a release.

[Nidal-Bakir]

These errors are not critical and will not cause any harm to the app. However, it would be beneficial to fix them since they are relatively easy to address, such as using the const keyword appropriately. Additionally, it is preferable to follow good coding practices. Overall, fixing these issues would be advantageous for the app's long-term performance and stability.

[mtrezza]

This issue is currently blocking the release pipeline. If there is no contrary opinion I'll remove the flag from all workflows since these are only warnings and merge this PR.

[Nidal-Bakir]

I will fix them all give me 30 min

[Nidal-Bakir]

#908
Should fix the Dart SDK I just test it. You can merge it and publish the Dart 5.1.1

#909
Should fix the Flutter info errors

Before merging either of those two PRs (#909, #908), please merge this PR first to ensure that the two PRs are actually fixing the errors. This will allow us to run the updated CI against them.

[mtrezza]

So are we sure that we want these warnings to block the merging of PRs and publishing of releases in the future? That decision is separate from fixing the warnings; they can always be fixed even without blocking PRs and releases.

[Nidal-Bakir]

So are we sure that we want these warnings to block the merging of PRs

Yes

and publishing of releases in the future?

If the PR has no warnings or errors, it should not block the publishing process, and the release process should proceed smoothly. The point is if the PR in the first place does not continue any errors we should not counter any errors in the publishing process.

[mtrezza]

We don't have a lock file for dependencies, so a PR may show no warnings but the publishing process may indeed, if a dependency has a new release between PR merging and release publishing. See #749.

That may be a low risk we can take. I'm more concerned that these are warnings on the info level. Do we want info level logs to block? Not sure how that is in dart, but in other frameworks it may he too ambiguous to always fix every info warning immediately. I leave this up for you to decide, as you are likely more familiar with what type of warnings these are.

In any case, we may well be ambiguous and merge this PR as is to see how it goes.

[Nidal-Bakir]

This has nothing to do with the dependencies and the lock file. it's about the actual code in the SDK. Plus we are testing on all Flutter and Dart versions

[mtrezza]

I assumed the warnings would include any warnings that are caused by dependencies as well. Alright, let's merge this.

[mtrezza]

Looks good!