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.

Sign up for GitHub

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

Jump to bottom

[Tool] Add ability to check dependencies independently of dev-dependencies, exclude integration_test from dependencies #6446

Merged
merged 9 commits into from
Apr 3, 2024
Merged

[Tool] Add ability to check dependencies independently of dev-dependencies, exclude integration_test from dependencies #6446

merged 9 commits into from
Apr 3, 2024

Conversation

reidbaker
Copy link
Contributor

@reidbaker reidbaker commented Apr 1, 2024 •
edited
Loading

Create a linter that ensures that integration_test is not used in dependencies.

Will be paired with a change to documentation

If you are considering adding an external dependency:

Consider other options, and discuss with #hackers-ecosystem in Discord.
* If you add a dev_dependency on an external package, pin it to a specific version if at all possible.
* If you add a dependency on an external package in an example/, pin it to a specific version if at all possible.
* Some dependencies should only be linked as dev dependencies like integration_test

Related to flutter/flutter/issues/145992

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [relevant style guides] and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/packages repo does use dart format.)
  • I signed the [CLA].
  • The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
  • I [linked to at least one issue that this PR fixes] in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy], or this PR is [exempt from version changes].
  • I updated CHANGELOG.md to add a description of the change, [following repository CHANGELOG style].
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is [test-exempt].
  • All existing and new tests are passing.

@reidbaker reidbaker marked this pull request as ready for review April 1, 2024 15:22
stuartmorgan
stuartmorgan approved these changes Apr 1, 2024
View reviewed changes
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

LGTM

@reidbaker reidbaker added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 2, 2024
@auto-submit Auto-submit
Copy link
Contributor

auto-submit bot commented Apr 2, 2024

