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

Tool reports errors that are not user responsibility related to FFI #75429

Closed
dnfield opened this issue Feb 4, 2021 · 9 comments
Closed

Tool reports errors that are not user responsibility related to FFI #75429

dnfield opened this issue Feb 4, 2021 · 9 comments
Labels
a: error message Error messages from the Flutter framework dependency: dart Dart team may need to help us P0 Critical issues such as a build break or regression

Comments

@dnfield
Copy link
Contributor

dnfield commented Feb 4, 2021

Related to #75113

I talked a bit more with @dcharkes about this.

Package:ffi is getting updated, so other packages will be able to update. I'm still concerned that users will see a big wall of red text that they shouldn't when they depend on a non-upgraded package on the current release candidate.

For example, this program (using path_provider: 1.6.27, which in turn depends on win32 and ffi transitively):

import 'package:path_provider/path_provider.dart'

void main() {
  print('hello');
}

Will print out the string of warnings listed in #75113.

If the CFE reports these issues in a dependency, we should not print out the warnings, similar to how we do not print out deprecation warnings in other packages (we let the analyzer handle that).

/cc @dcharkes @jonahwilliams @christopherfujino

@dnfield dnfield added tool Affects the "flutter" command-line tool. See also t: labels. a: error message Error messages from the Flutter framework P0 Critical issues such as a build break or regression cp: 1.26 labels Feb 4, 2021
@dnfield
Copy link
Contributor Author

dnfield commented Feb 4, 2021

In particular, we want to avoid users who are going to upgrade to the new release candidate and depend on any of a number of popular packages that depend no package:ffi/win32/etc from seeing a wall of red text whenever they run/test their app.

@dcharkes is going to provide the specific hint type the CFE is emitting.

@dcharkes
Copy link
Contributor

dcharkes commented Feb 4, 2021

Analyzer codes for deprecation messages: EMPTY_STRUCT_WARNING, NON_CONSTANT_TYPE_ARGUMENT_WARNING.

CFE identifiers: FfiEmptyStructWarning, FfiNonConstantTypeArgumentWarning.

@dnfield dnfield added dependency: dart Dart team may need to help us and removed tool Affects the "flutter" command-line tool. See also t: labels. labels Feb 4, 2021
@dnfield
Copy link
Contributor Author

dnfield commented Feb 4, 2021

We talked offline some more - it'll probably be best to remove the hints from the CFE for this, @dcharkes is going to look into this

@pcsosinski pcsosinski added this to the January Beta Release (1.26) milestone Feb 4, 2021
dart-bot pushed a commit to dart-lang/sdk that referenced this issue Feb 5, 2021
The info severity level has only been used in one instance before, and
that could only occur in the compile command.

https://dart-review.googlesource.com/c/sdk/+/180361 and
https://dart-review.googlesource.com/c/sdk/+/180560 introduced the first
uses of this severity level outside the compile command.
This is the first time an `info` is emitted in the frontend_server.

We don't want to emit `info`s for the server. This is a quick solution
to fix that. The solution has been manually verified with a Flutter app
run with a local engine build with this change patched in.
Bug: flutter/flutter#75429

Keeping the diff as small as possible to ease cherry pick for release.

The better solution is to make the frontend_server's verbosity level
configurable.
Bug: #44867

Change-Id: Ie40dbb27621053e90bc8943b8cb7c5ec4c9ed643
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/183005
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>
@dcharkes
Copy link
Contributor

dcharkes commented Feb 5, 2021

Turns out my CLs were the first to ever emit info-severity messages from the frontend_server. We've landed a change to no longer print infos in the frontend server and filed a CP on Dart (dart-lang/sdk#44869).

Thanks @johnniwinther for the background info and quick review!

dart-bot pushed a commit to dart-lang/sdk that referenced this issue Feb 8, 2021
The info severity level has only been used in one instance before, and
that could only occur in the compile command.

https://dart-review.googlesource.com/c/sdk/+/180361 and
https://dart-review.googlesource.com/c/sdk/+/180560 introduced the first
uses of this severity level outside the compile command.
This is the first time an `info` is emitted in the frontend_server.

We don't want to emit `info`s for the server. This is a quick solution
to fix that. The solution has been manually verified with a Flutter app
run with a local engine build with this change patched in.
Bug: flutter/flutter#75429

Keeping the diff as small as possible to ease cherry pick for release.

The better solution is to make the frontend_server's verbosity level
configurable.
Bug: #44867

Change-Id: Ie40dbb27621053e90bc8943b8cb7c5ec4c9ed643
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/183005
Reviewed-by: Johnni Winther <johnniwinther@google.com>
Commit-Queue: Daco Harkes <dacoharkes@google.com>
@tvolkert
Copy link
Contributor

tvolkert commented Feb 8, 2021

@dcharkes is there anything still to do here, or are we just waiting for the CLs to roll through?

@dcharkes
Copy link
Contributor

dcharkes commented Feb 8, 2021

Do you need to do anything on your side to make the Dart beta roll into a new Flutter beta? If no, then we're done.

Dart beta with CP dart-lang/sdk@bc1fa17

@christopherfujino
Copy link
Member

@tvolkert @pcsosinski sounds like with regards to the RC branch, the next release will pick up the dart fix and resolve this issue.

@pcsosinski
Copy link

removing cp label as this will be picked up with the Dart CP

@github-actions
Copy link

github-actions bot commented Aug 6, 2021

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: error message Error messages from the Flutter framework dependency: dart Dart team may need to help us P0 Critical issues such as a build break or regression
Projects
None yet
Development

No branches or pull requests

6 participants