-
Notifications
You must be signed in to change notification settings - Fork 543
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
Conversation
cc #1 |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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 ...
.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
@Michael-F-Bryan take another look, if you please |
It looks good from what I can see, although the only real way to find out is to merge this PR into To tell GitHub that Travis is allowed to push to GitHub Pages we'll also need to add an API key (with |
@Michael-F-Bryan ok; where do I put the key in travis.yml exactly? It is under |
I feel like |
What I usually do is run
I was thinking the exact same thing yesterday! Now that alternate backends have been merged into 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 |
OK, well, I'm going to merge this. We'll see what happens I suppose. |
Reset CI token
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)