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.

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

[old/abandoned/do not merge] Improve interactive dist plan output #1560

Closed
wants to merge 2 commits into from

Conversation

duckinator
Copy link
Contributor

@duckinator duckinator commented Nov 14, 2024

Here's what it looks like as of the last time I edited this comment (~3:30pm ET on November 14th):

image

If everything in a workspace has the same version, everything works as it did before this PR:

image

TODO:

  • if --output-format=json, behave as main does now.

@duckinator duckinator force-pushed the incoherent-plan branch 8 times, most recently from b21999c to a07111b Compare November 14, 2024 20:40
@duckinator

This comment was marked as outdated.

@duckinator
Copy link
Contributor Author

duckinator commented Nov 15, 2024

@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.

pub fn parse(&self) -> Result<cargo_dist_schema::DistManifest> {
let src = SourceFile::new("dist-manifest.json", self.raw_json.clone());
let val = src.deserialize_json()?;
Ok(val)
}
}

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.

@duckinator
Copy link
Contributor Author

revised plan for this:

  • add a command (or flag for dist plan) that prints a JSON representation of all possible tags
  • in the generated workflow, use that new command to determine what tags there are, and run dist plan --output-format=json --tag=${TAG} > plan-dist-manifest-${TAG}.json for each one

@duckinator

This comment was marked as outdated.

@duckinator duckinator closed this Nov 19, 2024
@duckinator duckinator reopened this Nov 19, 2024
@duckinator duckinator changed the title Have "dist plan" with mismatched versions explain what different tags would announce Improve interactive dist plan output Nov 19, 2024
@duckinator duckinator changed the title Improve interactive dist plan output [old/abandoned/do not merge] Improve interactive dist plan output Dec 3, 2024
@duckinator
Copy link
Contributor Author

Closing in favor of #1595.

@duckinator duckinator closed this Dec 3, 2024
@duckinator duckinator deleted the incoherent-plan branch December 6, 2024 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant