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

Use latest version of mdbook in CI #21

Conversation

austinglaser
Copy link

@austinglaser austinglaser commented Nov 11, 2019

Fixes #20

Fixes #8

As discussed in rust-embedded/book#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
Copy link

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
Copy link
Author

austinglaser commented Nov 18, 2019

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
Copy link
Contributor

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

@austinglaser
Copy link
Author

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

Yes, but I was waiting for direct feedback (since rust-embedded/embedonomicon#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
Copy link
Contributor

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
Copy link
Author

austinglaser commented Nov 18, 2019

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 (rust-embedded/book#58, rust-embedded/book#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:

  • Take a static version (e.g. 0.3.5), and require a manual step to update it
    • Prevents mdbook regressions
    • Prevents mdbook API breakage
  • Take the latest patch version of a stable series (e.g. 0.3.z)
    • Does not prevent mdbook regressions
    • Prevents mdbook API breakage
  • Take the latest minor version (e.g. 0.y.z)
    • Does not prevent mdbook regressions
    • Does not prevent mdbook API breakage
  • Take the latest version regardless
    • Does not prevent mdbook regressions
    • Does not prevent mdbook API breakage

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
Copy link
Contributor

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
Copy link
Author

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 #20)

@therealprof
Copy link
Contributor

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.

@therealprof therealprof closed this Jul 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs are built with an old pinned version of mdbook Reminder: Back out hardcoded mdbook v0.2.1 version
5 participants