Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Look at mdbook-keeper #175

Open
mgeisler opened this issue Jan 17, 2023 · 7 comments
Open

Look at mdbook-keeper #175

mgeisler opened this issue Jan 17, 2023 · 7 comments
Labels
good first issue Good for newcomers

Comments

@mgeisler
Copy link
Collaborator

We currently have a number of code blocks marked with compile_fail: this is often because they depend on third-party crates which aren't available when running mdbook test.

We should look into using https://github.com/tfpk/mdbook-keeper/ to handle testing instead. It explicitly supports using third-party crates in code snippets.

@mgeisler mgeisler added help wanted Extra attention is needed good first issue Good for newcomers and removed help wanted Extra attention is needed labels Feb 10, 2023
djmitche added a commit to djmitche/comprehensive-rust that referenced this issue Mar 22, 2023
`mdbook test` does not allow code samples to reference other crates, so
they must be marked as `compile_fail`; see google#175.
djmitche added a commit that referenced this issue Mar 22, 2023
NOTE: `mdbook test` does not allow code samples to reference other crates, so
they must be marked as `compile_fail`; see #175.
djmitche added a commit that referenced this issue Apr 14, 2023
* beginning of an Async section

* address review comments

* Add futures page (#497)

NOTE: `mdbook test` does not allow code samples to reference other crates, so
they must be marked as `compile_fail`; see #175.

* Add Runtimes & Tasks (#522)

These concepts are closely related, and there's not much else to know
about runtimes other than "they exist".

This removes the bit about futures being "inert" because it doesn't
really lead anywhere.

* Async chapter (#524)

* Add async channels chapter

* Async control flow

* Async pitfalls

* Separate in multiple chapters + add daemon section

* Merge reentering threads in blocking-executor

* async_trait

* Async fixes (#546)

* Async: some ideas for simplifying the content (#550)

* Simplify the async-await slide
* Shorten futures and move it up
* Add a page on Tokio

* Modifications to the async section (#556)

* Modifications to the async section

* Remove the "Daemon" slide, as it largely duplicates the "Tasks" slide.
  The introduction to the "Control Flow" section mentions tasks as a
  kind of control flow.

* Reorganize the structure in SUMMARY.md to correspond to the directory
  structure.

* Simplify the "Pin" and "Blocking the Executor" slides with steps in
  the speaker notes to demonstrate / fix the issues.

* Rename "join_all" to "Join".

* Simplify some code samples to shorten them, and to print output rather
  than asserting.

* Clarify speaker notes and include more "Try.." suggestions.

* Be consistent about where `async` blocks are introduced (in the
  "Tasks" slide).

* Explain `join` and `select` in prose.

* Fix formatting of section-header slides.

* Add a note on async trait (#558)

---------

Co-authored-by: sakex <alexandre@senges.ch>
Co-authored-by: rbehjati <razieh@google.com>
@mgeisler
Copy link
Collaborator Author

This is of course a bit of an open ended issue 🙂 What I hope for is that someone will look at what mdbook keeper can do for us:

  • can it run our current tests?
  • can we use it to run tests with more dependencies?

The goal would be remove the compile_fail from our code blocks.

If you can get this working locally, then we can work on a updating the GitHub action which runs the tests for us on every PR.

@mauvealerts
Copy link
Contributor

I started looking into this. Some issues/notes so far:

On my machine, it takes ~3min to run the tests which is pretty rough when I just want to mdbook serve. I think there's hope, potentially using:

I intend to work on this some more. In case my work stalls out, or anyone's curious, my keeper branch has the WIP.

@mgeisler
Copy link
Collaborator Author

mgeisler commented Jun 2, 2023

Thanks @mauvealerts! That's already a very valuable analysis! For example, I had not realized that you run the tests all the time... so we would definitely only enable it on demand like we do for the mdbook-xgettext renderer.

@mauvealerts
Copy link
Contributor

mauvealerts commented Jun 4, 2023

I have something that I'm fairly happy with, when combined with all of these changes to mdbook-keeper

I have things running comparable to mdbook test. IMO the steps necessary to make a mergable PR are:

  • Decide where the skeleton project should go (currently mdbook-keeper-deps)
  • Don't run tests by default for mdbook serve
  • mdbook-keeper has released a version with the changes
  • Prove out the usefulness by testing some snippets

@mgeisler
Copy link
Collaborator Author

mgeisler commented Jun 9, 2023

Thanks for pushing this!

  • Decide where the skeleton project should go (currently mdbook-keeper-deps)

I think this should be the root of the repository, or perhaps the src/ folder. We already have a Cargo workspace setup so that cargo check does something useful for the code in our exercises. I hope we can build on this for the book itself: perhaps we can move up the src/exercises/Cargo.toml file to src/ and then use its dependencies across the examples?

@mauvealerts
Copy link
Contributor

I didn't think it was possible to have both a [workspace] and (e.g.) a lib in the same Cargo.toml, but apparently it is. I've updated my branch to just use Cargo.toml in the root directory, at least for now. I've also locked mdbook-keeper to the markdown output format, so it doesn't slow down mdbook serve

I started updating some examples, but ran into link errors (tfpk/mdbook-keeper#9). In particular, not being able to use tokio on Windows makes me hesitant to migrate the async examples (which was most of what I planned to do).

@mgeisler
Copy link
Collaborator Author

I've also locked mdbook-keeper to the markdown output format, so it doesn't slow down mdbook serve

Ah, that's a clever work-around 😄 However, it almost suggests that mdbook-keeper should be a renderer instead of a preprocessor. @tfpk, have you considered that approach?

I started updating some examples, but ran into link errors (tfpk/mdbook-keeper#9). In particular, not being able to use tokio on Windows makes me hesitant to migrate the async examples (which was most of what I planned to do).

Oh, I see... I agree that it would be important to have test coverage for all the small code snippets involving Tokio and that was also my main reason to look at mdbook-keeper 🙂

NoahDragon pushed a commit to wnghl/comprehensive-rust that referenced this issue Jul 19, 2023
* beginning of an Async section

* address review comments

* Add futures page (google#497)

NOTE: `mdbook test` does not allow code samples to reference other crates, so
they must be marked as `compile_fail`; see google#175.

* Add Runtimes & Tasks (google#522)

These concepts are closely related, and there's not much else to know
about runtimes other than "they exist".

This removes the bit about futures being "inert" because it doesn't
really lead anywhere.

* Async chapter (google#524)

* Add async channels chapter

* Async control flow

* Async pitfalls

* Separate in multiple chapters + add daemon section

* Merge reentering threads in blocking-executor

* async_trait

* Async fixes (google#546)

* Async: some ideas for simplifying the content (google#550)

* Simplify the async-await slide
* Shorten futures and move it up
* Add a page on Tokio

* Modifications to the async section (google#556)

* Modifications to the async section

* Remove the "Daemon" slide, as it largely duplicates the "Tasks" slide.
  The introduction to the "Control Flow" section mentions tasks as a
  kind of control flow.

* Reorganize the structure in SUMMARY.md to correspond to the directory
  structure.

* Simplify the "Pin" and "Blocking the Executor" slides with steps in
  the speaker notes to demonstrate / fix the issues.

* Rename "join_all" to "Join".

* Simplify some code samples to shorten them, and to print output rather
  than asserting.

* Clarify speaker notes and include more "Try.." suggestions.

* Be consistent about where `async` blocks are introduced (in the
  "Tasks" slide).

* Explain `join` and `select` in prose.

* Fix formatting of section-header slides.

* Add a note on async trait (google#558)

---------

Co-authored-by: sakex <alexandre@senges.ch>
Co-authored-by: rbehjati <razieh@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants