theseus-os/Theseus

`make view-book` creates book when it already exists

amab8901 opened this issue · 10 comments

This is probably not a priority. More like an aesthetics/convenience issue. But I think it's good to add it to the issues list so that we can solve it in the future in case we run out of ideas on how to improve the OS further.

Here's how the issue looks like:

$ make view-book

2022-12-27 12:24:48 [INFO] (mdbook::book): Book building has started
2022-12-27 12:24:48 [INFO] (mdbook::book): Running the html backend
2022-12-27 12:24:48 [INFO] (mdbook::book): Running the linkcheck backend
2022-12-27 12:24:48 [INFO] (mdbook::renderer): Invoking the "linkcheck" renderer
2022-12-27 12:24:48 [WARN] (mdbook::renderer): The command `mdbook-linkcheck` for backend `linkcheck` was not found, but was marked as optional.

The Theseus Book is now available at "/home/amab/src/Theseus/build/book/html/index.html".
Opening the Theseus book in your browser...
2022-12-27 12:24:48 [INFO] (mdbook::book): Book building has started
2022-12-27 12:24:48 [INFO] (mdbook::book): Running the html backend
2022-12-27 12:24:48 [INFO] (mdbook::book): Running the linkcheck backend
2022-12-27 12:24:48 [INFO] (mdbook::renderer): Invoking the "linkcheck" renderer
2022-12-27 12:24:48 [WARN] (mdbook::renderer): The command `mdbook-linkcheck` for backend `linkcheck` was not found, but was marked as optional.
2022-12-27 12:24:48 [INFO] (mdbook): Opening web browser

Ah, thanks for finding this! Yeah, it's a pretty easy fix, just remove this line in the Makefile:

@mdbook build --open $(BOOK_SRC) -d $(BOOK_OUT)

If you'd like to submit a PR that fixes it, I'd be happy to accept it. Otherwise I can make the change myself and credit you in the commit.

If you just remove that line, then it probably will misbehave if you didn't build a book since previous. It will just say "Opening the Theseus book in your browser..." but without actually doing it.

What I'm thinking of instead is that it should check whether there is a book or not. If there is a book, then use the existing one. But if there isn't a book, then build it.

I explored the documentation of mdbook, and it turns out that this issue needs to be solved first.

If you just remove that line, then it probably will misbehave if you didn't build a book since previous. It will just say "Opening the Theseus book in your browser..." but without actually doing it.

What I'm thinking of instead is that it should check whether there is a book or not. If there is a book, then use the existing one. But if there isn't a book, then build it.

That should already be handled by the fact that the view-book make target depends on the book target.

I explored the documentation of mdbook, and it turns out that this issue needs to be solved first.

Hmm, I don't think that issue is relevant to us since we're not using watch or any other long-running command.

Also, if you want to run a command in the background, most bash-like shells allow you to suffix a command with & to do so, e.g.,

mdbook watch ... & 
## continue using the shell here

I explored the documentation of mdbook, and it turns out that this issue needs to be solved first.

Hmm, I don't think that issue is relevant to us since we're not using watch or any other long-running command.

Also, if you want to run a command in the background, most bash-like shells allow you to suffix a command with & to do so, e.g.,

mdbook watch ... & 
## continue using the shell here

I was thinking of using watch as an alternative to build since it doesn't build what's already built

I tried the & at the end but it doesn't seem to solve the issue. It still ends up building the book when it's already built.

Actually, I think it's hard to solve this issue. It seems to be due to limitations in the mdbook. That project is developing over time and maybe they will solve it one day. I think we can disregard this issue for now. There's plenty of other issues we can focus on. Perhaps we may revisit it in a distant future when mdbook is different from what it is today.

I see, no worries if it's a problem with mdbook itself.

Btw, I think the problem you identified initially is still valid. Would you like to submit a fix?

So here's the code we're dealing with:

### Opens the Theseus book.
view-book: book
	@echo -e "Opening the Theseus book in your browser..."
	@mdbook build --open $(BOOK_SRC) -d $(BOOK_OUT)

I've been playing around with different alternatives to @mdbook build --open $(BOOK_SRC) -d $(BOOK_OUT). Here is what I tried:

$ mdbook help
Commands:
  init         Creates the boilerplate structure and files for a new book
  build        Builds a book from its markdown files
  test         Tests that a book's Rust code samples compile
  clean        Deletes a built book
  completions  Generate shell completions for your shell to stdout
  watch        Watches a book's files and rebuilds it on changes
  serve        Serves a book at http://localhost:3000, and rebuilds it on changes
  help         Print this message or the help of the given subcommand(s)

Here are my thoughts on the commands:

  • init: doesn't seem relevant since we're assuming the book is already available and we just want to browse it
  • build: this will make it build (and optionally open), but we're trying to read it without building.
  • test: this would replace one redundancy with another. Instead of computing/spamming about book creation, it will instead do the same about testing (probably)
  • clean: we're trying to read the book, not delete it
  • completions: I don't understand the description, but mdbook completions doesn't show any options for opening/browsing the book
  • watch: this may work
  • serve: this may work
  • help: this doesn't open the book we're trying to read

