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

Actually add rustc-guide to toolstate, don't fail builds for the guide #62759

Merged
merged 12 commits into from
Jul 28, 2019
Merged

Actually add rustc-guide to toolstate, don't fail builds for the guide #62759

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
0fdf24b
actually test rustc-guide with toolstate
mark-i-m Jul 17, 2019
97b4156
don't fail builds for rustc-guide
mark-i-m Jul 17, 2019
8070bb8
Regressions ok for miri too
mark-i-m Jul 18, 2019
b2d05db
add a few comments for the toolstate stuff
RalfJung Jul 18, 2019
8b87162
Update src/ci/docker/x86_64-gnu-tools/checktools.sh
mark-i-m Jul 18, 2019
17c4084
Update src/ci/docker/x86_64-gnu-tools/checkregression.py
mark-i-m Jul 18, 2019
8940a27
embedded-book failures don't block beta
mark-i-m Jul 18, 2019
1aa1079
Update src/ci/docker/x86_64-gnu-tools/checkregression.py
mark-i-m Jul 18, 2019
9c48ed4
more comments for toolstate scripts
RalfJung Jul 23, 2019
92d432a
more callback docs
RalfJung Jul 23, 2019
82d1841
more comments
RalfJung Jul 23, 2019
11a3b74
add back check for update prs
mark-i-m Jul 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/ci/docker/x86_64-gnu-tools/checkregression.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@
import sys
import json

# Regressions for these tools do not cause failure.
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
REGRESSION_OK = ["rustc-guide", "miri", "embedded-book"]

if __name__ == '__main__':
os_name = sys.argv[1]
toolstate_file = sys.argv[2]
Expand Down Expand Up @@ -32,7 +35,8 @@
'The state of "{}" has {} from "{}" to "{}"'
.format(tool, verb, state, new_state)
)
regressed = True
if not (verb == 'regressed' and tool in REGRESSION_OK):
regressed = True

if regressed:
sys.exit(1)
14 changes: 9 additions & 5 deletions src/ci/docker/x86_64-gnu-tools/checktools.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ python2.7 "$X_PY" test --no-fail-fast \
src/doc/rust-by-example \
src/doc/embedded-book \
src/doc/edition-guide \
src/doc/rustc-guide \
src/tools/clippy \
src/tools/rls \
src/tools/rustfmt \
Expand All @@ -41,7 +42,7 @@ check_tool_failed() {
}

