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

Fix broken links in the site #2601

Merged
merged 2 commits into from Jul 15, 2014
Merged

Fix broken links in the site #2601

merged 2 commits into from Jul 15, 2014

Conversation

@alfredxing
Copy link
Member

@alfredxing alfredxing commented Jul 14, 2014

This PR fixes broken links found with html-proofer as suggested in #2593. I didn't attempt to fix any links to user/contributor content (like missing GitHub users or missing repos), nor any sites that simply timed out (the server might just be down).

There also seemed to have been an issue with some anchors linking to the History page from new releases blog posts:

./_site/news/2014/03/24/jekyll-1-5-0-released/index.html: linking to /docs/history/#150__20140324, but 150__20140324 does not exist
./_site/news/releases/index.html: linking to /docs/history/#150__20140324, but 150__20140324 does not exist
./_site/news/releases/index.html: linking to /docs/history/#103__20130607, but 103__20130607 does not exist
./_site/news/releases/index.html: linking to /docs/history/#102__20130512, but 102__20130512 does not exist
./_site/news/releases/index.html: linking to /docs/history/#101__20130508, but 101__20130508 does not exist
./_site/news/releases/index.html: linking to /docs/history/#100__20130506, but 100__20130506 does not exist
./_site/news/index.html: linking to /docs/history/#150__20140324, but 150__20140324 does not exist
./_site/news/index.html: linking to /docs/history/#103__20130607, but 103__20130607 does not exist
./_site/news/index.html: linking to /docs/history/#102__20130512, but 102__20130512 does not exist
./_site/news/index.html: linking to /docs/history/#101__20130508, but 101__20130508 does not exist
./_site/news/index.html: linking to /docs/history/#100__20130506, but 100__20130506 does not exist
./_site/news/2013/06/07/jekyll-1-0-3-released/index.html: linking to /docs/history/#103__20130607, but 103__20130607 does not exist
./_site/news/2013/05/12/jekyll-1-0-2-released/index.html: linking to /docs/history/#102__20130512, but 102__20130512 does not exist
./_site/news/2013/05/08/jekyll-1-0-1-released/index.html: linking to /docs/history/#101__20130508, but 101__20130508 does not exist
./_site/news/2013/05/05/jekyll-1-0-0-released/index.html: linking to /docs/history/#100__20130506, but 100__20130506 does not exist

Running html-proofer in the Travis build would also be possible, but that would also mean that an error in the site would cause the whole project to assume a "build failing" status.

alfredxing added 2 commits Jul 14, 2014
Fix broken link to Textile site to point to RedCloth
@parkr
Copy link
Member

@parkr parkr commented Jul 15, 2014

Thanks!

Running html-proofer in the Travis build would also be possible, but that would also mean that an error in the site would cause the whole project to assume a "build failing" status.

It's something we've looked into. Ideally, I'd ask git to list the files changed in this pull request, and only run html-proofer if any files in the site directory were modified. That's the ideal solution. We'd want to put installation & running it into one script.

@alfredxing
Copy link
Member Author

@alfredxing alfredxing commented Jul 15, 2014

Getting a diff of files since the last commit isn't difficult (something like git diff --name-only origin $(git log --pretty=format:"%h" -2 | tail -1) would do).

I'm not too sure on how Travis works exactly; how does it separate the pull requests from commits (i.e. will the line above work in all cases, or will we have to do different things for each situation?)

@doktorbro
Copy link
Member

@doktorbro doktorbro commented Jul 15, 2014

I'd ask git to list the files changed in this pull request, and only run html-proofer if any files in the site directory were modified.

@parkr And only install Proofer if any files were modified. The installation time was too long for you in #2311

@parkr
Copy link
Member

@parkr parkr commented Jul 15, 2014

@alfredxing That works for one commit, but doesn't get the whole PR, AFAICT. For reference, here's what Travis does:

$ git clone --depth=50 git://github.com/jekyll/jekyll.git jekyll/jekyll
$ cd jekyll/jekyll
$ git fetch origin +refs/pull/2601/merge: 
From git://github.com/jekyll/jekyll
 * branch            refs/pull/2601/merge -> FETCH_HEAD
$ git checkout -qf FETCH_HEAD

@penibelst Yeah, I really don't want to make testing slow down. Installing Nokogiri takes a long time, even when using the system libraries.

@parkr parkr merged commit 7f48331 into jekyll:master Jul 15, 2014
1 check passed
@alfredxing
Copy link
Member Author

@alfredxing alfredxing commented Jul 15, 2014

@parkr I set up a test repo, and the command seems to work for both commits and PR's (regardless of how many commits there are in a PR).
In the case of multi-commit PR's, I believe it works because it compares the tree of the last commit made in the PR with origin. This should result in a list of all changes made in the PR.
The script doesn't "work" for pull request merges, but I guess in the case there's really no need to run the tests again if they already ran in the PR.

Travis results from the test repo:

@parkr
Copy link
Member

@parkr parkr commented Jul 15, 2014

The script doesn't "work" for pull request merges, but I guess in the case there's really no need to run the tests again if they already ran in the PR.

👍 Awesome! Want to submit a pr? Please call it script/proof. It should:

  1. Check if site files were modified. Quickly exit 0 if none were. If any were, continue.
  2. Check to make sure html-proofer is installed. Install it (with NOKOGIRI_USE_SYSTEM_LIBRARIES=true set) if it's not installed. You can do this via gem list or the preferred command -v htmlproof.
  3. Run jekyll build -s site -d _site
  4. Run htmlproof ./_site

I think it would be fine if this was run inside script/cibuild (but the proofing script should be separate).

@alfredxing
Copy link
Member Author

@alfredxing alfredxing commented Jul 16, 2014

PR submitted! 🍴

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants