Skip to content

invoke cargo to install mdbook #19

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

Merged
merged 3 commits into from
Jan 23, 2018
Merged

Conversation

nikomatsakis
Copy link
Contributor

cc @Michael-F-Bryan -- does this look right?

(I'm also not sure what the ~/.cargo/env command does, but it seemed likely it might be needed to run cargo, so I moved it up)

@nikomatsakis
Copy link
Contributor Author

cc #1

Copy link
Contributor

@Michael-F-Bryan Michael-F-Bryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking up on this. I've always found that travis is annoying to get working just right, and it often takes me a couple commits to master to ensure the entire system works as intended.

.travis.yml Outdated
- source ~/.cargo/env || true
- bash ci/install.sh
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops! Looks like I must have messed up the order when copy/pasting across from .travis.yml files in other repositories.

.travis.yml Outdated
@@ -3,8 +3,8 @@ language: rust
cache: pip

install:
- bash ci/install.sh
- source ~/.cargo/env || true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sure ~/.cargo/bin is on our $PATH (its contents is just export PATH="$HOME/.cargo/bin:$PATH"). If we didn't make sure ~/.cargo/bin is on the $PATH we'd end up with command not found errors.

ci/install.sh Outdated
@@ -1,6 +1,8 @@
#!/bin/bash
set -ex

cargo install cargo install mdbook --vers "0.0.28"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may want to wrap this in a if command -v ... block. To skip recompiling mdbook every time CI rins, you only want to install it if it isn't already installed. The if command -v foo bit returns 1 if the foo command isn't found and tends to be more reliable than using which foo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK :)

ci/install.sh Outdated
@@ -1,6 +1,8 @@
#!/bin/bash
set -ex

cargo install cargo install mdbook --vers "0.0.28"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming you meant cargo install mdbook ... here, not cargo install cargo install ....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, yeah =)

.travis.yml Outdated
@@ -3,8 +3,8 @@ language: rust
cache: pip
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you change this to

cache:
- pip
- cargo

It'll let us cache both the ghp-import install and the mdbook install, as well as the cargo registry and cache in ~/.cargo so we don't need to re-download the world every time CI runs.

@nikomatsakis
Copy link
Contributor Author

@Michael-F-Bryan take another look, if you please

@Michael-F-Bryan
Copy link
Contributor

It looks good from what I can see, although the only real way to find out is to merge this PR into master and trigger uploading the rendered document.

To tell GitHub that Travis is allowed to push to GitHub Pages we'll also need to add an API key (with public_repo permissions) and then add the encrypted key to travis.yml.

@nikomatsakis
Copy link
Contributor Author

@Michael-F-Bryan ok; where do I put the key in travis.yml exactly? It is under deploy, as I see here?

@nikomatsakis
Copy link
Contributor Author

I feel like mdbook should have step-by-step instructions for this =)

@Michael-F-Bryan
Copy link
Contributor

What I usually do is run travis encrypt GH_TOKEN=$TOKEN --add on the command-line and it'll add the token to travis.yml for you. It should end up looking something like this.

I feel like mdbook should have step-by-step instructions for this =)

I was thinking the exact same thing yesterday! Now that alternate backends have been merged into mdbook, I was thinking of making a backend which will automatically build your book and then push it up to GitHub Pages for you, printing detailed messages if anything is wrong with your environment. As part of that tool you'd mention how to set everything up in the README.

I've always found CI to be a massive pain to get set up, adding a bunch of noise to the git history because you'll typically need to make a commit to master push it up, wait 5-10 minutes for the build to fail, make a one-line change to see if you've fixed the issue, rinse and repeat. Appveyor is even worse because it only runs one build at a time for your entire project 😞

@nikomatsakis
Copy link
Contributor Author

OK, well, I'm going to merge this. We'll see what happens I suppose.

@nikomatsakis nikomatsakis merged commit 6792c63 into rust-lang:master Jan 23, 2018
david0u0 pushed a commit to david0u0/rustc-guide that referenced this pull request Aug 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants