Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[path_provider, video_player] Tweak pubspecs to pass bulk analysis. #2959

Merged
merged 1 commit into from
Aug 24, 2020
Merged

[path_provider, video_player] Tweak pubspecs to pass bulk analysis. #2959

merged 1 commit into from
Aug 24, 2020

Conversation

ditman
Copy link
Member

@ditman ditman commented Aug 24, 2020

Description

It seems the ./script/incremental_build.sh analyze script is failing to analyze some random packages, and failing CI.

It seems pub get is not downloading the latest versions of some dependencies, and this is causing analyzer problems (missing classes, etc).

This change fixes a couple of pubspec.yaml files to ensure pub is downloading the appropriate packages for the packages I saw failing on my machine (path_provider_macos/example and video_player).

Related Issues

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • No, this is not a breaking change.

@ditman ditman requested a review from cyanglaz August 24, 2020 18:10
cyanglaz
cyanglaz approved these changes Aug 24, 2020
View reviewed changes
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

LGTM on video_player. Ping @stuartmorgan for the macos change.
Not sure why "any" wouldn't pass CI tho.

@ditman
Copy link
Member Author

ditman commented Aug 24, 2020 •
edited
Loading

My theory is that on a previous pub get on the analyze script, it grabbed a version of the path_provider that is "too old" for the macos example, and the any dependency is satisfied by that being on the cache.

PS: My pub knowledge is insufficient to back any of my claims above, though :)

@ditman ditman requested a review from stuartmorgan-g August 24, 2020 18:58
stuartmorgan-g
stuartmorgan-g approved these changes Aug 24, 2020
View reviewed changes
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

I thought we needed any because of some detail of the complicated tangle of circular dependencies, to avoid conflicting with something else. But if it passes tests, LGTM.

@ditman
Copy link
Member Author

ditman commented Aug 24, 2020

@stuartmorgan yes, without "any" and specifying the _macos dependency by path it fails (because in the versioned dependency of the plugin, the _macos dependency is set with a version).

Using dependency_overrides is another way of fixing this issue with the desired result, I think. I'll merge this, but please, let me know if there are any problems with _macos development/example later!

@ditman
Copy link
Member Author

ditman commented Aug 24, 2020

Merging on red to see if the analyze check in master gets better.

@ditman ditman merged commit e0a49a7 into flutter:master Aug 24, 2020
@ditman
Copy link
Member Author

ditman commented Aug 25, 2020

Looks like this made the tree green, yay!

@ditman ditman deleted the fix-analysis branch August 25, 2020 22:03
KevinTheGray added a commit to KevinTheGray/plugins that referenced this pull request Aug 31, 2020
@KevinTheGray
Merge branch 'master' into kg-connectivity-ios-permissions
c2732ba 
* master:
  [url_launcher, url_launcher_web]Added webOnlyWindowName parameter to launch()  (flutter#2979)
  [battery_platform_interface] Introduce package. (flutter#2975)
  [url_launcher_platform_interface] Add webOnlyWindowName parameter to launch() (flutter#2974)
  [share] Add sharing file support (android & ios) (flutter#970)
  [integration_test] add support to get timeline (flutter#2947)
  [battery] Moved battery to its own subdir (flutter#2969)
  [integration_test] Fix channel name for iOS, docs (flutter#2972)
  [integration_test] Show stack trace of widget test errors on the platform side (flutter#2967)
  [video_player] Fix `setWithOthers` test (flutter#2970)
  [video_player_platform_interface] fix `setMixIWithOthers` test channel (flutter#2968)
  Add README instructions for enablePendingPurchases (flutter#2921)
  Rebase previous commits (flutter#2966)
  Avoid walleye 26
  Updating documentation to use isEmpty check. (flutter#2955)
  [integration_test] Minor cleanup (flutter#2956)
  [path_provider, video_player] Tweak pubspecs to pass bulk analysis. (flutter#2959)
  [android_intent] fix platform package dep (flutter#2948)
  [shared_preferences_linux] update `file` package dep version to be compatible with framework (flutter#2950)
jorgefspereira pushed a commit to jorgefspereira/plugins_flutter that referenced this pull request Oct 10, 2020
@ditman @jorgefspereira
[path_provider, video_player] Tweak pubspecs to pass bulk analysis. (f…
aa5ea75 
…lutter#2959)
FlutterSu pushed a commit to FlutterSu/flutter-plugins that referenced this pull request Nov 20, 2020
@ditman
[path_provider, video_player] Tweak pubspecs to pass bulk analysis. (f…
d996cf1 
…lutter#2959)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stuartmorgan-g stuartmorgan-g stuartmorgan-g approved these changes

@cyanglaz cyanglaz cyanglaz approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ditman @stuartmorgan-g @cyanglaz @googlebot