Support Pub Workspaces

[Levi-Lesches]

Motivation

Fixes #137 by supporting use in a mono-repo

Changes

In a regular package: nothing
In a sub-package of a workspace: nothing
In a workspace: instead of failing, recursively run itself on all sub-packages

I did remove the error condition that dart pub get must be run before running the tool, since that was throwing errors for workspaces, and I did not see a use for it (the pubspec.lock or .dart_tool files are never checked, only the code and pubspec.yaml).

Testing/QA Instructions

Looking into adding unit tests now, but basically, create a workspace and try running the tool in there.

For now, I had to add a dependency override on pubspec_parse until dart-lang/tools#1948 is merged. For some reason that's affecting the tests, so many of them are failing as they can't override the package while executing.

@evanweible-wf for review

[matthewnitschke-wk]

Thanks for the contribution! I'll take a look in a bit!

[matthewnitschke-wk]

@Levi-Lesches

Approach looks good to me! but I do have a few questions about some edge cases of the pub workspace implementation

Its not documented super well, but it does appear that you are still able to implement dart source in the root (interestingly any dependencies added to the root are available to all sub-packages, but this messes up publishing of each sub-package, which would to be a negative, and transitively a fantastic opportunity for dependency_validator to catch)

Because of the above, I think it would be best to not stop evaluation of the root lib/ assets if workspaces is declared, like is currently implemented

Additionally, I noticed that when one of the sub-packages has a violation, other subpackages are not outputted as a "success", the output is empty. Visually, it would be nice to see each subpackage outputted, and whether it passed validation

Finally, dart apis support a -C option to execute on provided subpackages. Not something that I'd hold this PR back on, but it might be nice to mirror this option and allow dependency_validator to be ran from the root, but only evaluate a single sub-package

Since we're currently blocked on dart-lang/tools#1948, I'll go ahead and convert this PR to a draft, and apply a Hold label. Feel free to ping me directly when dependent change is merged to the upstream, and the above issues resolved

[gabrielgarciagava]

@matthewnitschke-wk Just a heads up regarding:

Since we're currently blocked on dart-lang/tools#1948, I'll go ahead and convert this PR to a draft, and apply a Hold label. Feel free to ping me directly when dependent change is merged to the upstream, and the above issues resolved

The linked PR is merged now.

[matthewnitschke-wk]

@matthewnitschke-wk Just a heads up regarding:

Since we're currently blocked on dart-lang/tools#1948, I'll go ahead and convert this PR to a draft, and apply a Hold label. Feel free to ping me directly when dependent change is merged to the upstream, and the above issues resolved

The linked PR is merged now.

Thanks! Looks like we still need the package published: https://pub.dev/packages/pubspec_parse/changelog

[Levi-Lesches]

Because of the above, I think it would be best to not stop evaluation of the root lib/ assets if workspaces is declared, like is currently implemented

Additionally, I noticed that when one of the sub-packages has a violation, other subpackages are not outputted as a "success", the output is empty. Visually, it would be nice to see each subpackage outputted, and whether it passed validation

Fixed both

Validating dependencies for pkg1...
✓ No dependency issues found!

Validating dependencies for pkg2...
These packages may be unused, or you may be using assets from these packages:
  * meta

Validating dependencies for root...
These packages are used in lib/ but are not dependencies:
  * flutter

(interestingly any dependencies added to the root are available to all sub-packages, but this messes up publishing of each sub-package, which would to be a negative, and transitively a fantastic opportunity for dependency_validator to catch)

Already handled! Say root imports and declares package:http, and pkg1 uses http without declaring it:

Validating dependencies for pkg1...
These packages are used in lib/ but are not dependencies:
  * http

Validating dependencies for pkg2...
✓ No dependency issues found!

Validating dependencies for root...
✓ No dependency issues found!

Worth noting that dart analyze already flags this as depend_on_referenced_packages

Finally, dart apis support a -C option to execute on provided subpackages. Not something that I'd hold this PR back on, but it might be nice to mirror this option and allow dependency_validator to be ran from the root, but only evaluate a single sub-package

Done!

$ dart run dependency_validator --directory pkg1
$ dart run dependency_validator -C pkg1

Building package executable... 
Built dependency_validator:dependency_validator.

Validating dependencies for pkg1...
✓ No dependency issues found!

Thanks! Looks like we still need the package published

Version 1.5.0 will probably hit soon, I know they're working on some other small PRs first (glad I managed to get mine in!)

