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

Deprecate --packages flag and add --package #370

Merged
merged 12 commits into from
Mar 29, 2022
Merged

Conversation

liamappelbe
Copy link
Contributor

@liamappelbe liamappelbe commented Mar 10, 2022

Add a --package flag to format_coverage, which takes the package's root directory, instead of the .package file. This flag defaults to the current working directory, so in the common case the user won't have to pass it at all. Deprecate the --packages flag.

Also, deprecate the packagesPath parameter and add packagePath instead, in HitMap.parseJson, HitMap.parseFiles, createHitmap, and parseCoverage.

Fixes #369

Copy link
Member

@natebosch natebosch left a comment

Choose a reason for hiding this comment

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

In some places we've kept the --packages argument and use it to point to package_config.json instead of .packages.

https://pub.dev/documentation/package_config/latest/package_config.package_config/findPackageConfigUri.html

@liamappelbe
Copy link
Contributor Author

In some places we've kept the --packages argument and use it to point to package_config.json instead of .packages.

Michael thinks we should delete the flag. We can discuss it more here: #369

@liamappelbe liamappelbe marked this pull request as ready for review March 22, 2022 17:04
@coveralls
Copy link

coveralls commented Mar 22, 2022

Coverage Status

Coverage increased (+2.5%) to 93.05% when pulling 6143820 on package_flag into 0790b22 on master.

@liamappelbe liamappelbe changed the title Swap --packages flag for --package Deprecate --packages flag and add --package Mar 22, 2022
README.md Outdated Show resolved Hide resolved
@liamappelbe
Copy link
Contributor Author

Friendly ping @natebosch . I think this is ready to go. I've switched it from deleting the old flag/api to just deprecating it.

@liamappelbe liamappelbe merged commit 19bd6d4 into master Mar 29, 2022
@liamappelbe liamappelbe deleted the package_flag branch March 29, 2022 23:44
cbracken added a commit to cbracken/flutter that referenced this pull request May 14, 2022
This rolls depdendencies to latest using
flutter update-packages --force-upgrade

This change includes three code changes:

* Removes charcode from the dependencies allowlist since it no longer
  appears in the transitive closure of dependencies of the flutter,
  flutter_test, flutter_driver, flutter_localizations, and
  integration_test packages.

* Uses Resolver.create instead of the deprecated Resolver constructor.
  The default Resolver constructor has been deprecated in favour of the
  static Resolver.create() factory function, which unfortunately happens
  to be async. Propagated the async-ness up the chain.

* Eliminates the use of the deprecated packagesPath parameter to
  HitMap.parseJson. This parameter was deprecated and replaced with
  packagePath in dart-lang/coverage#370 which
  was part of the overall deprecation of the .packages file in Dart
  itself dart-lang/sdk#48272. The overall goal
  being that end-user code shouldn't need to know about implementation
  details such as whether dependency information is stored in a
  .packages file or a package_info.json file, but rather use the
  package_config package to obtain the package metadata and perform
  other functions such as resolving its dependencies to filesystem
  paths. packagesPath was replaced by packagePath, which takes the path
  to the package directory itself. Internally, package:coverage then
  uses package_config to do the rest of the package/script URI
  resolution to filesystem paths.

This is a pre-update prior to updating flutter_template_images in
flutter#103739

Issue: flutter#103371
Issue: flutter#103775
@cbracken
Copy link
Member

@liamappelbe @natebosch looks like this was soft-breaking to Flutter (though our lint rules cause this to be hard-breaking). I've got a patch that fixes it up here:
flutter/flutter#103771

Unfortunately, applying that patch would break google3, because package:coverage appears to not have rolled to google3 yet, as a result, we're blocked on all flutter depdendency upgrades at the moment.

Is there a plan to roll this in soon?

@cbracken
Copy link
Member

cbracken commented May 14, 2022

I've temporarily marked these uses as ignore to get us unblocked on the Flutter side. Filed flutter/flutter#103830 to track. This will require some internal patches for Flutter coverage collection under Blaze as well. I've noted them in a hand-wavy way in the above-mentioned issue.

cbracken added a commit to flutter/flutter that referenced this pull request May 14, 2022
Roll dependendencies

This rolls depdendencies to latest using
flutter update-packages --force-upgrade

