Improve resolution warning for wit-manifest from commit instead of disk

jackkoenig commented 4 years ago

Concrete proposal for improvement to warning. The idea is that it's as clear as possible to the user that the Wit resolution algorithm picks what wit-manifest.jsons are being used and they may not match the ones currently checked out on disk.

I got these messages by creating a workspace for https://github.com/sifive/example-scala-wake-project and then checking out master of it's dependency, api-scala-sifive. Wit correctly tells me that it's ignoring the wit-manifest.json that's currently on disk for api-scala-sifive, instead using the one from the commit that is pointed to by example-scala-wake-project

Wit `v0.12.0` message

$ wit status
Clean packages:
    example-scala-wake-project
Dirty packages:
    api-scala-sifive (new commits)
[WARNING] using 'api-scala-sifive::cade56f' manifest instead of checked-out version of 'api-scala-sifive'
api-scala-sifive (will be checked out to cade56f)

Currently proposed small change

instead of checked-out version of <package> -> currently checked-out version in <package>

$ wit status
Clean packages:
    example-scala-wake-project
Dirty packages:
    api-scala-sifive (new commits)
[WARNING] using 'api-scala-sifive::cade56f' manifest instead of currently checked-out version in 'api-scala-sifive'
api-scala-sifive (will be checked out to cade56f)

Alternative proposal

$ wit status
Clean packages:
    example-scala-wake-project
Dirty packages:
    api-scala-sifive (new commits)
[WARNING] using wit-manifest at 'api-scala-sifive::cade56f' instead of 'api-scala-sifive'/wit-manifest.json' in current wit workspace
api-scala-sifive (will be checked out to cade56f)

mmjconolly commented 4 years ago

what does it mean for wit to 'use' a manifest

jackkoenig commented 4 years ago

what does it mean for wit to 'use' a manifest

It means it's "using" the dependencies specified in the manifest for dependency resolution. Is there a different verb that would be more clear? "reading"?

mmjconolly commented 4 years ago

To bikeshed a little, how about

Warning about potential unexpected behaviour
Current Status
How to change expectation

[NOTICE] 
Repository 'api-scala-sifive' would be reverted to revision 'cade56f' by a 'wit update'.
Currently checked out revision is 'abcd123'. 
Use 'wit update-pkg/dep' to change the expected revision.

richardxia commented 4 years ago

what does it mean for wit to 'use' a manifest

It means it's "using" the dependencies specified in the manifest for dependency resolution. Is there a different verb that would be more clear? "reading"?

I think the issue is that from a user's perspective, they don't understand what the result of the dependency resolution is used for. I'll admit that even I get confused by the message, even as an advanced user of Wit, since I don't know the internals of Wit very well.

I think for the specific scenario of using wit status, I think the warning message is probably unnecessary altogether, since the purpose of running wit status is to see the differences between what you have on disk or "staged" vs what you'll get with a wit update. I have basically taught myself to ignore that warning when I run wit status.

In other scenarios, different behaviors may be clearer, but it's not obvious to me what is desirable without examining each case individually.

hcook commented 4 years ago

At the moment the "WARNING" status is the only place that contains the information that an update will change the the checked out version BECAUSE a different manifest is being used than a locally dirty one.

I'd like to see the various facts all chained together:

<nameX> will be checked out to <commitA>,
because wit is using dependency in the manifest from <nameY>::<commitB>,
instead of checked out version <nameY>::<commitC>, 
which depends on <nameX>::<commitD>

Is that possible?

sifive / wit