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

+vats options #6395

Merged
merged 2 commits into from Mar 31, 2023
Merged

+vats options #6395

merged 2 commits into from Mar 31, 2023

Conversation

hoclun-rigsep
Copy link

With a hat tip to #6390, this pr also implements #6297.

Not quite ready to merge; in certain cases I don't understand, the formatting of the resulting tank gets messed up. Also I need to start getting my updates from ~binnec-dozzod-marzod. Will report back asap.

@hoclun-rigsep hoclun-rigsep force-pushed the i/6297/vats branch 6 times, most recently from b990032 to e46eadf Compare March 15, 2023 19:52
@hoclun-rigsep
Copy link
Author

Hmm, also this doesn't build after latest rebase. 18205a7 appears to break +vats.

/sur/hood/hoon::[76 14].[76 38]>
-need.@ud
- have
[ let=@ud
    hit
  ?(
    %~
    [ n=[p=@ud q=@uvI]
      l=?(%~ [n=[p=@ud q=@uvI] l=nlr([p=@ud q=@uvI]) r=nlr([p=@ud q=@uvI])])
      r=?(%~ [n=[p=@ud q=@uvI] l=nlr([p=@ud q=@uvI]) r=nlr([p=@ud q=@uvI])])
    ]
  )
    lab
  ?(
    %~
    [ n=[p=@tas q=@ud]
      l=?(%~ [n=[p=@tas q=@ud] l=nlr([p=@tas q=@ud]) r=nlr([p=@tas q=@ud])])
      r=?(%~ [n=[p=@tas q=@ud] l=nlr([p=@tas q=@ud]) r=nlr([p=@tas q=@ud])])
    ]
  )
]
nest-fail

@hoclun-rigsep
Copy link
Author

Here is my bug; you see the section of the output for %base is messed up.

> +vats, =verb |
%groups
  /sys/kelvin:      [%zuse 417] [%zuse 416] [%zuse 415]
  app status:       running
  publishing ship:  [~ ~sogryp-dister-dozzod-dozzod]
  pending updates:  ~
::
%base/sys/kelvin:      [%zuse 415]app status:       runningpublishing ship:  ~pending updates:  ~::
%landscape
  /sys/kelvin:      [%zuse 416] [%zuse 415]
  app status:       running
  publishing ship:  [~ ~lander-dister-dozzod-dozzod]
  pending updates:  ~
::

@pkova
Copy link
Collaborator

pkova commented Mar 16, 2023

Looks like the build fail is caused by the type changes from #6332.

@belisarius222
Copy link
Contributor

@elizasviel and @hoclun-rigsep: this architecture is cleaner than the other PR, so both of you, please focus your efforts on this one. Please also write some docs in the top of the generator to describe how to pass in the arguments.

@belisarius222
Copy link
Contributor

You probably need to make some small changes to the Clay types to get it to compile, due to the PR that @pkova mentioned.

@hoclun-rigsep
Copy link
Author

hoclun-rigsep commented Mar 16, 2023

You probably need to make some small changes to the Clay types to get it to compile, due to the PR that @pkova mentioned.

I get the compile error before I introduce any of my changes:

  1. fakezod with or without developer pill
  2. mount %base
  3. check out e17cb3e
  4. rsync -aL arvo/{app,mar,sur,gen,lib,ted} ../../zod/base/
  5. |commit %base
  6. nest-fail at /sur/hood/hoon::[76 14].[76 38]

I assume CI builds everything before it gets merged, so what is going on here? Is there something wrong with my flow?

@pkova
Copy link
Collaborator

pkova commented Mar 16, 2023

I fail to reproduce your issue. Here are the exact commands I ran:

git fetch --all
git checkout e17cb3e801a7403d67c6e4a926b80733056a8535
urbit -F zod -B bin/solid.pill
|mount %base
rsync -aL pkg/arvo/ zod/base/
|commit %base

Note that you need to be on vere edge to run this |commit since the branch develop is now on [%zuse 414]. Still, no matter what vere version you're on it shouldn't nest-fail there unless you're somehow missing this change:

0286ca7#diff-d2279d1657ac67537f222ce9b819365ba27aca4741becdce9d2f3f38e7f79f8cL76-R76

@hoclun-rigsep
Copy link
Author

@pkova thanks for checking that out. I am now on bleeding vere and included pill and back on track. I would feel better if I knew how I was able to produce the nest-fail on a release pier but I can come back to that.

@belisarius222 looking more closely at this file there seems to be a ton of repeated code/unnecessary scries; I've basically refactored it.

belisarius222
belisarius222 previously approved these changes Mar 23, 2023
@belisarius222
Copy link
Contributor