This change includes three code changes:

* Removes charcode from the dependencies allowlist since it no longer
  appears in the transitive closure of dependencies of the flutter,
  flutter_test, flutter_driver, flutter_localizations, and
  integration_test packages.

* Uses Resolver.create instead of the deprecated Resolver constructor.
  The default Resolver constructor has been deprecated in favour of the
  static Resolver.create() factory function, which unfortunately happens
  to be async. Propagated the async-ness up the chain.
  This change was partially reverted and the deprecation ignored in this
  patch until package:coverage can be rolled internally at Google.

* Eliminates the use of the deprecated packagesPath parameter to
  HitMap.parseJson. This parameter was deprecated and replaced with
  packagePath in dart-lang/coverage#370 which
  was part of the overall deprecation of the .packages file in Dart
  itself dart-lang/sdk#48272. The overall goal
  being that end-user code shouldn't need to know about implementation
  details such as whether dependency information is stored in a
  .packages file or a package_info.json file, but rather use the
  package_config package to obtain the package metadata and perform
  other functions such as resolving its dependencies to filesystem
  paths. packagesPath was replaced by packagePath, which takes the path
  to the package directory itself. Internally, package:coverage then
  uses package_config to do the rest of the package/script URI
  resolution to filesystem paths.
  This change was partially reverted and the deprecation ignored in this
  patch until package:coverage can be rolled internally at Google.

This is a pre-update prior to updating flutter_template_images in
#103739

Issue: #103371
Issue: #103775
Issue: #103830

When re-applying the partially-reverted changes to code coverage,
we'll need to patch host_entrypoint.dart internally to await the Future
that we'll be returning rather than a non-async value.
cbracken added a commit to flutter/flutter that referenced this pull request May 31, 2022
In #103771, we rolled
dependencies in Flutter, which triggered an update of package:coverage
to v1.3.1. The new version includes
dart-lang/coverage#370 in which two deprecations
landed:

* The `Resolver` default constructor was deprecated and replaced with
  the `Resolver.create` static factory method, which unfortunately
  happens to be async.
* The `packagesPath` parameter to `HitMap.parseJson`, which takes the
  path to the `.packages` file of the package for which coverage is to
  be collected, was deprecated. This parameter was replaced with
  `packagePath` in dart-lang/coverage#370 which
  was part of the overall deprecation of the .packages file in Dart
  itself dart-lang/sdk#48272. The overall goal
  being that end-user code shouldn't need to know about implementation
  details such as whether dependency information is stored in a
  .packages file or a package_info.json file, but rather use the
  package_config package to obtain the package metadata and perform
  other functions such as resolving its dependencies to filesystem
  paths. packagesPath was replaced by packagePath, which takes the path
  to the package directory itself. Internally, package:coverage then
  uses package_config to do the rest of the package/script URI
  resolution to filesystem paths.

This migrates off the deprecated `packagesPath` parameter to the
replacement `packagePath` paramter.

Issue: #103830
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
In flutter#103771, we rolled
dependencies in Flutter, which triggered an update of package:coverage
to v1.3.1. The new version includes
dart-lang/coverage#370 in which two deprecations
landed:

* The `Resolver` default constructor was deprecated and replaced with
  the `Resolver.create` static factory method, which unfortunately
  happens to be async.
* The `packagesPath` parameter to `HitMap.parseJson`, which takes the
  path to the `.packages` file of the package for which coverage is to
  be collected, was deprecated. This parameter was replaced with
  `packagePath` in dart-lang/coverage#370 which
  was part of the overall deprecation of the .packages file in Dart
  itself dart-lang/sdk#48272. The overall goal
  being that end-user code shouldn't need to know about implementation
  details such as whether dependency information is stored in a
  .packages file or a package_info.json file, but rather use the
  package_config package to obtain the package metadata and perform
  other functions such as resolving its dependencies to filesystem
  paths. packagesPath was replaced by packagePath, which takes the path
  to the package directory itself. Internally, package:coverage then
  uses package_config to do the rest of the package/script URI
  resolution to filesystem paths.

This migrates off the deprecated `packagesPath` parameter to the
replacement `packagePath` paramter.

Issue: flutter#103830
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.

Deprecate --packages flag
5 participants