# This function checks that if a tool's submodule changed, the tool's state must improve
verify_status() {
verify_submodule_changed() {
echo "Verifying status of $1..."
if echo "$CHANGED_FILES" | grep -q "^M[[:blank:]]$2$"; then
echo "This PR updated '$2', verifying if status is 'test-pass'..."
Expand All @@ -66,7 +67,7 @@ verify_status() {
check_dispatch() {
if [ "$1" = submodule_changed ]; then
# ignore $2 (branch id)
verify_status $3 $4
verify_submodule_changed $3 $4
elif [ "$2" = beta ]; then
echo "Requiring test passing for $3..."
if check_tool_failed "$3"; then
Expand All @@ -75,7 +76,9 @@ check_dispatch() {
fi
}

# list all tools here
# List all tools here.
# This function gets called with "submodule_changed" for each PR that changed a submodule,
# and with "beta_required" for each PR that lands on beta/stable.
status_check() {
check_dispatch $1 beta book src/doc/book
check_dispatch $1 beta nomicon src/doc/nomicon
Expand All @@ -85,10 +88,10 @@ status_check() {
check_dispatch $1 beta rls src/tools/rls
check_dispatch $1 beta rustfmt src/tools/rustfmt
check_dispatch $1 beta clippy-driver src/tools/clippy
# these tools are not required for beta to successfully branch
# These tools are not required on the beta/stable branches.
# They will still cause failure during the beta cutoff week, see `checkregression.py` for that.
check_dispatch $1 nightly miri src/tools/miri
check_dispatch $1 nightly embedded-book src/doc/embedded-book
check_dispatch $1 nightly rustc-guide src/doc/rustc-guide
Copy link
Member

@kennytm kennytm Jul 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Put this back. This line only verifies that if you have updated rustc-guide the test must pass, and the nightly argument already excluded it from being checked in the beta branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel it would be beneficial to remove it. If I include rustc-guide in an update, but it fails due to some transient error, I would prefer if that didn't fail the build. Generally, I think bad links in rustc-guide should not prevent it from being updated.

Copy link
Member

Choose a reason for hiding this comment

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

Disagreed.

If it is really a transient failure you could just @bors retry.

If the link is an actual bad link it could simply be updated or removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

It takes time to investigate why something failed. It involves loading log files. It involves investigating if it is transient or legitimate. It requires time to remove from a PR, and reporting issues. These are all things I do not want to do. Particularly for rustc-guide, which is not really published from here (I love rustc-guide BTW, don't get me wrong). No other documentation checks external links, and I don't think it is a good idea to gate on it.

If it is enforced, I probably won't include rustc-guide in book updates. Someone else can submit updates for it. I apologize for sounding curt, but I just don't want to spend my time on it.

Copy link
Member

Choose a reason for hiding this comment

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

If we don't care about whether a link failed why do we add a linkchecker? Ignoring the alarm isn't good.

Besides, not gating on it is even worse for debugging since the maintainer of rustc-guide would now need to dig the log archive to get the error message. While with gating at least the Rust-Log-Analyzer could help you extract the relevant logs. If you don't want to investigate you could just mention the rustc-guide maintainer. Since other books are already gated on doc-tests you do need to investigate the log to find out which book is broken anyway.

Furthermore you seem to be exaggerating the probability of spurious external link failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK without this line, there's basically no toolstate tracking happening at all, or is there?

The status is saved in the local toolstate file by the call to x.py test at the top of the file (save-toolstates is set in config.toml). The publishing is done at the bottom (commit_toolstate_change).

Copy link
Member

Choose a reason for hiding this comment

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

So @ehuss @mark-i-m how can we proceed here? Personally I agree with @kennytm and, as @mark-i-m said, "not gating update PRs on this might be less helpful because the status would just remain broken". If a PR updates this submodule and tests still don't pass, surely something is wrong with that PR?

If you have spuriously failing tests, the precedent with RLS is to disable/ignore those tests on your side so that at least the other tests can still do their job.

Copy link
Contributor

Choose a reason for hiding this comment

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

As I said, I'm fine adding it back and seeing how it goes. I just don't want to spend much time dealing with failures going forward.

I personally don't think a failure indicates a problem with the PR. It just indicates the problem hasn't been fixed, yet. A PR that updates a broken rustc-guide when it remains broken doesn't make the problem worse, it's already broken. Making me take time to remove it from the PR doesn't magically make it fixed, it just takes my time, with no benefit for anyone.

Copy link
Member

Choose a reason for hiding this comment

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

They way I see it, allowing tools/doctests to fail at all is a concession to the fact that synchronized updates are hard. But really a failing tool is not an acceptable state. If you are updating a tool and your tests are failing, that should be rejected just like when you are updating rustc and tests are failing.

Making sure tests pass is not a waste of time, it is a basic part of software maintenance.

Copy link
Member Author

Choose a reason for hiding this comment

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

As I said, I'm fine adding it back and seeing how it goes. I just don't want to spend much time dealing with failures going forward.

Thank you :) I will add it back. If it fails again, feel free to just drop rustc-guide and I can investigate.

If you have spuriously failing tests, the precedent with RLS is to disable/ignore those tests on your side so that at least the other tests can still do their job.

Yes, this is the approach I have been taking with rust-lang/rustc-dev-guide#388. Also, Michael-F-Bryan re-wrote the linkchecker to take better advantage of caching, and maybe it will have more flexible handling of timeouts and server errors later too, so hopefully spurious failures will be less common in the future too.

}

# If this PR is intended to update one of these tools, do not let the build pass
Expand All @@ -97,6 +100,7 @@ status_check() {
status_check "submodule_changed"

CHECK_NOT="$(readlink -f "$(dirname $0)/checkregression.py")"
# This callback is called by `commit_toolstate_change`, see `repo.sh`.
change_toolstate() {
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
# only update the history
if python2.7 "$CHECK_NOT" "$OS" "$TOOLSTATE_FILE" "_data/latest.json" changed; then
Expand Down
4 changes: 4 additions & 0 deletions src/ci/docker/x86_64-gnu-tools/repo.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ commit_toolstate_change() {
MESSAGE_FILE="$1"
shift
for RETRY_COUNT in 1 2 3 4 5; do
# Call the callback; this will in the end call `change_toolstate` from
# `checktools.sh` if we are in the `auto` branch (pre-landing) or
# `src/tools/publish_toolstate.py` if we are in the `master` branch
RalfJung marked this conversation as resolved.
Show resolved Hide resolved
# (post-landing).
"$@"
# `git commit` failing means nothing to commit.
FAILURE=0
Expand Down