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.

Sign up for GitHub

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

Jump to bottom

Port books to mdbook #39633

Merged
merged 8 commits into from
Feb 15, 2017
Merged

Port books to mdbook #39633

merged 8 commits into from
Feb 15, 2017

Conversation

steveklabnik
Copy link
Member

@steveklabnik steveklabnik commented Feb 8, 2017 •
edited
Loading

Part of #39588

blocked on #39431

As a first step towards the bookshelf, we vendor mdbook in-tree and port our books to it. Eventually, both of these books will be moved out-of-tree, but the nightly book will rely on doing the same thing. As such, this intermediate step is useful.

r? @alexcrichton @brson

/cc @azerupi

@steveklabnik steveklabnik changed the title Bring mdbook in-tree and port our books to it Vendor mdbook in-tree and port our books to it Feb 8, 2017
@steveklabnik
Copy link
Member Author

Oh, and pour one out for rustbook; you had a good run. /cc @aturon

@alexcrichton
Copy link
Member

Looking good! I take it that the src/tools/mdBook directory is literally the source code of mdBook? If so, could we perhaps use a submodule instead?

Also, I think that you'll need to re-run cargo vendor to pull in mdBook's dependencies as well. Ideally though we'd only pull in a small number (e.g. the non-default ones). By default though a workspace member pulls in all optional dependencies and we don't have a way to turn this off. Would it be easy to depend on mdbook as a library and write a small wrapper that emulates https://github.com/azerupi/mdBook/blob/master/src/bin/mdbook.rs perhaps?

@steveklabnik
Copy link
Member Author

If so, could we perhaps use a submodule instead?

Ah ha! I always forget about submodules; totally.

By default though a workspace member pulls in all optional dependencies and we don't have a way to turn this off. Would it be easy to depend on mdbook as a library and write a small wrapper that emulates https://github.com/azerupi/mdBook/blob/master/src/bin/mdbook.rs perhaps?

yes, that shouldn't be too bad. It's a tiny file.

@alexcrichton
Copy link
Member

Ok in that case let's scratch the submodule idea and instead just depend on it as a library, vendor a small bit of the book logic, and then we can rely on cargo-vendor to pull in the vendor'd deps

@steveklabnik
Copy link
Member Author

@alexcrichton I guess I'll call this wrapper........... rustbook. :trollface:

IT LIVES

@steveklabnik
Copy link
Member Author

@alexcrichton I think I did this correctly...

@frewsxcv
Copy link
Member

frewsxcv commented Feb 8, 2017

Are all these fingerprint files intentional? I don't really know what they are, so maybe they are intentional?

@steveklabnik
Copy link
Member Author

@frewsxcv not sure, i just ran cargo vendor.

@steveklabnik
Copy link
Member Author

Hrm, looks like maybe the vendoring didn't work? https://travis-ci.org/rust-lang/rust/jobs/199465089#L371

@est31
Copy link
Member

est31 commented Feb 8, 2017

I'm not sure, but I feel like this file shouldn't be there: https://github.com/steveklabnik/rust/blob/041312e1909be2954852317339629715d314e045/src/target/release/.fingerprint/kernel32-sys-4995bb2d2dfb906c/dep-build-script-build_script_build-4995bb2d2dfb906c

maybe some bug in cargo vendor?

@steveklabnik
Copy link
Member Author

@est31 @frewsxcv it looks like cargo new didn't give me a .gitignore; ignoring that now

@steveklabnik
Copy link
Member Author

@alexcrichton i added a src/.gitignore that ignores target; lmk if that's wrong

@steveklabnik
Copy link
Member Author