auto label is removed for flutter/packages/6446, due to - The status or check suite Linux repo_checks has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 2, 2024
reidbaker
reidbaker commented Apr 2, 2024
View reviewed changes
script/tool/lib/src/pubspec_check_command.dart Outdated
// pigeon/platform_tests/shared_test_plugin_code is allowed to violate
// the dev only dependencies rule beause pidgeon has generated tests that
// are intended to ship to customers.
if (pubspec.name == 'shared_test_plugin_code') {
Copy link
Contributor Author

@reidbaker reidbaker Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@stuartmorgan requesting re-review because I am not a fan of how I exempted pidgeon from the new check and am hoping you have a better idea.

Copy link
Contributor

@stuartmorgan stuartmorgan Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You can just conditionalize this whole new check on being publishable, and remove the warning:

packages/script/tool/lib/src/pubspec_check_command.dart

Line 209 in dcb4f85

if (pubspec.publishTo != 'none') {

@reidbaker reidbaker changed the title [Tool} Add ability to check dependencies independently of dev-dependencies, exclude integration_test from dependencies [Tool] Add ability to check dependencies independently of dev-dependencies, exclude integration_test from dependencies Apr 2, 2024
stuartmorgan
stuartmorgan approved these changes Apr 3, 2024
View reviewed changes
Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

LGTM with one minor comment-comment.

script/tool/lib/src/pubspec_check_command.dart Outdated Show resolved Hide resolved
@reidbaker reidbaker added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 3, 2024
@auto-submit Auto-submit
Copy link
Contributor

auto-submit bot commented Apr 3, 2024

auto label is removed for flutter/packages/6446, due to - The status or check suite Mac_arm64 macos_platform_tests master - packages has failed. Please fix the issues identified (or deflake) before re-applying this label.

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Apr 3, 2024
@reidbaker reidbaker added the autosubmit Merge PR when tree becomes green via auto submit App label Apr 3, 2024
@auto-submit auto-submit bot merged commit 3ff376b into main Apr 3, 2024
78 checks passed
@auto-submit auto-submit bot deleted the i145992-ban-integration-test-from-dependencies branch April 3, 2024 19:44
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Apr 4, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 4, 2024
@engine-flutter-autoroll
3ff376b [Tool] Add ability to check dependencies independently of dev…
291555f 
…-dependencies, exclude integration_test from dependencies (flutter/packages#6446)
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Apr 4, 2024
@engine-flutter-autoroll
Roll Packages from 0e848fa to dce6f0c (4 revisions) (#146276)
a70452a 
flutter/packages@0e848fa...dce6f0c

2024-04-04 49699333+dependabot[bot]@users.noreply.github.com Bump lewagon/wait-on-check-action from 1.3.3 to 1.3.4 (flutter/packages#6459)
2024-04-03 [email protected] [pigeon] Allow multi instance support with message channel name suffix (flutter/packages#6224)
2024-04-03 [email protected] [Tool] Add ability to check dependencies independently of dev-dependencies, exclude integration_test from dependencies (flutter/packages#6446)
2024-04-03 [email protected] Roll Flutter from a418568 to e868e2b (34 revisions) (flutter/packages#6455)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@ekuleshov ekuleshov mentioned this pull request Apr 5, 2024
Merged
11 tasks
auto-submit bot pushed a commit that referenced this pull request Apr 10, 2024
@ekuleshov
[tool] Check for test and flutter_test in non-dev dependencies (#…
e98839a 
…6472)

*Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.*

Additional checks as a followup to #6446

*List which issues are fixed by this PR. You must list at least one issue.*

Related to flutter/flutter#145992

*If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*
gilnobrega pushed a commit to gilnobrega/flutter that referenced this pull request Apr 22, 2024
@engine-flutter-autoroll @gilnobrega
Roll Packages from 0e848fa to dce6f0c (4 revisions) (flutter#146276)
22ea0eb 
flutter/packages@0e848fa...dce6f0c

2024-04-04 49699333+dependabot[bot]@users.noreply.github.com Bump lewagon/wait-on-check-action from 1.3.3 to 1.3.4 (flutter/packages#6459)
2024-04-03 [email protected] [pigeon] Allow multi instance support with message channel name suffix (flutter/packages#6224)
2024-04-03 [email protected] [Tool] Add ability to check dependencies independently of dev-dependencies, exclude integration_test from dependencies (flutter/packages#6446)
2024-04-03 [email protected] Roll Flutter from a418568 to e868e2b (34 revisions) (flutter/packages#6455)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages-flutter-autoroll
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
TecHaxter pushed a commit to TecHaxter/flutter_packages that referenced this pull request May 22, 2024
@reidbaker @TecHaxter
[Tool] Add ability to check dependencies independently of dev-depende…
980417e 
…ncies, exclude integration_test from dependencies (flutter#6446)

Create a linter that ensures that `integration_test` is not used in dependencies. 

Will be paired with a change to documentation 
```
If you are considering adding an external dependency:

Consider other options, and discuss with #hackers-ecosystem in Discord.
* If you add a dev_dependency on an external package, pin it to a specific version if at all possible.
* If you add a dependency on an external package in an example/, pin it to a specific version if at all possible.
* Some dependencies should only be linked as dev dependencies like integration_test 
```

Related to flutter/flutter/issues/145992
TecHaxter pushed a commit to TecHaxter/flutter_packages that referenced this pull request May 22, 2024
@ekuleshov @TecHaxter
[tool] Check for test and flutter_test in non-dev dependencies (f…
a97418a 
…lutter#6472)

*Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.*

Additional checks as a followup to flutter#6446

*List which issues are fixed by this PR. You must list at least one issue.*

Related to flutter/flutter#145992

*If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
@reidbaker @arc-yong
[Tool] Add ability to check dependencies independently of dev-depende…
429a6e7 
…ncies, exclude integration_test from dependencies (flutter#6446)

Create a linter that ensures that `integration_test` is not used in dependencies. 

Will be paired with a change to documentation 
```
If you are considering adding an external dependency:

Consider other options, and discuss with #hackers-ecosystem in Discord.
* If you add a dev_dependency on an external package, pin it to a specific version if at all possible.
* If you add a dependency on an external package in an example/, pin it to a specific version if at all possible.
* Some dependencies should only be linked as dev dependencies like integration_test 
```

Related to flutter/flutter/issues/145992
arc-yong pushed a commit to Arctuition/packages-arc that referenced this pull request Jun 14, 2024
@ekuleshov @arc-yong
[tool] Check for test and flutter_test in non-dev dependencies (f…
9416808 
…lutter#6472)

*Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.*

Additional checks as a followup to flutter#6446

*List which issues are fixed by this PR. You must list at least one issue.*

Related to flutter/flutter#145992

*If you had to change anything in the [flutter/tests] repo, include a link to the migration guide as per the [breaking change policy].*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekuleshov ekuleshov ekuleshov left review comments

@stuartmorgan stuartmorgan stuartmorgan approved these changes

@camsim99 camsim99 Awaiting requested review from camsim99

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@reidbaker @stuartmorgan @ekuleshov