Re: [PATCH] crosvm: Rename `--vhost-user-{fs,gpu}` args

[Dom Rodriguez <shymega@shymega.org.uk>]

[-- Attachment #1: Type: text/plain, Size: 2960 bytes --]

On 08.09.2024 09:46, Alyssa Ross wrote:
>Dom Rodriguez <shymega@shymega.org.uk> writes:
>
>> On 06.09.2024 13:22, Alyssa Ross wrote:
>>>Dom Rodriguez <shymega@shymega.org.uk> writes:
>>>
>>>> crosvm was producing warnings when using `--vhost-user-gpu` and
>>>> `--vhost-user-fs`.
>>>>
>>>> In this commit, I have adjusted the `crosvm` invocations to look
>>>> something like `--vhost-user $DEVICE`, where `$DEVICE` is, in this case,
>>>> `gpu` or `fs`.
>>>>
>>>> I have run unit tests, but would appreciate testing.
>>>
>>>How did you run them?  release/checks/wayland is failing:
>>>
>>>vm-test-run-spectrum-wayland> machine # [   14.028205] crosvm[968]: [2024-09-06T11:19:20.132395456+00:00 ERROR crosvm] arg parsing failed: Error parsing option '--vhost-user' with value 'gpu': missing field `socket`
>>>vm-test-run-spectrum-wayland> machine # [   14.029442] crosvm[968]:  [src/main.rs:737] arg parsing failed: Error parsing option '--vhost-user' with value 'gpu': missing field `socket`
>>>
>>>Does it need to be (from memory) --vhost-user gpu,socket=path or something?
>>
>> I thought I had tested them, but as it turned out, the command I used
>> was only for shellcheck. I've run unit tests again, and all seems to
>> pass. I've queued up the v2 patch for review.
>
>Do you think we could change anything about the documentation to avoid
>people making that mistake in future?  It's hard to anticipate what will
>confuse people in advance.

 From my newcomer pespective, I assumed the command on the documentation
for testing ran all checks. I just blindly copied and pasted it. That
was an error on my part, and I should have known better.

I think this is more of a PEBKAC problem with me, rather than the
documentation.

>> Just a side note from my article about Flakes - we could use the
>> `checks` output to run on the `pre-commit` Git hook. This could then
>> check the commit message, run tests, etc.
>
>We could do that already, right?  There's already a command you can run
>to run all the checks.  My concern would be that some of the checks are
>slow (the ones that involve building images), so I think we might want a
>release/checks/fast.nix or something so it's not too annoying — I find
>slow pre-commit hooks to be a real drag if I want to keep rewording or
>slightly modifying a commit.  The flake eval cache would certainly help
>with the other slow bit (Nix eval), though.

I agree that the checks as a pre-commit hook would be slow.

I would like to integrate a CI environment on the list for each patch.
Perhaps we could use your suggestion of 'fast checks', and the CI could
run all of the checks upon receiving a patch?

I should have discussed this in the Flakes thread, so my suggestion
is that we continue discussion there, and in Matrix/IRC if the need
arises.

Best wishes,
--
Dom Rodriguez
GPG Fingerprint: EB0D 45E6 D0DC 1BA1 A2B5  FC24 72DC F123 1E54 BD43

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]