It is unclear to me why this works for me locally but fails on travis :(

@steveklabnik
Copy link
Member Author

@est31 points out to me on IRC that the bots are using make still, it seems, so that's probably why.

@bors
Copy link
Contributor

bors commented Feb 8, 2017

☔ The latest upstream changes (presumably #39638) made this pull request unmergeable. Please resolve the merge conflicts.

@azerupi
Copy link
Contributor

azerupi commented Feb 8, 2017

Would it be easy to depend on mdbook as a library and write a small wrapper that emulates

mdBook is designed to be able to be used as a library, but I think you are the first ones actually using it like that 😉 So don't hesitate to give feedback so that we can improve that use case.

@steveklabnik
Copy link
Member Author

I am extremely confused.

$ git push origin -f vendor-mdbook
Counting objects: 778, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (248/248), done.
Writing objects: 100% (543/543), 726.68 KiB | 0 bytes/s, done.
Total 543 (delta 353), reused 358 (delta 283)
remote: Resolving deltas: 100% (353/353), completed with 152 local objects.
To git@github.com:steveklabnik/rust
 + bb66fa9...3812b66 vendor-mdbook -> vendor-mdbook (forced update)
steve@BECOMING:~/src/rust$ git push origin -f vendor-mdbook
Everything up-to-date

yet the update doesn't show up here. hmmmmmmmmmmmmmmmmmmm

@steveklabnik
Copy link
Member Author

It looks like github is having some issues; I assume this PR will be updated soon, then.

@steveklabnik
Copy link
Member Author

Okay, I don't know why the bot is failing here; I can run make tidy on my local machine. @alexcrichton any ideas?

(It fails with license issues, but it doesn't fail like it does on travis)

@steveklabnik
Copy link
Member Author

Here is the summary of license issues:

invalid license Unlicense/MIT in /home/steve/src/rust/src/vendor/aho-corasick/Cargo.toml
invalid license MIT in /home/steve/src/rust/src/vendor/ansi_term/Cargo.toml
invalid license MIT in /home/steve/src/rust/src/vendor/clap/Cargo.toml
invalid license MIT in /home/steve/src/rust/src/vendor/handlebars/Cargo.toml
invalid license MIT in /home/steve/src/rust/src/vendor/kernel32-sys/Cargo.toml
invalid license MIT in /home/steve/src/rust/src/vendor/lazy_static/Cargo.toml
invalid license Unlicense/MIT in /home/steve/src/rust/src/vendor/memchr/Cargo.toml
invalid license MIT in /home/steve/src/rust/src/vendor/open/Cargo.toml
invalid license MPL-2.0 in /home/steve/src/rust/src/vendor/pest/Cargo.toml
invalid license MIT in /home/steve/src/rust/src/vendor/pulldown-cmark/Cargo.toml
invalid license MIT OR Apache-2.0 in /home/steve/src/rust/src/vendor/quick-error/Cargo.toml
invalid license MIT in /home/steve/src/rust/src/vendor/strsim/Cargo.toml
invalid license Apache-2.0 in /home/steve/src/rust/src/vendor/thread-id/Cargo.toml
invalid license Apache-2.0/MIT in /home/steve/src/rust/src/vendor/thread_local/Cargo.toml
invalid license Unlicense/MIT in /home/steve/src/rust/src/vendor/utf8-ranges/Cargo.toml
invalid license MIT in /home/steve/src/rust/src/vendor/winapi/Cargo.toml
invalid license MIT in /home/steve/src/rust/src/vendor/winapi-build/Cargo.toml

So, that's

  • Unlicense
  • MIT
  • MPL-2.0
  • Apache-2.0

All of the Unlicense projects are dual with MIT, so the real issue is pest, I think. Ironic that MPL would be the issue here 😉

@azerupi
Copy link
Contributor

azerupi commented Feb 8, 2017

so the real issue is pest, I think. Ironic that MPL would be the issue here

mdBook is also licensed under the MPL 2.0, is that a problem?

@steveklabnik
Copy link
Member Author

@azerupi interesting, wonder why it wasn't caught by the lint.

We're going to discuss it at the core team meeting today; it's not inherently a problem, but right now, everything in the repo is licensed identically. Moving away from that would be a big change, possibly.

@steveklabnik
Copy link
Member Author

steveklabnik commented Feb 8, 2017 •
edited
Loading

Updating myself: it appears that I did not run ./configure --enable-vendor when building, so now the travis issue reproduces for me. hrm.

It's not clear why, with the vendor, it says that it's looking at crates.io to get the source and can't find it.

@steveklabnik
Copy link
Member Author

@azerupi expect a post on internals sometime in the near-ish future; we're going to ask everyone what they think 😄

@steveklabnik steveklabnik mentioned this pull request Feb 8, 2017
Closed
10 tasks
@steveklabnik
Copy link
Member Author

Internals post is here: https://internals.rust-lang.org/t/licenses-of-vendored-tools-in-the-rust-repo/4758

@bors
Copy link
Contributor

bors commented Feb 8, 2017

☔ The latest upstream changes (presumably #39523) made this pull request unmergeable. Please resolve the merge conflicts.

bors added a commit that referenced this pull request Feb 14, 2017
@bors
Auto merge of #39728 - eddyb:vendeur-tres-bien, r=alexcrichton
c03f12e 
Automate vendoring by invoking cargo-vendor when building src dist tarballs.

This avoids #39633 bringing the `src/vendor` checked into git by #37524, past 200,000 lines of code.

I believe the strategy of having rustbuild run `cargo vendor` during the `dist src` step is sound.

However, the only way to be sure `cargo-vendor` exists is to run `cargo install --force cargo-vendor`, which will recompile it every time (not passing `--force` means you can't tell between "already exists" and "build error"). ~~This is quite suboptimal and I'd like to somehow do it in each `Dockerfile` that would need it.~~

* [ ] Cache `CARGO_HOME` (i.e. `~/.cargo`) between CI runs
  * `bin/cargo-vendor` and the actual caches are the relevant bits
* [x] Do not build `cargo-vendor` all the time
  * ~~Maybe detect `~/.cargo/bin/cargo-vendor` already exists?~~
  * ~~Could also try to build it in a `Dockerfile` but do we have `cargo`/`rustc` there?~~
  * Final solution: check `cargo install --list` for a line starting with `cargo-vendor `

cc @rust-lang/tools
bors added a commit that referenced this pull request Feb 14, 2017
@bors
Auto merge of #39728 - eddyb:vendeur-tres-bien, r=alexcrichton
48bc082 
Automate vendoring by invoking cargo-vendor when building src dist tarballs.

This avoids #39633 bringing the `src/vendor` checked into git by #37524, past 200,000 lines of code.

I believe the strategy of having rustbuild run `cargo vendor` during the `dist src` step is sound.

However, the only way to be sure `cargo-vendor` exists is to run `cargo install --force cargo-vendor`, which will recompile it every time (not passing `--force` means you can't tell between "already exists" and "build error"). ~~This is quite suboptimal and I'd like to somehow do it in each `Dockerfile` that would need it.~~

* [ ] Cache `CARGO_HOME` (i.e. `~/.cargo`) between CI runs
  * `bin/cargo-vendor` and the actual caches are the relevant bits
* [x] Do not build `cargo-vendor` all the time
  * ~~Maybe detect `~/.cargo/bin/cargo-vendor` already exists?~~
  * ~~Could also try to build it in a `Dockerfile` but do we have `cargo`/`rustc` there?~~
  * Final solution: check `cargo install --list` for a line starting with `cargo-vendor `

cc @rust-lang/tools
@steveklabnik
Copy link
Member Author

@eddyb's PR has landed, 🎊

@bors: r=alexcrichton

@bors
Copy link
Contributor

bors commented Feb 14, 2017

📌 Commit cacb3bc has been approved by alexcrichton

@steveklabnik
Copy link
Member Author

@bors: p=1

I'm giving this a priority because there are a few PRs to the book that will need to be rebased after this PR, and this PR has already been sitting in the queue for three days.

@bors
Copy link
Contributor

bors commented Feb 14, 2017

⌛ Testing commit cacb3bc with merge f19eb11...

@bors
Copy link
Contributor

bors commented Feb 14, 2017

💔 Test failed - status-travis

@steveklabnik
Copy link
Member Author

@bors: retry

@bors
Copy link
Contributor

bors commented Feb 14, 2017

⌛ Testing commit cacb3bc with merge c3ab706...

@bors
Copy link
Contributor

bors commented Feb 14, 2017

💔 Test failed - status-travis

@alexcrichton
Copy link
Member

alexcrichton commented Feb 14, 2017 via email

@bors
Copy link
Contributor

bors commented Feb 15, 2017

⌛ Testing commit cacb3bc with merge 025c328...

bors added a commit that referenced this pull request Feb 15, 2017
@bors
Auto merge of #39633 - steveklabnik:vendor-mdbook, r=alexcrichton
025c328 
Port books to mdbook

Part of #39588

blocked on #39431

As a first step towards the bookshelf, we ~vendor mdbook in-tree and~ port our books to it. Eventually, both of these books will be moved out-of-tree, but the nightly book will rely on doing the same thing. As such, this intermediate step is useful.

r? @alexcrichton @brson

/cc @azerupi
@bors
Copy link
Contributor

bors commented Feb 15, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 025c328 to master...

@bors bors merged commit cacb3bc into rust-lang:master Feb 15, 2017
This was referenced Feb 15, 2017
Merged
Merged
Merged
Merged
Merged
Closed
Closed
@steveklabnik steveklabnik deleted the vendor-mdbook branch February 15, 2017 15:58
@hoodie
Copy link
Contributor

hoodie commented Feb 23, 2017

could we perhaps switch back to the rustbook color scheme?

@jameslzhu jameslzhu mentioned this pull request Jul 20, 2017
Merged
@kennytm kennytm mentioned this pull request Aug 25, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@est31 est31 est31 left review comments

@alexcrichton alexcrichton alexcrichton left review comments

Assignees

@alexcrichton alexcrichton

Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.