Open duckinator opened 1 week ago
TODO: Figure out what to do with JSON output. Right now, it prints the JSON representation of each report individually, which means if you have mismatched versions the JSON output is invalid.
This should be resolved somehow before this is merged.
@ashleygwilliams here's my understanding of everything related to this PR.
dist plan
on main
intentionally fails when projects in the same workspaces have mismatched versions and --tag
isn't specified. However, when the generated workflow is running in a PR, there is no tag to specify — so it just runs dist plan --output-format=json > plan-dist-manifest.json
. This is guaranteed to cause errors for workspaces with mismatched versions.
This was encountered in the real-world here: https://github.com/mitre/hipcheck/actions/runs/11808204242/job/32896362439
It resulted in this error:
× There are too many unrelated apps in your workspace to coherently Announce!
help: Please either specify --tag, or give them all the same version
Here are some options:
--tag=v0.1.0 will Announce: activity, affiliation, binary, churn, entropy, fuzz, git, github, identity,
linguist, npm, review, typo
--tag=v3.7.0 will Announce: hipcheck
you can also request any single package with --tag=activity-v0.1.0
Error: Process completed with exit code 255.
Making dist plan
provide a human-readable output for multiple things is relatively straightforward -- you get a list of all known versions and iterate over them, generating the usual dist plan --output-format=human
output for each one.
But if you do that for the JSON output, it falls on its face: it tries to print multiple JSON documents in a row, which aren't valid.
The obvious solution is to put them in a JSON array. That looks something like:
[
{
"dist_version": "...",
"announcement_tag": "v0.1.0",
...
},
{
"dist_version": "...",
"announcement_tag": "v0.3.1",
...
}
]
But, everything in dist appears to be built under the assumption that the JSON output for dist plan
, dist manifest
, and dist build
would always be a single map. This change broke over 50 tests, all parsing the output of dist plan --output-format=json
.
I determined that if you change PlanResult::parse
so that it returns Result<Vec<cargo_dist_schema::DistManifest>>
instead of Result<cargo_dist_schema::DistManifest>
, it'll fix all 50-something tests... but breaks another one.
The one that is broken by changing this is parsing the output of dist build --output-format=json
-- which, prior to the work in this PR, has been identically structured to dist plan
, which has been identically structured to dist manifest
.
It is also unclear to me if the code in the generated workflows would work, even if we get the entire test suite passing. Fixing the tests may just mean we're no longer testing real-world usage.
I think the manifest is used by receipts, so it might break upgrades.
It also doesn't make sense for dist manifest
or dist build
to return an array, because they both are currently designed to only return a single manifest — and returning a single-item array is kind of misleading.
Basically: This functionality is at odds with what may be the most common assumption in this codebase.
revised plan for this:
dist plan
) that prints a JSON representation of all possible tagsdist plan --output-format=json --tag=${TAG} > plan-dist-manifest-${TAG}.json
for each oneClosing in favor of #1567.
Here's what it looks like as of the last time I edited this comment (~3:30pm ET on November 14th):
If everything in a workspace has the same version, everything works as it did before this PR:
TODO:
--output-format=json
, behave asmain
does now.