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

Check the docs as part of check-stageN #31689

Closed
wants to merge 2 commits into from

Conversation

james-w
Copy link

@james-w james-w commented Feb 15, 2016

This addresses #31587 by adding check-stage2-docs as a dependency
of make check.

With this change make check output shows

LD_LIBRARY_PATH=/home/jw2328/devel/rust/x86_64-unknown-linux-gnu/stage2/lib:/home/jw2328/devel/rust/x86_64-unknown-linux-gnu/llvm/Release/lib:$LD_LIBRARY_PATH x86_64-unknown-linux-gnu/stage2/bin/rustdoc --test doc/error-index.md

running 453 tests
test Rust_Compiler_Error_Index_0 ... ok
test Rust_Compiler_Error_Index_100 ... ok
test Rust_Compiler_Error_Index_1 ... ok
test Rust_Compiler_Error_Index_10 ... ok
test Rust_Compiler_Error_Index_103 ... ok
test Rust_Compiler_Error_Index_101 ... ok
test Rust_Compiler_Error_Index_105 ... ok
test Rust_Compiler_Error_Index_104 ... ok
...

I'm not entirely sure this is the right fix, we may instead want to depend
on a slightly more specific target, but I'm not sure what the
expected behaviour of each target is. Running all the docs tests
on make check seems perfectly reasonable, and the dependencies
should mean there is no duplicated work.

If you do suggest a change I would appreciate it if you could also
let me know if there's a trick to avoid the process from deleting
all the build artefacts if the makefiles are changed. A simple tweak
to this rule means that I have to rebuild all of rust to test it. If there's
a way to avoid running get-snapshot then I would like to know,
because NO_REBUILD=1 doesn't seem to be it.

This is my first PR for rust, so I likely missed some simple things
(commit message template, NEWS entry?) Please let me know
what they are and I will happily adjust my submission.

Thanks,

James

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (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.

@james-w
Copy link
Author

james-w commented Feb 15, 2016

Looking again it looks like check-notidy probably wants this adding as well right? Do you
know why check is not defined as

check: check-notidy tidy

?

@james-w
Copy link
Author

james-w commented Feb 16, 2016

To answer my own question, avoiding a full rebuild on Makefile changes can
be done with

$ make check NO_REBUILD=1 NO_MKFILE_DEPS=1

@alexcrichton
Copy link
Member

Perhaps this might be most appropriate behind the check-stage2 rule rather than check? That way make check-stage1 would also run the stage1 doctests

@james-w james-w changed the title Check the full stage2-docs suite on make check Check the docs as part of check-stageN Feb 16, 2016
@james-w
Copy link
Author

james-w commented Feb 16, 2016

Hi, that's a good idea, I've updated the branch to add check-stageN-docs to
the deps for check-stageN, so each stage will run the doc tests.

@petrochenkov
Copy link
Contributor

IIRC, something was wrong with check-stage1-docs.
Could you verify that it works as expected and doesn't build anything excessive (stage2 for example)?

@james-w
Copy link
Author

james-w commented Feb 16, 2016

Ah, it looks like it might build the error generator from stage2.

@alexcrichton
Copy link
Member

In the case of make check-stage1, does it then build a bunch of stage2 artifacts? It may just be a bug in where the error index generator comes from (hardwired to stage2 when it maybe should be a parameter)

@james-w
Copy link
Author

james-w commented Feb 18, 2016

https://gist.github.com/james-w/6d88ee407f2fd180c04d is the output of make check-stage1 with this change. It looks like it is building some stage2 libraries?

@brson
Copy link
Contributor

brson commented Feb 20, 2016

These doc dependencies are also specified here. I think what should be done is that the new -doc-error-index-exec rule should be added to that.

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to resubmit with comments addressed!

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.

5 participants