Conversation
|
Don't we want that in mirage directly ? |
|
Actually, it's a very good example on how to have custom devices inside a |
| method clean i = | ||
| let dir = Info.root i in | ||
| Functoria_app.Cmd.run "rm %s/disk.img" dir | ||
| method module_name = "Functoria_runtime" |
There was a problem hiding this comment.
the module name is useless here, right?
There was a problem hiding this comment.
Yes. In those cases, my usual practice is to use "whichever module name is defined and mildly relevant".
We could define an empty module and use it for all those cases. Arguably we should have a way to express that, though ...
There was a problem hiding this comment.
maybe that should be set in base_configurable?
There was a problem hiding this comment.
have a default in base_configurable sounds sensible to me... it will break code, though (clients need method !module_name now).. I prepared a PR for functoria mirage/functoria#62 -- let me know what you think (this is independent from this PR: this improves the block example for current released stack ; certainly block should be updated once (a released) functoria provides a default module_name)
|
|
||
| method clean i = | ||
| let dir = Info.root i in | ||
| Functoria_app.Cmd.run "rm -f %s/disk.img" dir |
There was a problem hiding this comment.
@Drup would be really nice if functoria provided a safe (array-based) alternative to run. String-interpolation + rm -f is not a good example to set... (will break if dir contains spaces or other special characters, etc).
There was a problem hiding this comment.
@talex5 I agree. could we discuss this in the functoria issue tracker, instead of here, please?
There was a problem hiding this comment.
I know @samoht wanted to replace the various misc stuff by bos at some point. I don't know much about those APIs, it's probably a good idea, but please open a ticket and discuss it with him.
|
While this change raised some ideas to improve (mirage and functoria), it is good to merge as is (and already improves the |
|
I agree with your assessment, @hannesm ! Thanks for upstreaming this improvement, and for starting a good discussion. |
I stole^Wcherry-picked this commit from @Engil (and added the Makefile modifications myself)