from the CI:

clay: read-at-aeon fail [desk=%base care=%a case=[%da p=~2023.3.23..15.33.40..ef7c] path=/gen/vat/hoon]
-find.report-vat
/gen/vat/hoon::[6 11].[6 21]>
/gen/vat/hoon::[6 10].[6 61]>
/gen/vat/hoon::[6 8].[6 62]>
/gen/vat/hoon::[6 1].[6 63]>
/gen/vat/hoon::[3 1].[6 63]>
/gen/vat/hoon::[2 1].[6 63]>
[%error-building /gen/vat/hoon]
clay: %a build failed [%base 1 /gen/vat/hoon]

@belisarius222
Copy link
Contributor

+report-vat is now inside another core, which the call site in /gen/vat/hoon needs to instantiate. Should be a quick fix.

@hoclun-rigsep
Copy link
Author

I think this will pass now but maybe hold off, there's still something wrong with my terse output.

@hoclun-rigsep hoclun-rigsep force-pushed the i/6297/vats branch 2 times, most recently from cd5a483 to 720b8e0 Compare March 24, 2023 00:12
@belisarius222
Copy link
Contributor

@hoclun-rigsep I saw you posted a commit after saying to hold off. Should I still hold off, or is this ready for review?

@hoclun-rigsep
Copy link
Author

@belisarius222 Hold off please; I might as well get the =verb | output right before merge. This is fun

This commit refactors `sur/hood.hoon` and changes the signature of the
`+vats` generator thus:

```hoon
|=  $:  [now=@da eny=@uvj bec=beak]
        $@(~ [?(%suspended %running %blocking %nonexistent) ~])
        $:  verb=?
            show-suspended=?
            show-running=?
            show-blocking=?
            show-nonexistent=?
        ==
    ==
```

Called with a single positional argument, `+vats` will show only those
desks fitting the description given, while keyword parameters allow
finer-grained control over which desks are described. The `verb`
parameter determines whether the full load of desk information be shown,
or a subset thereof.

Resolves urbit#6297.
@hoclun-rigsep
Copy link
Author

This can be reviewed. Issue: if the terminal is wide enough, information about the desk prints out on one line, which is probably not what we want. If someone can explain idiomatic use of the tank/tang system I can conform to it.

@belisarius222
Copy link
Contributor

@hoclun-rigsep I just learned from @joemfb that removing the top-level %rose and instead have the $tang be a list of lines (with each line as a $tank) should get it to print the way you want. Do it in a new PR, though. I'm merging this, because it's already a lot better than what we have now.

Thank you for your hard work on this, @hoclun-rigsep.

@belisarius222 belisarius222 merged commit dca3713 into urbit:develop Mar 31, 2023
@zalberico
Copy link
Collaborator

It’d be helpful if the details in the pr or release notes explained the options/showed basic usage and examples

@jfranklin9000
Copy link
Contributor

+vats doesn't handles the %kids presentation well:

+vats
<snip>
::
 %kids %kids %cz hash:     0v14.4jub3.fe4i1.5pfa6.do69s.fqkh7.tf6o9.g7b68.pgdeu.jjec8.u8517::
+vat %kids
%kids %cz hash:     0v14.4jub3.fe4i1.5pfa6.do69s.fqkh7.tf6o9.g7b68.pgdeu.jjec8.u8517

Also, I noticed that %suspended more corresponds to uninstalled. I had %escape installed and then uninstalled it from the UI and I see it in the +vats %suspended output. I have %bitcoin installed, but suspended, and I see it in the UI, but not in the +vats %suspended output.

@jfranklin9000
Copy link
Contributor

OK, I see that %escape has app status: suspended and that %bitcoin hasapp status: suspended until next update and is shown by +vats %blocking so you can disregard this part of my last comment.

@jalehman
Copy link
Member

It’d be helpful if the details in the pr or release notes explained the options/showed basic usage and examples

@tinnus-napbus would you mind working this into the Manual? Alternatively, would love if @hoclun-rigsep could help out with this.

@tinnus-napbus
Copy link
Contributor

It’d be helpful if the details in the pr or release notes explained the options/showed basic usage and examples

@tinnus-napbus would you mind working this into the Manual? Alternatively, would love if @hoclun-rigsep could help out with this.

yeah I'll PR an update to the +vats entry in dojo tools in operators shortly

@hoclun-rigsep
Copy link
Author

yeah I'll PR an update to the +vats entry in dojo tools in operators shortly

Thank you @tinnus-napbus; also happy to take this task if you wish.

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.

None yet

7 participants