urbit icon indicating copy to clipboard operation
urbit copied to clipboard

Figure out doccords release process

Open drbeefsupreme opened this issue 3 years ago • 1 comments

There are two main issues with doccords release:

  • Memory usage upon upgrade
  • Changes to the binary to jet the changes to the parser

Updating hoon.hoon means rebuilding everything, so as an OTA it has a high memory requirement. Thus we probably ought to wait until @philipcmonk completes changes to Ford that are designed help with the OTA memory usage issue.

I don't really understand what would be involved with changes to the binary. I should meet with @joemfb to help figure this out.

drbeefsupreme avatar May 24 '22 20:05 drbeefsupreme

#5849 needs to be fixed in a separate OTA that goes out before doccords release.

drbeefsupreme avatar Jun 22 '22 15:06 drbeefsupreme

Last week:

  • The :: syntax is working!

This week:

  • Knocking out a bunch of small issues
  • Refactoring the parser to clean things up prior to release
  • Jon ran into an issue with the Ford rune parsing, would be helpful to look it over with @Fang-

By EOW should be ready for code review of hoon.hoon changes - Jon will ping the engineering chat when that's ready.

zalberico avatar Oct 31 '22 16:10 zalberico

What remains (summary from @drbeefsupreme in team sync):

  • Going through each rune one by one and making sure comments attach as they're supposed to
  • Get the hoon.hoon PR ready for review
  • A little bit of refactoring work to get the parser in a good state
  • Then moving to the printing library: sometimes when asking the dojo for a doccords on a type it hangs for a long time, not sure why (may need a timeout here).

zalberico avatar Nov 14 '22 17:11 zalberico

Current branch for :: syntax changes:

https://github.com/urbit/urbit/tree/jon/doccords-parser-simplify - pardon the unhelpful commit messages, I'm planning on squashing them all into one when merging back into this branch.

It's cooled down from "exploded whitespace parser" back into something that thankfully looks a lot like the parsing for :> and :< syntax with a handful of new whitespace parsers in +norm that fit the pattern of the existing ones.

I'm expecting it will be good for review sometime this week (hopefully tomorrow).

drbeefsupreme avatar Nov 15 '22 00:11 drbeefsupreme

From talking with Jon in 1:1

  • This may require a new runtime version for some jet related stuff (if so we should create an issue with details and link it here)
  • #6052 is separated out because it would require a hoon kelvin version change, but since it doesn't require app developers to change things we should do it and ship it.
  • @drbeefsupreme will follow up with @joemfb about doing a synchronous code review on the hoon.hoon PR to get feedback and get it ready to release.

zalberico avatar Nov 16 '22 17:11 zalberico

Only remaining thing here is @drbeefsupreme needs to go over jet signatures with @joemfb, otherwise we think we're ready for release.

@drbeefsupreme is also thinking through other potential ways doccords could cause failures.

zalberico avatar Dec 05 '22 17:12 zalberico

@drbeefsupreme were you able to get the review in a good place? I'd like to have this in the 'ready to merge' category before the end of the year so we can get it into one of the first weekly releases.

zalberico avatar Dec 07 '22 03:12 zalberico

Sorry @zalberico , I need to mess with my GH notification settings so I don't miss these things.

The printing library and dojo changes haven't been reviewed at all yet. I'd say hoon.hoon is roughly 75% reviewed (whatever that means - its just somewhere more than halfway). Since @joemfb is sick and @Fang- is out for the rest of the year, we might not be able to finish the review before the end of the week. It will just depend on how much time @joemfb ends up having this week.

drbeefsupreme avatar Dec 12 '22 19:12 drbeefsupreme

Today I finally got my hands dirty reading/working through the documentation on jets. Since @joemfb is out and we want to have a release ready in a couple weeks and I was going to rely on Joe to handle the release/runtime stuff, I'm going to take a crack at handling all the release stuff myself. I'll definitely still need some help, especially process-wise.

Today was all tutorials, tomorrow I'll try actually fixing the jet registration in the doccords branch. Then I'll move onto figuring out the changes in Vere required for a Hoon kelvin decrement.

One outstanding question: what does the Kelvin shimming stuff #6073 have to do with jets and the runtime? Are we going to leave old jet registrations in the runtime or....?

drbeefsupreme avatar Jan 03 '23 23:01 drbeefsupreme

Things I found out/did today:

  • I wasn't actually 100% certain that we absolutely needed a Hoon kelvin decrement for #5873, though I was certain for #6064. I tested to ensure that, while you can |commit #5873 to a fresh fake ship, it doesn't actually use the new version of hoon.hoon. I'm still confused as to what exactly is going on here (something with Ford caches, I imagine?). This is basically the same reason as #6064, I think.
  • The following, which comes up when hot-reloading hoon.hoon with changes to the layer 1 core, is apparently expected and unrelated to jets, if I haven't messed with them?
fund: in in, parent 66fb3125 not found at 7
fund: in apt, parent 691abd03 not found at 7

I thought there was something for me to fix here, but I guess not. (still not certain)

  • I tried to make a version of Vere that supports Hoon 139. https://github.com/urbit/urbit/commit/9714a89166bf8181e530a3f5ce55809389bbd961 make freezes when it gets to the boot tests. @ashelkovnykov suggested that its because the +hoon-version core hash is wrong. I haven't yet figured out how to compute these hashes, but we're meeting tomorrow so I expect we'll make some progress.

I went through open PR's with the hoon tag. Here's what I'm expecting to include: #5873 #6160 #6169 #6117 I'm unsure about #5939 . Anything missing (besides #6064 which was a placeholder when I wasn't sure whether we needed to burn a Hoon kelvin)?

