Skip to content

Conversation

blasten
Copy link

@blasten blasten commented Nov 3, 2020

Re-attempts #69238

@blasten blasten added a: tests "flutter test", flutter_test, or one of our tests a: null-safety Support for Dart's null safety feature labels Nov 3, 2020
@blasten blasten requested a review from jonahwilliams November 3, 2020 00:59
@flutter-dashboard flutter-dashboard bot added c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. labels Nov 3, 2020
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@google-cla google-cla bot added the cla: yes label Nov 3, 2020
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

Still LGTM

@blasten
Copy link
Author

blasten commented Nov 3, 2020

same issue :/ cc @dnfield - should we disable this test?

@jonahwilliams
Copy link
Contributor

@blasten is this the same branch? Or did you re-branch from a later commit?

Only the former will change your flutter version

@blasten
Copy link
Author

blasten commented Nov 3, 2020

New branch but with same commits from the previous one. Would that be the issue?

@jonahwilliams
Copy link
Contributor

I don't think so

@jonahwilliams
Copy link
Contributor

Could flutter_svg loosen the constraint a bit? @dnfield

@blasten
Copy link
Author

blasten commented Nov 3, 2020

what about we don't block this PR on the flutter_svg issue?

@dnfield
Copy link
Contributor

dnfield commented Nov 3, 2020

We should be able to retag flutter SVG with a dev version that this won't fail on. I don't think I'll get to that before tomorrow but if anyone else does I can help land

@blasten
Copy link
Author

blasten commented Nov 3, 2020

@mehmetf are we allowing null-safety plugins internally? I might need to do that shortly, see "Google testing".

@blasten
Copy link
Author

blasten commented Nov 3, 2020

Will import the plugin into g3 in a bit.

@blasten blasten merged commit 2129674 into flutter:master Nov 3, 2020
@blasten blasten deleted the nnbd_plugins_retry branch November 3, 2020 20:03
@gspencergoog
Copy link
Contributor

@blasten I think this has/will break the build. When I sync, I get multiple analysis errors, and it also looks like several tests were failing when this was merged.

Here are the analysis errors I get on master:

  error • Target of URI doesn't exist: 'package:connectivity_for_web/connectivity_for_web.dart' •
         dev/integration_tests/flutter_gallery/lib/generated_plugin_registrant.dart:8:8 • uri_does_not_exist
  error • Target of URI doesn't exist: 'package:url_launcher_web/url_launcher_web.dart' •
         dev/integration_tests/flutter_gallery/lib/generated_plugin_registrant.dart:9:8 • uri_does_not_exist
  error • Undefined name 'ConnectivityPlugin' • dev/integration_tests/flutter_gallery/lib/generated_plugin_registrant.dart:16:3 •
         undefined_identifier
  error • The argument type 'dynamic' can't be assigned to the parameter type 'Type' •
         dev/integration_tests/flutter_gallery/lib/generated_plugin_registrant.dart:16:57 • argument_type_not_assignable
  error • Undefined name 'ConnectivityPlugin' • dev/integration_tests/flutter_gallery/lib/generated_plugin_registrant.dart:16:57 •
         undefined_identifier
  error • Undefined name 'UrlLauncherPlugin' • dev/integration_tests/flutter_gallery/lib/generated_plugin_registrant.dart:17:3 •
         undefined_identifier
  error • The argument type 'dynamic' can't be assigned to the parameter type 'Type' •
         dev/integration_tests/flutter_gallery/lib/generated_plugin_registrant.dart:17:56 • argument_type_not_assignable
  error • Undefined name 'UrlLauncherPlugin' • dev/integration_tests/flutter_gallery/lib/generated_plugin_registrant.dart:17:56 •
         undefined_identifier

@gspencergoog
Copy link
Contributor

Nevermind: this was just cruft left in my repo, after cleaning I get no analysis errors.

But why was this committed with failing tests?

@blasten
Copy link
Author

blasten commented Nov 3, 2020

@gspencergoog the errors are unrelated, and due to dnfield/flutter_svg#435. The g3 test is being fixed internally.

@@ -64,7 +63,11 @@ const Map<String, String> _kManuallyPinnedDependencies = <String, String>{
// https://github.com/dart-lang/build/issues/2772
'build_runner_core': '5.2.0',
'build_modules': '2.10.1',
'path_provider': '1.6.14'
'path_provider': '1.6.14',
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we pinning path_provider?
(just asking because i'm adding comments to update_packages.dart)

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure why path_provider was pinned. maybe @jonahwilliams ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: null-safety Support for Dart's null safety feature a: tests "flutter test", flutter_test, or one of our tests c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants