Fail on non-dev packages with executables that are only used within bin/

.github/PULL_REQUEST_TEMPLATE.md

@@ -1,45 +1,15 @@
 ## Motivation
-  <!-- High-level overview of what you are trying to fix or improve, and why.
-         Include any relevant background information that reviewers should know. -->
+  <!--
+    High-level overview of what you are trying to fix or improve, and why.
+    Include any relevant background information that reviewers should know.
+  -->
 
 ## Changes
-  <!-- What this PR changes to fix the problem. -->
-
-#### Release Notes
-  <!-- A concise description of your changes, written in the imperative.
-         ("Fix bug" and not "Fixed bug" or "Fixes bug.") -->
-
-## Review
-_[See CONTRIBUTING.md][contributing-review-types] for more details on review types (+1 / QA +1 / +10) and code review process._
-
-  <!-- If you're making a PR from outside of the Frontend Development Experience team, then first off, thanks! :)
-
-        For open-source contributors, tag @Workiva/app-frameworks and we'll take a look!
-
-        For Workiva employees:
-
-            *** Please refrain from tagging the whole team to prevent extraneous notifications. ***
-
-            If you're not sure who from our team should review these changes, then leave this section
-            blank for now and post a link to the PR in the #support-frontend-dx Slack channel.
-
+  <!--
+    What this PR changes to fix the problem.
   -->
 
-Please review: <!-- Tag people here or via GitHub's "Request Review" feature-->
-
-### QA Checklist
-- [ ] Tests were updated and provide good coverage of the changeset and other affected code
-- [ ] Manual testing was performed if needed
-    - [ ] Steps from PR author: 
-        <!-- Call out any specific manual testing instructions here, or omit this section if not applicable -->
-    - [ ] Anything falling under manual testing criteria [outlined in CONTRIBUTING.md][contributing-manual-testing]
-
-## Merge Checklist
-While we perform many automated checks before auto-merging, some manual checks are needed:
-- [ ] A Frontend Frameworks Architecture member has reviewed these changes
-- [ ] There are no unaddressed comments _- this check can be automated if reviewers use the "Request Changes" feature_
-- [ ] _For release PRs -_ Version metadata in Rosie comment is correct
-
-
-[contributing-review-types]: https://github.com/Workiva/dependency_validator/blob/master/CONTRIBUTING.md#review-types
-[contributing-manual-testing]: https://github.com/Workiva/dependency_validator/blob/master/CONTRIBUTING.md#manual-testing-criteria
+## Testing/QA Instructions
+  <!--
+    List manual testing instructions here if necessary, or indicate passing CI suffices if the changes are well covered by automated tests.
+  -->

.github/workflows/dart_ci.yaml

@@ -12,21 +12,17 @@ on:
 jobs:
   test:
     runs-on: ubuntu-latest
-    strategy:
-      matrix:
-        sdk: [ 2.19.6 ]
     steps:
       - uses: actions/checkout@v2
       - uses: dart-lang/setup-dart@v1
         with:
-          sdk: ${{ matrix.sdk }}
+          sdk: 2.19.6
       - name: Install dependencies
         run: dart pub get
       - name: Validate dependencies
         run: dart run dependency_validator
-      - name: Check formatting
-        run: dart format --output=none --set-exit-if-changed .
-        if: ${{ matrix.sdk == 'stable' }}
+      # - name: Check formatting
+      #   run: dart format --output=none --set-exit-if-changed .
       - name: Analyze project source
         run: dart analyze
       - name: Run tests

README.md

@@ -14,7 +14,7 @@ dev_dependencies:
 ## Usage
 
 ```bash
-pub run dependency_validator
+dart run dependency_validator
 ```
 
 This will report any missing, under-promoted, over-promoted, and unused

lib/src/dependency_validator.dart

@@ -308,22 +308,35 @@ Future<void> run() async {
   unusedDependencies.removeAll(packagesWithConsumedBuilders);
 
   // Remove deps that provide executables, those are assumed to be used
-  final packagesWithExecutables = Set<String>();
-  for (final package in unusedDependencies.map((name) => packageConfig[name])) {
-    // Search for executables, if found we assume they are used
-    final binDir = Directory(p.join(p.fromUri(package!.root), 'bin'));
-    hasDartFiles() =>
-        binDir.listSync().any((entity) => entity.path.endsWith('.dart'));
-    if (binDir.existsSync() && hasDartFiles()) {
-      packagesWithExecutables.add(package.name);
-    }
+  final packagesWithExecutables = unusedDependencies
+    .where((name) {
+      final package = packageConfig[name];
+      final binDir = Directory(p.join(p.fromUri(package!.root), 'bin'));
+      if (!binDir.existsSync()) return false;
+
+      // Search for executables, if found we assume they are used
+      return binDir.listSync().any((entity) => entity.path.endsWith('.dart'));
+    }).toSet();
+
+  final nonDevPackagesWithExecutables = packagesWithExecutables
+    .where(pubspec.dependencies.containsKey)
+    .toSet();
+  if (nonDevPackagesWithExecutables.isNotEmpty) {
+    logIntersection(
+      Level.WARNING,
+      'The following packages contain executables, and are only used outside of lib/. These should be downgraded to dev_dependencies:',
+      unusedDependencies,
+      nonDevPackagesWithExecutables,
+    );
+    exitCode = 1;
   }
 
   logIntersection(
-      Level.INFO,
-      'The following packages contain executables, they are assumed to be used:',
-      unusedDependencies,
-      packagesWithExecutables);
+    Level.INFO,
+    'The following packages contain executables, they are assumed to be used:',
+    unusedDependencies,
+    packagesWithExecutables,
+  );
   unusedDependencies.removeAll(packagesWithExecutables);
 
   if (unusedDependencies.contains('analyzer')) {

test/executable_test.dart

@@ -561,6 +561,40 @@ void main() {
       expect(result.stdout, contains('No dependency issues found!'));
     });
 
+    test('fails when dependencies not used provide executables, but are not dev_dependencies', () async {
+      final pubspec = unindent('''
+          name: common_binaries
+          version: 0.0.0
+          private: true
+          environment:
+            sdk: '>=2.4.0 <3.0.0'
+          dependencies:
+            build_runner: ^1.7.1
+            coverage: any
+            dart_style: ^1.3.3
+            dependency_validator:
+              path: ${Directory.current.path}
+          dependency_overrides:
+            build_config:
+              git:
+                url: https://github.com/dart-lang/build.git
+                path: build_config
+                ref: $buildConfigRef
+          ''');
+
+      await d.dir('common_binaries', [
+        d.dir('lib', [
+          d.file('fake.dart', 'bool fake = true;'),
+        ]),
+        d.file('pubspec.yaml', pubspec),
+      ]).create();
+
+      result = checkProject('${d.sandbox}/common_binaries');
+
+      expect(result.exitCode, 1);
+      expect(result.stderr, contains('The following packages contain executables, and are only used outside of lib/. These should be downgraded to dev_dependencies'));
+    });
+
     test(
         'passes when dependencies are not imported but provide auto applied builders',
         () async {
@@ -680,7 +714,7 @@ void main() {
         await d.dir('dependency_pins', [
           d.dir('lib', [
             d.file('test.dart', unindent('''
-            "import 'package:logging/logging.dart'; 
+            "import 'package:logging/logging.dart';
             final log = Logger('ExampleLogger');"
             ''')),
           ]),