search

rust-embedded / rust-embedded.github.io

A collection of books and other documents about embedded Rust
https://docs.rust-embedded.org
Apache License 2.0
84 stars 8 forks source link

Use latest version of mdbook in CI #21

Closed austinglaser closed 4 years ago

austinglaser commented 5 years ago

Fixes #20

Fixes #8

As discussed in https://github.com/rust-embedded/book/pull/212, this is causing rendering differences between the rendered book from https://docs.rust-embedded.org/book/ and https://rust-embedded.github.io/book/.

Probably this shouldn't be merged until the reference PR is, as without it attributes will be erroneously hidden in some examples.

rust-highfive commented 5 years ago

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @adamgreig (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

austinglaser commented 4 years ago

Probably this shouldn't be merged until the reference PR is, as without it attributes will be erroneously hidden in some examples.

This is no longer the case -- mdbook 0.3.5 fixed the issue with atttributes

therealprof commented 4 years ago

@austinglaser Doesn't this one have the same regex problem as https://github.com/rust-embedded/debugonomicon/commit/c6cba56f16a28a9c11cc0f299cbe8d9a61d626dc originally?

austinglaser commented 4 years ago

Doesn't this one have the same regex problem as rust-embedded/debugonomicon@c6cba56 originally?

Yes, but I was waiting for direct feedback (since https://github.com/rust-embedded/embedonomicon/pull/59 was merged without comment, I was assuming that there might be different requirements regarding stability for each).

I'm happy to open PRs to make this consistent across all rust-embedded books, though, if we want all to be pinned to 0.x.y versions of mdbook.

But is that really what we want? If the goal is to avoid breaking changes, we should instead pin to 0.3.x versions. (not trying to ask a rhetorical question here, just honestly curious)

therealprof commented 4 years ago

Pinning to 0.3 is not going to prevent breakage. 0.2.2 was the breaking version so pinning to 0.2.x wouldn't have done anything. Using 0.2.1 specifically was a stopgap measure.

austinglaser commented 4 years ago

I'm talking about API changes, not bugs. Certainly we won't prevent breakage if a buggy version of mdbook is released -- I don't think anything short of pinning to a static version could do that (as was the original case).

Regressions do demonstrably happen (https://github.com/rust-embedded/book/pull/58, https://github.com/rust-embedded/book/issues/211).

But limiting to a semver series that shouldn't contain any breaking API changes could also be a reason to avoid blindly using the latest. 0.4 could bring breaking changes relative to 0.3, as could an eventual 1.0 release.

I see three strategies for selecting an mdbook version for a build:

From that standpoint, I don't see a strong reason to require across-the-board updates when mdbook 1.0 comes out if it doesn't gain us any safety over just letting the version freely update to the latest.

therealprof commented 4 years ago

From where I'm standing, there's not a whole lot of API which could break; it more or less boils down to HTML quality and design as well as regressions.

I'm not sure whether it's more cumbersome to fix any fallout or to manually fire off a series of PRs to bump the version (and making sure they don't cause breakage) every now and then.

austinglaser commented 4 years ago

I'm not sure whether it's more cumbersome to fix any fallout or to manually fire off a series of PRs to bump the version (and making sure they don't cause breakage) every now and then.

If we allow it to just take the latest, there's a possibility for that series of PRs to fix potential breakage. If we pin it in any way, there will definitely be a series of PRs any time it needs to be manually bumped.

I agree about your point regarding the (lack of) value in API stability as well. Which pushes me more in the direction of "just use the latest."

(also apologies for the duplication between this thread and https://github.com/rust-embedded/docs/issues/20)

therealprof commented 4 years ago

We merged #24 which is essentially the same but had an up-to-date CI build attached and was manually inspected to work now. Kudos for your work @austinglaser.