• pubspec_parse: Added support for executables field in pubspec.yaml dart-lang/tools#1952
• pubspec_parse: Added support for false_secrets dart-lang/tools#1951
• pubspec_parse: update dependencies and require Dart 3.6 dart-lang/tools#1946

---

Most of the tests are not going to work until the new pubsepc_parse version is published, so I'll check again once that happens

[matthewbelisle-wf]

Going to close and re-open this, trying to fix one of the CI errors.

[Levi-Lesches]

I'm wondering if I can make testing easier by basically wrapping all existing tests, and then running them twice: once as normal packages and one as a sub-package in a workspace. How feasible do you think that is?

[matthewnitschke-wk]

I'm wondering if I can make testing easier by basically wrapping all existing tests, and then running them twice: once as normal packages and one as a sub-package in a workspace. How feasible do you think that is?

My personal preference would be to write specific tests for the new functionality that workspace support implements:

• execution of subpackages
  • with respecting nested dart_dependency_validator.yaml files
• path normalization
• evaluation of the root package

That way we can isolate what specifically needs to be tested for workspaces, and what is more generally applicable to individual package evaluation

[matthewnitschke-wk]

@Levi-Lesches Excellent updates! Thanks for optimizing our tests!!

Now that we're updating the minimimum dart version to 3, we'll need to update ci.yaml to:

name: CI

on:
  pull_request:
  push:
    branches:
      - 'master'

jobs:
  build:
    uses: Workiva/gha-dart-oss/.github/workflows/build.yaml@v0.1.7
    with:
      sdk: stable

  checks:
    uses: Workiva/gha-dart-oss/.github/workflows/checks.yaml@v0.1.7
    with:
      sdk: stable

  unit-tests:
    uses: Workiva/gha-dart-oss/.github/workflows/test-unit.yaml@v0.1.7
    with:
      sdk: stable

and

name: Publish

on:
  push:
    tags:
      - '[0-9]+.[0-9]+.[0-9]+'

permissions:
  contents: write
  id-token: write
  pull-requests: write

jobs:
  publish:
    uses: Workiva/gha-dart-oss/.github/workflows/publish.yaml@v0.1.7
    with:
      sdk: stable

Once that's updated, we should be able to get passing CI, and merge this work!

As a side note, I've annotated this PR with semver: Major, meaning the next tagged release should be: v5.0.0. This is mostly due to the supported dart sdk version increasing, which is a breaking change

[Levi-Lesches]

Done, updated:

• the CI to use stable only with your changes
• the example doc to include more details about installing and configuring
• the README to include details about workspaces
• the pubspec.yaml to version 5.0.0
• the changelog to include workspaces and the new Dart 3.0 requirement

---

That issue of CI not having the correct version of Dart happens to me a lot. I filed #141 with a way the tool can flag these cases.

[dustinlessard-wf]

@Levi-Lesches , we're intending on merging and releasing this change today. 🙇🏼

[dustinlessard-wf]

Closing and reopening the pr as it looks like the unit-tests workflow is stuck. 🤞🏼

[dustinlessard-wf]

There's one issue that we need to resolve first. It looks like the unit-tests aren't actually running.

I was hoping a close/reopen would be a quick-fix, but it looks like something else is going on.

[Levi-Lesches]

@dustinlessard-wf I looked through your ci for a bit, and I found this:

chrome-platform: false
dart-dev: false
build-runner: true
build-test: false

The tests are only configured to run if:

• both builds are false, or
• both builds are true

Since this case is neither, both workflows are being skipped (see: gha-oss/test-unit.yaml)

[matthewnitschke-wk]

@Levi-Lesches This was a nuance of how we handle "required" workflows at Workiva. Fixed it from an external system we have. Fixed that issue and it should be good to go now!

Just running through some manual testing and then we should be good to merge!

[matthewnitschke-wk]

QA +1

Everything's looking good, pub workspaces is taken into account, verified that nested dart_dependency_validator packages are respected, verified the new -C option, and also confirmed that the root package is still working

Thanks again @Levi-Lesches, phenomenal job on this!

[matthewnitschke-wk]

🚀 @Workiva/release-management-p 🚢

[matthewnitschke-wk]

semver +1

Increases the accepted dart version from 2 to 3

[rmconsole-wf]

+1 from RM

[gabrielgarciagava]

Thank you all. I am looking forward to this new release. This was the last step for us to enable workspaces in our project <3

[Levi-Lesches]

Thanks for the quick merge and publish!

I love this tool and use it for everything, so I'm very happy to get the chance to contribute