Analyze is failing, Warning: transitive closure contained non-allowlisted packages: [frontend_server_client]

[jmagman]

dart --enable-asserts ./dev/bots/analyze.dart

▌17:09:37▐ Package Allowlist...
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Warning: transitive closure contained non-allowlisted packages:
[frontend_server_client]
See dev/bots/allowlist.dart for instructions on how to update the package allowlist.
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

I see this on 07fc1a6a48 but I don't think this is related to a framework commit...

[jmagman]

test_core 0.3.20 was just published an hour ago and is the package with the frontend_server_client dependency, unless I'm missing something.

[Hixie]

Git bisect points to f6f59c5, which is when we added the allow list, which suggests that somehow our fixed pins are not working and we got caught by an upstream change. That's not supposed to be possible.

[jmagman]

frontend_server_client was added in dart-lang/test#1399. I'm not sure why the script picked up an unpinned version, but it looks legit to be added to the allowlist in the meantime. @jonahwilliams @jakemac53 do you agree?

[Hixie]

dev dependencies should be pinned too, right?

[jonahwilliams]

I suspect this is a bug in the --transitive-closure script in update packages. I'll plan on taking a look, but in the meantime we can add that package to the allowlist since its a dev_dep anyway. Maybe we should count dev deps separately anyway.

[jonahwilliams]

This is how it happens: update-packages tries to avoid pinning "transitive dependencies" in the synthetic pubspec, since its goal is to allow easy upgrades to the latest version of everything. --transitive-closure just wants to show the dependencies as they exist today, but re-uses the update-packages logic to generate a synthetic package. Since test_core is not pinned in this synthetic package, we version solve to the latest version and show frontend_server_client in the dep list - even though nothing in our repo is updated to use this

[jakemac53]

I don't see any alternative to pinning it. If it becomes a problem we can copy it into package:test_core instead, but I think it should be fine to pin for now.

[jonahwilliams]

So the bug is on our end, and I'll take a look at fixing that - in the meantime we can add it to the allowlist to unblock the tree

[jmagman]

Workaround #80912 merged, tree should be unblocked.
Lowering priority.

[jonahwilliams]

Changes to update-packages didn't take, will try a new approach

[jonahwilliams]

Update: this is still technically broken, though we worked around it. i need to rewrite the allowlist check to use pub deps directly.