Document and build daemon CLIs for debugging #198

minishrink · 2018-01-30T10:15:51Z

Forgot to edit v6 jbuild to create v6_cli.exe, as well as updating make test rules to include it.
Note: make test still fails as it requires fixing Lwt_unix.bind.

Update: this PR has changed in nature and now includes the following:

debugging CLI tools no longer automatically built by make test
V6 jbuild file now includes rules to build V6 CLI (update to CP-26908)
README documents these CLI tools

Signed-off-by: Akanksha Mathur [email protected]

mseri · 2018-01-30T10:17:30Z

v6/jbuild

+
+(alias
+ ((name runtest)
+  (deps (v6_cli.exe))))


This doesn't make sense. Is it actually used in the tests?

You can build it already using the executable sexp that you added in line 43 by doing jbuilder build v6/v6_cli.exe and run it with _build/default/v6/v6_cli.exe (and soon with jbuilder exec ./v6/v6_cli.exe)

[Can I just say that is possibly the fastest response to a PR I've ever had?]

I just checked, and you are correct! The difference is that including these lines tells make test to build v6_cli.exe, whereas without, it currently builds the following:

lib/channel_helper.exe lib_test/test.exe memory/memory_cli.exe example/example.exe

I think the question is: do we want to include the daemon CLIs for make test, or have them explicitly built? If the former, we probably shouldn't build the squeezed CLI either, and if the latter, we should perhaps document the CLI a little more explicitly on our side so devs know about it. What do you think?

As we discussed, let's remove all of them, and let's run the example in the tests.

minishrink · 2018-01-30T10:17:04Z

v6/jbuild

    go ic ""

-let rewriters_ppx = ["ppx_deriving_rpc"; "ppx_sexp_conv"]
+let rewriters_ppx = ["ppx_deriving_rpc" ]


I wasn't really sure about removing ppx_sexp_conv, but it's not used in the squeezed jbuild, and I was able to locally build all v6 executables without any issue after removing this. Please let me know if it needs to be included.

It's fine, thanks for the cleanup

minishrink · 2018-01-30T10:17:52Z

v6/jbuild

+    rpclib.cmdliner
+    rpclib.markdown
+    xcp.v6))))
+


Not sure if there's a more succinct way to include rules for v6_cli, this is just based on the squeezed jbuild and how it builds memory_cli.exe

You can create aliases, but it's not worth. Let's get rid of those empty runtests and add a section in the readme about the debugging cli tools that you can build

The empty runtests were there to check that the examples/clis can be built even if not installed, my worry was is that if we don't build them they'll bitrot and when we do want to use them they won't work anymore. There's probably a nicer way to do that than using runtest though.

I imagined something like that, but I'd rather add a post-install-hook to run jbuilder build for them, eventually adding an alias for it, like @clitools or something else

mseri · 2018-01-30T11:23:11Z

README.md

     * RPCs
+  3. The following CLI tools for debugging:
+     * lib/channel_helper.exe -- a channel passing helper CLI
+     * [A] lib_test/test.exe -- a test suite built from tests in lib_test


Don't put the test.exe

True, it's not a CLI tool. I'll remove example.exe too.

mseri · 2018-01-30T11:23:26Z

README.md

+     * memory/memory_cli.exe -- a squeezed debugging CLI
+     * v6/v6_cli.exe -- a V6d debugging CLI
+
+      [A] - `make test` automatically builds and runs this


I think we don't need to mention this

I don't think this info is included anywhere else, since INSTALL.md just points to the README.md for "detailed build instructions". Might be worth including for the sake of completion?

(This would be redundant if we removed references to test and example from the list of CLI tools.)

Actually, is it worth expanding on make test in INSTALL.md?

It's outdated, I think we should remove it

Remove INSTALL.md or discussion of make test?

mseri · 2018-01-30T11:24:07Z

README.md

+     * v6/v6_cli.exe -- a V6d debugging CLI
+
+      [A] - `make test` automatically builds and runs this
+      For other executables, run `jbuilder build path/to/exec.exe`


add

and then run them with _build/default/path/to/exec.exe.

mseri · 2018-01-30T11:30:38Z

Note: tests will not pass until xapi-project/xs-opam#215 is merged

mseri · 2018-01-30T15:28:09Z

The build and tests now succeed, the failure is due to a broken .coverage.sh script. I suggest to ignore that issue and fix it as a followup to this PR, this should allow us to rewrite the coverage script and put it in an external repository once and for all!

mseri

Add cluster/cluster_cli.exe to the list of debugging CLI and then this can be merged

mseri · 2018-01-31T10:40:28Z

Can you please squash the commits and keep only two:

the refactoring of jbuilder test
update readme and remove install

- add rules to build V6 CLI - `make test` no longer automatically builds CLI tools - `make test` automatically runs test and example executables Signed-off-by: Akanksha Mathur <[email protected]> Make test builds (and runs) only test and example executables Previously, the following executables were built by `make test` despite not being used in tests: ``` - lib/channel_helper.exe - lib_test/test.exe - memory/memory_cli.exe - example/example.exe [note: make test still runs this] ``` With this commit, `make test`: - still builds `example.exe` and `test.exe` - automatically runs `example.exe` and `test.exe` - no longer builds `memory_cli.exe` nor `channel_helper.exe` Note that all of these can still be individually built using `jbuilder build [path to .exe as listed above]` Signed-off-by: Akanksha Mathur <[email protected]> fixup! whitespace strip Signed-off-by: Akanksha Mathur <[email protected]>

Signed-off-by: Akanksha Mathur <[email protected]> Only list CLI tools in README, not all executables Remove references to `test.exe` or `example.exe` in list of CLI tools. Signed-off-by: Akanksha Mathur <[email protected]> Add cluster_cli to list of CLI tools Signed-off-by: Akanksha Mathur <[email protected]> Remove outdated INSTALL.md Signed-off-by: Akanksha Mathur <[email protected]>

mseri reviewed Jan 30, 2018

View reviewed changes

minishrink commented Jan 30, 2018

View reviewed changes

mseri reviewed Jan 30, 2018

View reviewed changes

mseri mentioned this pull request Jan 30, 2018

Add clustering interface #195

Merged

mseri approved these changes Jan 30, 2018

View reviewed changes

minishrink force-pushed the v6_fix branch from 2b4b805 to 92f58cf Compare January 30, 2018 17:53

minishrink changed the title ~~CP-26098: Update jbuild to build V6_cli, add rule for make test~~ Document and build daemon CLIs for debugging Jan 31, 2018

Akanksha Mathur added 2 commits January 31, 2018 10:45

minishrink force-pushed the v6_fix branch from 0c11bc2 to cb84fa0 Compare January 31, 2018 10:47

mseri approved these changes Jan 31, 2018

View reviewed changes

mseri merged commit 109234a into xapi-project:master Jan 31, 2018

minishrink deleted the v6_fix branch January 31, 2018 13:04

Document and build daemon CLIs for debugging #198

Document and build daemon CLIs for debugging #198

Uh oh!

Conversation

minishrink commented Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mseri Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mseri Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minishrink Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mseri commented Jan 30, 2018

Uh oh!

mseri commented Jan 30, 2018

Uh oh!

mseri left a comment

Choose a reason for hiding this comment

Uh oh!

mseri commented Jan 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

minishrink commented Jan 30, 2018 •

edited

Loading

mseri Jan 30, 2018 •

edited

Loading

mseri Jan 30, 2018 •

edited

Loading

minishrink Jan 30, 2018 •

edited

Loading