So now we're left with watch and serve as viable candidates. I'll play around with it more and update this comment until I fully covered all my options.

@Mdbook serve --open $(BOOK_SRC)
$ make view-book
2022-12-30 10:16:37 [INFO] (mdbook::book): Book building has started
2022-12-30 10:16:37 [INFO] (mdbook::book): Running the html backend
2022-12-30 10:16:38 [INFO] (mdbook::book): Running the linkcheck backend
2022-12-30 10:16:38 [INFO] (mdbook::renderer): Invoking the "linkcheck" renderer
2022-12-30 10:16:38 [WARN] (mdbook::renderer): The command `mdbook-linkcheck` for backend `linkcheck` was not found, but was marked as optional.

The Theseus Book is now available at "/home/amab/src/Theseus/build/book/html/index.html".
Opening the Theseus book in your browser...
# @mdbook build --open /home/amab/src/Theseus/book -d /home/amab/src/Theseus/build/book
2022-12-30 10:16:38 [INFO] (mdbook::book): Book building has started
2022-12-30 10:16:38 [INFO] (mdbook::book): Running the html backend
2022-12-30 10:16:38 [INFO] (mdbook::book): Running the linkcheck backend
2022-12-30 10:16:38 [INFO] (mdbook::renderer): Invoking the "linkcheck" renderer
2022-12-30 10:16:38 [WARN] (mdbook::renderer): The command `mdbook-linkcheck` for backend `linkcheck` was not found, but was marked as optional.
2022-12-30 10:16:38 [INFO] (mdbook::cmd::serve): Serving on: http://localhost:3000
2022-12-30 10:16:38 [INFO] (mdbook): Opening web browser
2022-12-30 10:16:38 [ERROR] (mdbook::cmd::serve): Unable to serve: panicked at 'error binding to [::1]:3000: error creating server listener: Address already in use (os error 98)', /home/amab/.cargo/registry/src/github.com-1ecc6299db9ec823/warp-0.3.3/src/server.rs:213:27
make: *** [Makefile:634: view-book] Error 1

@Mdbook serve --open
$ make view-book
2022-12-30 10:19:42 [INFO] (mdbook::book): Book building has started
2022-12-30 10:19:42 [INFO] (mdbook::book): Running the html backend
2022-12-30 10:19:42 [INFO] (mdbook::book): Running the linkcheck backend
2022-12-30 10:19:42 [INFO] (mdbook::renderer): Invoking the "linkcheck" renderer
2022-12-30 10:19:42 [WARN] (mdbook::renderer): The command `mdbook-linkcheck` for backend `linkcheck` was not found, but was marked as optional.

The Theseus Book is now available at "/home/amab/src/Theseus/build/book/html/index.html".
Opening the Theseus book in your browser...
# @mdbook build --open /home/amab/src/Theseus/book -d /home/amab/src/Theseus/build/book
2022-12-30 10:19:42 [ERROR] (mdbook::utils): Error: Couldn't open SUMMARY.md in "/home/amab/src/Theseus/src" directory
2022-12-30 10:19:42 [ERROR] (mdbook::utils):    Caused By: No such file or directory (os error 2)
make: *** [Makefile:634: view-book] Error 101

@Mdbook watch --open $(BOOK_SRC) &
$ make view-book
2022-12-30 10:22:48 [INFO] (mdbook::book): Book building has started
2022-12-30 10:22:48 [INFO] (mdbook::book): Running the html backend
2022-12-30 10:22:48 [INFO] (mdbook::book): Running the linkcheck backend
2022-12-30 10:22:48 [INFO] (mdbook::renderer): Invoking the "linkcheck" renderer
2022-12-30 10:22:48 [WARN] (mdbook::renderer): The command `mdbook-linkcheck` for backend `linkcheck` was not found, but was marked as optional.

The Theseus Book is now available at "/home/amab/src/Theseus/build/book/html/index.html".
Opening the Theseus book in your browser...
# @mdbook build --open /home/amab/src/Theseus/book -d /home/amab/src/Theseus/build/book

 ~/src/Theseus $  
2022-12-30 10:22:48 [INFO] (mdbook::book): Book building has started
2022-12-30 10:22:48 [INFO] (mdbook::book): Running the html backend
2022-12-30 10:22:49 [INFO] (mdbook::book): Running the linkcheck backend
2022-12-30 10:22:49 [INFO] (mdbook::renderer): Invoking the "linkcheck" renderer
2022-12-30 10:22:49 [WARN] (mdbook::renderer): The command `mdbook-linkcheck` for backend `linkcheck` was not found, but was marked as optional.
2022-12-30 10:22:49 [INFO] (mdbook): Opening web browser
2022-12-30 10:22:49 [INFO] (mdbook::cmd::watch): Listening for changes...

In each of these instances, it says Book building has started. I don't know what other options there are for me to try and solve this.

Ok I'm not exactly sure what your goal is, but it sounds like something completely separate from the initial problem you identified (the view-book target building the book twice).

Since make targets are currently used in CI passes, they need to be one or more commands that run to completion, not a process that runs persistently in the background like watch or serve. (Apologies if this is not what you're suggesting... I can't really tell.)

In any case, I've submitted a fix in #781.