Suggest updates and fixes to hello-world.md #769
Conversation
Given how common 8080 is, it is likely people trying to preview the site will have other things using the port. This change makes an easy workaround for that scenario. IIUC, it shouldn't impact the Mirage deployment at all, since it uses an entirely different configuration.
Make it clear that `start` functions re required. Correct terminology around `Lwt` value (afaik, `Lwt.return` does not "return a thread", rather it creates a (fulfilled) promise).
- even - "that is"
- Format markdown to produce a semantically correct list in the DOM:
this improves the formatting and readability in the site rendering IMO,
and should help with accessibility too.
- Remove environment specific output (i.e., paths on the the original
author's mchine)
- Remove outdated output and command names
- `make depend` is no now `make depends`
- The default make target now includes `make depend`, the correct
target to invoke just the build phase is `make build`
- Removes noisy output that doesn't help with instruction
- Improve ordering, so that command invocation example precedes
explanation of the steps the command performs, to prevent misleading
the reader into thinking they are meant to perform the merely
explanatory steps.
Despite the title, this section does not give any example of how to define or use functors. IMO, it is rather likely to confuse readers, since it speaks of functors and dependencies, but doesn't give any examples of either functors or how to use them to manage dependencies! In the next commit, I integrate the relevant explanatory parts with the example that actually shows how to use functors for dependency management.
In addition to integrating the general information about functors from the removed `hello-functor` section, this suggestion's revisions aim to: - clarify the exposition of key concepts, - improve organization, so related concepts are introdcued together and and developed in a progress flow. - reduce redundancy It also removes the `packages` parameter from the example, since it is not discussed at all in this section and, as far as I can tell from my own builds following the tutorial, actually not needed here at all.
I almost went down a rabbit of hole of building Solo5 from scratch due the to the note. Readers should know that generlaly shouldn't have to do anything special here.
|
Really thanks for this huge work! I did not look into details but I really want to thank you when we know that this kind of work is sorely lacking. |
|
Dear @shonfeder, thank you very much for your contribution and for splitting your changes into reasonable pieces. I reviewed the commits, and this looks good to be merged. One note, though, the target for installing dependencies started out as |
|
Anyone in @mirage/core against merging this PR? Specifically asking @talex5 who (I believe) has written the original text and may have an opinion. |
There was a problem hiding this comment.
Specifically asking @talex5 who (I believe) has written the original text and may have an opinion.
I don't think I did, but I reviewed it anyway. I haven't used Mirage since it moved to dune, so I can't comment on the accuracy of those bits, but it sounds reasonable.
| configuration is stored in `config.ml`, so let's take a look: | ||
| The concrete implementation of `Time` will be supplied at compile time, | ||
| depending on the target that you are compiling for. This calls for some | ||
| additional configuration in `config.ml`, so let's take a look: | ||
|
|
||
| ```ocaml | ||
| $ cat tutorial/hello/config.ml | ||
|
|
||
| open Mirage | ||
|
|
||
| let main = |
There was a problem hiding this comment.
It's a bit confusing that we're using main twice here. Maybe unikernel or hello would be clearer.
Co-authored-by: Thomas Leonard <talex5@gmail.com>
Co-authored-by: Thomas Leonard <talex5@gmail.com>
Co-authored-by: Thomas Leonard <talex5@gmail.com>
Co-authored-by: Anil Madhavapeddy <anil@recoil.org>
|
Looks great. Many thanks for your contributions! Happy to merge once the few remaining items raised by Thomas are addressed. |
| $ make depend | ||
| $ make | ||
| $ make depends | ||
| $ make build |
There was a problem hiding this comment.
I don't know if make build or dune build is the most useful to mention here. Maybe just saying that after make depend(s) has run, the workspace is setup and can just be built using dune (no need to interact with the mirage tool nor the Makefile file again).
There was a problem hiding this comment.
Note: we should probably hide this make depends into mirage depends at one point (and/or wait for dune to be able to deal with this directly as well)
There was a problem hiding this comment.
Yeah: I think it'd be lovely to be able to hide/remove the make invocations if possible.
On a related note, do you think the generated build artifacts during "configure" could end up being moved into _build at some point? It'd be cool to be able to run all the different configurations, and have those only set up different sub-trees in _build without having all the intermediate files changing the work tree.
There was a problem hiding this comment.
Maybe just saying that after make depend(s) has run, the workspace is setup and can just be built using dune (no need to interact with the mirage tool nor the Makefile file again).
That removes a level of indirection, but introduces a third command, leaving the user with mirage, make, and dune, instead of just mirage and make. Is that trade off preferable?
There was a problem hiding this comment.
On a related note, do you think the generated build artifacts during "configure" could end up being moved into _build at some point? It'd be cool to be able to run all the different configurations, and have those only set up different sub-trees in
_buildwithout having all the intermediate files changing the work tree.
Sounds like a good suggestion to me, would you mind opening an issue on github.com/mirage/mirage about it?
That removes a level of indirection, but introduces a third command, leaving the user with
mirage,make, anddune, instead of justmirageandmake. Is that trade off preferable?
I'm not sure. In the end I'd prefer to remove make from the process, since we won't get rid of mirage soon, and for more sophisticated things we need to interact with dune directly anyways. But indeed for the tutorial, I'd stick with mirage and make, and avoid dune. :)
There was a problem hiding this comment.
Sounds like a good suggestion to me, would you mind opening an issue on github.com/mirage/mirage about it?
Happy to do so!
Co-authored-by: Thomas Leonard <talex5@gmail.com>
data/wiki/hello-world.md
Outdated
|
|
||
| ...and now, onwards to unikernels that actually **do** something! | ||
| [rwo-9]: https://realworldocaml.org/v1/en/html/functors.html |
There was a problem hiding this comment.
Thanks for your valuable work on that! That's great!
Real World Ocaml v2 was just released last september, may be it's better to point to https://dev.realworldocaml.org/functors.html ?
There was a problem hiding this comment.
Yes, that's a good idea. Thanks :)
|
Is this now ready to be merged? |
Just two followups to fix before it's ready I think.
For the disambiguation of Thanks all for the encouragement and most of all for the careful and helpful reviews and corrections :) Sorry for the slightly slow pace in followup: just a bit of juggling free-time OSS contribs around work and life 馃Ε |
|
Thanks @shonfeder for working on this. My main motivation was to avoid this PR being stuck. I'm fine with merging it any time since it is already an improvement over the current text. And of course we can always have follow-up PRs that improve the text even more :) |
|
Oh that's fine with me then! I can just address these changes in followup work. Feel free to merge. Thanks for asking :) |
|
Thanks again, I updated the links to realworldocaml.org :) |
馃Ε ~~ Greetings,
I've been working through the hello world tutorial, and I've been very pleased to see how well all the pieces fit together. When I got the hello world running on Solo5 with just a few command invocations, I was truly delighted! Thanks for the wonderful infrastructure :)
Along the way I noticed several places where the documentation was out of date, noisy, or otherwise able to benefit from some revision. This PR includes my initial offering of a beginners revision.
If you find these suggestions helpful and welcome, I will continue to offer them as I work through the other tutorial material. Or, if the general idea of such contributions is welcome by I should alter my approach, this is a good time for me to get feedback. Or, lastly, if such contribution are unwelcome, I can save myself time by learning that :)
Reviewing
I tried to make the commits reasonably well isolated, and to explain the motivations for any substantial suggestions in the commit message, so reviewing by commit will likely make the review easier.