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

Auto-toolstate management follow-up. #48405

Merged
merged 6 commits into from
Mar 2, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ matrix:
script:
MESSAGE_FILE=$(mktemp -t msg.XXXXXX);
. src/ci/docker/x86_64-gnu-tools/repo.sh;
commit_toolstate_change "$MESSAGE_FILE" "$TRAVIS_BUILD_DIR/src/tools/publish_toolstate.py" "$(git rev-parse HEAD)" "$(git log --format=%s -n1 HEAD)" "$MESSAGE_FILE"
commit_toolstate_change "$MESSAGE_FILE" "$TRAVIS_BUILD_DIR/src/tools/publish_toolstate.py" "$(git rev-parse HEAD)" "$(git log --format=%s -n1 HEAD)" "$MESSAGE_FILE" "$TOOLSTATE_REPO_ACCESS_TOKEN";

env:
global:
Expand Down
6 changes: 4 additions & 2 deletions src/bootstrap/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ pub struct ShouldRun<'a> {
paths: BTreeSet<PathSet>,

// If this is a default rule, this is an additional constraint placed on
// it's run. Generally something like compiler docs being enabled.
// its run. Generally something like compiler docs being enabled.
is_really_default: bool,
}

Expand Down Expand Up @@ -326,7 +326,9 @@ impl<'a> Builder<'a> {
test::RunPassPretty, test::RunFailPretty, test::RunPassValgrindPretty,
test::RunPassFullDepsPretty, test::RunFailFullDepsPretty, test::RunMake,
test::Crate, test::CrateLibrustc, test::Rustdoc, test::Linkcheck, test::Cargotest,
test::Cargo, test::Rls, test::Docs, test::ErrorIndex, test::Distcheck,
test::Cargo, test::Rls, test::ErrorIndex, test::Distcheck,
test::Nomicon, test::Reference, test::RustdocBook, test::RustByExample,
test::TheBook, test::UnstableBook,
test::Rustfmt, test::Miri, test::Clippy, test::RustdocJS, test::RustdocTheme),
Kind::Bench => describe!(test::Crate, test::CrateLibrustc),
Kind::Doc => describe!(doc::UnstableBook, doc::UnstableBookGen, doc::TheBook,
Expand Down
3 changes: 2 additions & 1 deletion src/bootstrap/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,8 @@ impl Step for CodegenBackend {
.arg(build.src.join("src/librustc_trans/Cargo.toml"));
rustc_cargo_env(build, &mut cargo);

let _folder = build.fold_output(|| format!("stage{}-rustc_trans", compiler.stage));

match &*self.backend {
"llvm" | "emscripten" => {
// Build LLVM for our target. This will implicitly build the
Expand All @@ -643,7 +645,6 @@ impl Step for CodegenBackend {
features.push_str(" emscripten");
}

let _folder = build.fold_output(|| format!("stage{}-rustc_trans", compiler.stage));
println!("Building stage{} codegen artifacts ({} -> {}, {})",
compiler.stage, &compiler.host, target, self.backend);

Expand Down
87 changes: 69 additions & 18 deletions src/bootstrap/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,15 +78,17 @@ fn try_run(build: &Build, cmd: &mut Command) -> bool {
true
}

fn try_run_quiet(build: &Build, cmd: &mut Command) {
fn try_run_quiet(build: &Build, cmd: &mut Command) -> bool {
if !build.fail_fast {
if !build.try_run_quiet(cmd) {
let mut failures = build.delayed_failures.borrow_mut();
failures.push(format!("{:?}", cmd));
return false;
}
} else {
build.run_quiet(cmd);
}
true
}

#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
Expand Down Expand Up @@ -994,23 +996,19 @@ impl Step for Compiletest {
}

#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub struct Docs {
struct DocTest {
compiler: Compiler,
path: &'static str,
name: &'static str,
is_ext_doc: bool,
}

impl Step for Docs {
impl Step for DocTest {
type Output = ();
const DEFAULT: bool = true;
const ONLY_HOSTS: bool = true;

fn should_run(run: ShouldRun) -> ShouldRun {
run.path("src/doc")
}

fn make_run(run: RunConfig) {
run.builder.ensure(Docs {
compiler: run.builder.compiler(run.builder.top_stage, run.host),
});
run.never()
}

/// Run `rustdoc --test` for all documentation in `src/doc`.
Expand All @@ -1026,9 +1024,9 @@ impl Step for Docs {

// Do a breadth-first traversal of the `src/doc` directory and just run
// tests for all files that end in `*.md`
let mut stack = vec![build.src.join("src/doc")];
let mut stack = vec![build.src.join(self.path)];
let _time = util::timeit();
let _folder = build.fold_output(|| "test_docs");
let _folder = build.fold_output(|| format!("test_{}", self.name));

while let Some(p) = stack.pop() {
if p.is_dir() {
Expand All @@ -1046,11 +1044,64 @@ impl Step for Docs {
continue;
}

markdown_test(builder, compiler, &p);
let test_result = markdown_test(builder, compiler, &p);
if self.is_ext_doc {
let toolstate = if test_result {
ToolState::TestPass
} else {
ToolState::TestFail
};
build.save_toolstate(self.name, toolstate);
}
}
}
}

macro_rules! test_book {
($($name:ident, $path:expr, $book_name:expr, default=$default:expr;)+) => {
$(
#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub struct $name {
compiler: Compiler,
}

impl Step for $name {
type Output = ();
const DEFAULT: bool = $default;
const ONLY_HOSTS: bool = true;

fn should_run(run: ShouldRun) -> ShouldRun {
run.path($path)
}

fn make_run(run: RunConfig) {
run.builder.ensure($name {
compiler: run.builder.compiler(run.builder.top_stage, run.host),
});
}

fn run(self, builder: &Builder) {
builder.ensure(DocTest {
compiler: self.compiler,
path: $path,
name: $book_name,
is_ext_doc: !$default,
});
}
}
)+
}
}

test_book!(
Nomicon, "src/doc/nomicon", "nomicon", default=false;
Reference, "src/doc/reference", "reference", default=false;
RustdocBook, "src/doc/rustdoc", "rustdoc", default=true;
RustByExample, "src/doc/rust-by-example", "rust-by-example", default=false;
TheBook, "src/doc/book", "book", default=false;
UnstableBook, "src/doc/unstable-book", "unstable-book", default=true;
);

#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
pub struct ErrorIndex {
compiler: Compiler,
Expand Down Expand Up @@ -1101,13 +1152,13 @@ impl Step for ErrorIndex {
}
}

fn markdown_test(builder: &Builder, compiler: Compiler, markdown: &Path) {
fn markdown_test(builder: &Builder, compiler: Compiler, markdown: &Path) -> bool {
let build = builder.build;
let mut file = t!(File::open(markdown));
let mut contents = String::new();
t!(file.read_to_string(&mut contents));
if !contents.contains("```") {
return;
return true;
}

println!("doc tests for: {}", markdown.display());
Expand All @@ -1121,9 +1172,9 @@ fn markdown_test(builder: &Builder, compiler: Compiler, markdown: &Path) {
cmd.arg("--test-args").arg(test_args);

if build.config.quiet_tests {
try_run_quiet(build, &mut cmd);
try_run_quiet(build, &mut cmd)
} else {
try_run(build, &mut cmd);
try_run(build, &mut cmd)
}
}

Expand Down
1 change: 1 addition & 0 deletions src/ci/docker/x86_64-gnu-tools/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ RUN apt-get update && apt-get install -y --no-install-recommends \
COPY scripts/sccache.sh /scripts/
RUN sh /scripts/sccache.sh

COPY x86_64-gnu-tools/checkregression.py /tmp/
COPY x86_64-gnu-tools/checktools.sh /tmp/
COPY x86_64-gnu-tools/repo.sh /tmp/

Expand Down
40 changes: 40 additions & 0 deletions src/ci/docker/x86_64-gnu-tools/checkregression.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
#!/usr/bin/env python
# -*- coding: utf-8 -*-

# Copyright 2018 The Rust Project Developers. See the COPYRIGHT
# file at the top-level directory of this distribution and at
# http://rust-lang.org/COPYRIGHT.
#
# Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
# http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
# <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
# option. This file may not be copied, modified, or distributed
# except according to those terms.

import sys
import json

if __name__ == '__main__':
os_name = sys.argv[1]
toolstate_file = sys.argv[2]
current_state = sys.argv[3]

with open(toolstate_file, 'r') as f:
toolstate = json.load(f)
with open(current_state, 'r') as f:
current = json.load(f)

regressed = False
for cur in current:
tool = cur['tool']
state = cur[os_name]
new_state = toolstate.get(tool, '')
if new_state < state:
print(
'Error: The state of "{}" has regressed from "{}" to "{}"'
.format(tool, state, new_state)
)
regressed = True

if regressed:
sys.exit(1)
47 changes: 35 additions & 12 deletions src/ci/docker/x86_64-gnu-tools/checktools.sh
Original file line number Diff line number Diff line change
Expand Up @@ -17,39 +17,57 @@ TOOLSTATE_FILE="$(realpath $2)"
OS="$3"
COMMIT="$(git rev-parse HEAD)"
CHANGED_FILES="$(git diff --name-status HEAD HEAD^)"
SIX_WEEK_CYCLE="$(( ($(date +%s) / 604800 - 3) % 6 ))"
# ^ 1970 Jan 1st is a Thursday, and our release dates are also on Thursdays,
# thus we could divide by 604800 (7 days in seconds) directly.

touch "$TOOLSTATE_FILE"

set +e
python2.7 "$X_PY" test --no-fail-fast \
src/doc/book \
src/doc/nomicon \
src/doc/reference \
src/doc/rust-by-example \
src/tools/rls \
src/tools/rustfmt \
src/tools/miri \
src/tools/clippy
set -e

cat "$TOOLSTATE_FILE"
echo

# If this PR is intended to update one of these tools, do not let the build pass
# when they do not test-pass.
for TOOL in rls rustfmt clippy; do
echo "Verifying status of $TOOL..."
if echo "$CHANGED_FILES" | grep -q "^M[[:blank:]]src/tools/$TOOL$"; then
echo "This PR updated 'src/tools/$TOOL', verifying if status is 'test-pass'..."
if grep -vq '"'"$TOOL"'[^"]*":"test-pass"' "$TOOLSTATE_FILE"; then
verify_status() {
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'..."
if grep -vq '"'"$1"'":"test-pass"' "$TOOLSTATE_FILE"; then
echo
echo "⚠️ We detected that this PR updated '$TOOL', but its tests failed."
echo "⚠️ We detected that this PR updated '$1', but its tests failed."
echo
echo "If you do intend to update '$TOOL', please check the error messages above and"
echo "If you do intend to update '$1', please check the error messages above and"
echo "commit another update."
echo
echo "If you do NOT intend to update '$TOOL', please ensure you did not accidentally"
echo "change the submodule at 'src/tools/$TOOL'. You may ask your reviewer for the"
echo "If you do NOT intend to update '$1', please ensure you did not accidentally"
echo "change the submodule at '$2'. You may ask your reviewer for the"
echo "proper steps."
exit 3
fi
fi
done
}

# If this PR is intended to update one of these tools, do not let the build pass
# when they do not test-pass.

verify_status book src/doc/book
verify_status nomicon src/doc/nomicon
verify_status reference src/doc/reference
verify_status rust-by-example src/doc/rust-by-example
verify_status rls src/tool/rls
verify_status rustfmt src/tool/rustfmt
verify_status clippy-driver src/tool/clippy
#verify_status miri src/tool/miri

if [ "$RUST_RELEASE_CHANNEL" = nightly -a -n "${TOOLSTATE_REPO_ACCESS_TOKEN+is_set}" ]; then
. "$(dirname $0)/repo.sh"
Expand All @@ -59,6 +77,11 @@ if [ "$RUST_RELEASE_CHANNEL" = nightly -a -n "${TOOLSTATE_REPO_ACCESS_TOKEN+is_s
sed -i "1 a\\
$COMMIT\t$(cat "$TOOLSTATE_FILE")
" "history/$OS.tsv"
# if we are at the last week in the 6-week release cycle, reject any kind of regression.
if [ $SIX_WEEK_CYCLE -eq 5 ]; then
Copy link
Member

Choose a reason for hiding this comment

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

I would be a lot more comfortable if this was a 4, since the 5 basically doesn't help much -- last release, we landed breakage to RLS on Monday of the 2nd to last week, which as far as I can tell, this wouldn't have caught. I would like the tools to ideally be green the week of the release, and the week before the release. That seems like a fairly reasonable requirement.

Copy link
Member

Choose a reason for hiding this comment

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

That is 1/3 rd of the release cycle, which seems kind of long. I think what is important is for tool authors to know if their tool is broken going into that final week, since they probably don;t keep track of the release cycle.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with leaving it at 5 for a cycle or two and then reevaluating if necessary; my hope is that over time there will be some other solution that we come up with...

Copy link
Member Author

Choose a reason for hiding this comment

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

What about T-10 days as a compromise? 🤔

python2.7 "$(dirname $0)/checkregression.py" \
"$OS" "$TOOLSTATE_FILE" "rust-toolstate/_data/latest.json"
fi
rm -f "$MESSAGE_FILE"
exit 0
fi
Expand Down
Loading