drbeefsupreme avatar Jan 04 '23 23:01 drbeefsupreme

@drbeefsupreme Just tested with the following changes to master:

Change hardcoded hoon version to 139 in pier.c:

...
u3nc(c3__hoon, 139),        //  god_u->hon_y
...

Register 139 kelvin version as jet in tree.c:

...
static u3j_core _d[] = {
  { "k140", 0, 0, _k140_d, _k140_ha, 0, (u3j_core*) 140, 0 },
  { "k139", 0, 0, _k139_d, _k139_ha, 0, (u3j_core*) 139, 0 },
  { "a50", 0, 0, _a50_d, _k140_ha, 0, (u3j_core*) c3__a50, 0 },
  {}
};
...

Have _k139_d reference the same cores as _140_d and copy _k140_ha hash for _k139_ha (since as @frodwith and the jetting guide point out, they don't do anything):

...
u3j_core _k139_d[] =
{ { "one", 3, 0, _140_one_d, _140_one_ha },
  {}
};
static c3_c* _k139_ha[] = {
  "9b82a903093c077afb3f0b9d4e95e1a9c9789d1ca605b57bbacf79857e3d5c52",
  0
};
...

Change kelvin version in hoon.hoon to %139:

...
=>  %139  =>
...

This built a working version of Vere which then correctly recompiled hoon.hoon.

@frodwith The hashes don't do anything (yet), but do you recall the command to compute the hash of a core for jet registration? I remember doing it for the JSON jet bounty, but I can't remember the command I used in the dojo. I thought it was somewhere in one of the jetting guides, but I can't find it anywhere.

ashelkovnykov avatar Jan 05 '23 00:01 ashelkovnykov

@frodwith Nevermind! I found it - I remembered that it's at the top of tree.c:

Ex.
`@ux`(shax (jam -:rip))

ashelkovnykov avatar Jan 05 '23 00:01 ashelkovnykov

Hey it works! Thanks @ashelkovnykov

arvo: recompiling hoon %139
...
> # layer-1
^layer-1
layer-1

basic mathematical operations

   <33.sam 1.pnw>

chapters:
^layer-1|containers
the most basic of data types

^layer-1|math
unsigned arithmetic

^layer-1|tree
tree addressing

compiled against:
(undocumented)

drbeefsupreme avatar Jan 05 '23 16:01 drbeefsupreme

@Fang- has raised the concern that four-space one-liner and two-space paragraphs looks ugly. and having two valid doccord formats makes styling inconsistent. the reason for it was to avoid "wrong doccords" for comments written without this in mind, but we're already gonna have these cuz two spaces for inline comments doccords is the only thing that makes sense. so perhaps that battle is already lost.

we already do things like ::TODO so maybe we should start insisting that if you dont want your comment to be a doccord, dont use spaces after the ::. i think these comments look fine

any thoughts/feedback?

EDIT: leave a thumbs up if you agree, comment optional. If you disagree, thumbs down and please leave a comment.

drbeefsupreme avatar Jan 06 '23 17:01 drbeefsupreme

Here's an example of each format so you have a better idea of what it looks like:

EDIT: links were wrong

https://github.com/urbit/urbit/blob/jon/doccords/pkg/arvo/lib/dprint.hoon#L58-L64 https://github.com/urbit/urbit/blob/jon/doccords/pkg/arvo/lib/dprint.hoon#L66-L69

drbeefsupreme avatar Jan 06 '23 17:01 drbeefsupreme

So there isn't a real problem with "wrong doccords" if someone goes through and fixes them all. I'm probably the right person to do it. So maybe "wrong doccords" is my subconscious misdirection trying to avoiding monotonous work. But I think it would ratchet this project from an A- to an A, and I can't accept an A-...

drbeefsupreme avatar Jan 06 '23 18:01 drbeefsupreme

Per conversation with @zalberico we're going to try to divide and conquer the comment formatting in parallel with a swarm of folks.

My opinion that having only one longform doccords format of two/four spaces, and insisting that non-doccords are of the form ::foo has grown stronger. Stylistically its simpler - you can immediately tell what is a doccord and what isn't at a glance. There's no ambiguity as to which format to use. There's no uglier format. The main cost is changing people's habits, but one of the core reasons for doccords was to change documentation habits - if you have to include the docs in your PR, then they'll get reviewed at the same time as your code. So there was going to be a change of habit in terms of process anyways.

Changing the parser in the limited way we're considering here is a 5 minute task, so comments are still welcome. It probably won't be until later next week where we'd actually start going through all the comments (at least in the kernel).

drbeefsupreme avatar Jan 06 '23 21:01 drbeefsupreme

Here is a simple outline of the proposed spacing style for comments that live on their own line. Informal comments are any comments thrown away by the whitespace parser. Formal comments are everything else - doccords.

:: - col col and then a new line. spacer, used for aesthetics as well as appears in between summaries and paragraphs of formal comments. ::foo - col col followed by zero spaces. Informal comments - do not appear as doccords. :: foo or :: $bar: foo - col col followed by two spaces, and an optional link ($bar:). A one-line summary doccord. Mandatory for all doccords. :: foo - col col followed by four spaces. optional body text formatted in paragraphs beneath a one-line summary. :: |=(* *) - col col followed by six spaces. optional body text marked as code beneath a one-line summary. this isn't used for anything yet but might be for e.g. tests in the future.

For inline comments, we only have informal comments ::foo and formal comments :: foo.

Anything that doesn't fit into one of these categories is also an informal comment. Most informal comments should be of the ::foo style, though.

drbeefsupreme avatar Jan 07 '23 09:01 drbeefsupreme

We got this out the door! Big thanks to @drbeefsupreme

zalberico avatar Feb 01 '23 00:02 zalberico