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

Automatically open an issue when a tool breaks #56951

Open
wants to merge 7 commits into
base: master
from

Conversation

@oli-obk
Copy link
Contributor

oli-obk commented Dec 18, 2018

cc @nikomatsakis

fixes rust-lang-nursery/rust-toolstate#6

documentation about issue opening via the github api: https://developer.github.com/v3/issues/#create-an-issue

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 18, 2018

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 18, 2018

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 18, 2018

This is awesome— can we make it be automatically T-compiler and I-nominated? (I'm assuming T-compiler will always be the appropriate team)

Show resolved Hide resolved src/tools/publish_toolstate.py Outdated
@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Dec 18, 2018

I'm not sure if this is a good idea as RLS is known to sometimes fail spuriously (recent example: #56161 (comment) → "fixed" by #56461 (comment)).

@phansch

This comment has been minimized.

Copy link
Contributor

phansch commented Dec 19, 2018

This probably would mention the wrong person/PR when a rollup caused breakage, right?

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Dec 19, 2018

For rollups it would mention the person who created the rollup (same as today), but since the rollupers are also Rust team members they could just edit the issue to point to the actual cause.

What I worried is blaming new contributors over something not related to their code.

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Dec 19, 2018

What I worried is blaming new contributors over something not related to their code.

due to the spurious case? We could just do this for rustfmt and clippy for now until rls isn't spurious anymore.

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Dec 19, 2018

@oli-obk Yes due to spurious cases. What if we submit the issue only if the state is degraded to build-fail, which won't be spurious?

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Dec 19, 2018

I pushed some changes, not sure if they actually do what they're supposed to. Are we really using < and > to compare the strings build-fail, test-fail, test-pass and that just happens to work out because of the lexical ordering of fail and pass and the lexical ordering of build and test?

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Dec 19, 2018

@oli-obk

Are we really using < and > to compare the strings build-fail, test-fail, test-pass and that just happens to work out because of the lexical ordering of fail and pass and the lexical ordering of build and test?

Yes 😛

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Dec 19, 2018

I think that in terms of "blaming" contributors, we should just be careful with our wording, and make sure to cc enough folks from the team.

Is the plan to ignore test failures and just focus on build failures? That seems "ok" to start.

I would like to see the template be something a bit more friendly and perhaps include a bit more information. Maybe something like this?


issue title: {TOOL} no longer builds after PR #{PR}

Hello, this is your friendly neighborhood mergebot. After merging PR #{PR}, I observed that the tool {TOOL} no longer builds. A follow-up PR to the repository {TOOL_REPOSITORY} is needed to fix the fallout.

cc @{PR_AUTHOR}, do you think you would have time to do the follow-up work? If so, that would be great!

cc @{PR_REVIEWER}, the PR reviewer, and @rust-lang/compiler -- nominating for prioritization.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 19, 2018

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:2bc5a7ee:start=1545237978429117636,finish=1545238035323829083,duration=56894711447
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---

[00:03:02] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:03:02] tidy error: /checkout/src/tools/publish_toolstate.py:78: line longer than 100 chars
[00:03:02] tidy error: /checkout/src/tools/publish_toolstate.py:147: line longer than 100 chars
[00:03:02] tidy error: /checkout/src/tools/publish_toolstate.py:171: line longer than 100 chars
[00:03:03] some tidy checks failed
[00:03:03] 
[00:03:03] 
[00:03:03] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:03:03] 
[00:03:03] 
[00:03:03] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:03] Build completed unsuccessfully in 0:00:43
[00:03:03] Build completed unsuccessfully in 0:00:43
[00:03:03] Makefile:79: recipe for target 'tidy' failed
[00:03:03] make: *** [tidy] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:22644c00
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Wed Dec 19 16:50:27 UTC 2018
---
travis_time:end:189241f0:start=1545238228319777533,finish=1545238228324256399,duration=4478866
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0ba2a63f
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:00182946
travis_time:start:00182946
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0277539b
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@kennytm
Copy link
Member

kennytm left a comment

Rest LGTM, but I'd like to first go through an FCP after the holiday (next week).

Show resolved Hide resolved src/tools/publish_toolstate.py Outdated

@kennytm kennytm added T-compiler and removed T-compiler labels Dec 29, 2018

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Dec 29, 2018

@rfcbot fcp merge

This PR will change rust-highfive's behavior on toolstate change. If a tool's state is changed to build-fail after a PR is merged, rust-highfive will automatically create an issue according to the template like #56951 (comment). A regression to test-fail is ignored since it won't prevent release of a tool and is potentially spurious.

Pulling in T-compiler since the team is explicitly mentioned in the issue template.
Pulling in T-dev-tools since it involves the tools.
Pulling in T-infra since it involves a bot.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Dec 29, 2018

Team member @kennytm has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@mark-i-m

This comment has been minimized.

Copy link
Contributor

mark-i-m commented Dec 30, 2018

Would it be possible to create a per-dependency setting? e.g. rls could open an issue on build failures only while another dependency would open an issue on test failures. The reason I ask is that we hope to add rustc-guide and use toolstate to check for link breakage.

@gibfahn

This comment has been minimized.

Copy link
Contributor

gibfahn commented Dec 30, 2018

If/when this lands it would be great to have a way to link to it from the Red squares on the rustup-components-history page, and maybe even from the rustup update error message (although maybe that should just statically link to the rustup-components-history page.

Maybe it should just be a link to the list of issues with a certain tag, but the current implementation only adds T-compiler and I-nominated, which probably isn't enough to differentiate, not sure if another label would be the right way to go or not.

More relevant to this PR, maybe the issue text should link back to rustup-components-history, which could then be a dashboard/overview.

EDIT: There's already a PR open for the rustup -> rustup-components-history page: rust-lang/rustup.rs#1595

@kennytm

This comment has been minimized.

Copy link
Member

kennytm commented Dec 31, 2018

@gibfahn rustup-component-history is currently not officially maintained. You should file a feature request on their repository.

As this PR will publish an issue before a nightly is released, linking to rustup-component-history won't reflect the actual status.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 1, 2019

How necessary is this extra bureaucracy?
The process as is seems to work good enough without it, tools are fixed timely.

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Jan 6, 2019

My thought was that this is unnecessary since tools (mostly Clippy) break frequently but are fixed quickly. However, it is often @oli-obk who does the fixing, so if he thinks it is useful to have this, then lets do it :-)

cc @{}, the PR reviewer, and @rust-lang/compiler -- nominating for prioritization.
''').format(relevant_pr_number, tool, REPOS[tool], relevant_pr_user, pr_reviewer),

This comment has been minimized.

@shepmaster

shepmaster Jan 7, 2019

Member

If we have a new enough version of Python, it might be more maintainable to use named parameters here, instead of positional. I don't have to touch this code, so it's no skin off my back!

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Jan 7, 2019

However, it is often @oli-obk who does the fixing, so if he thinks it is useful to have this, then lets do it :-)

Actually that torch has been taken over by @matthiaskrgr

I still think this is useful, because trying to find the relevant comments inside the failing PRs is not fun. Also I think it's noise if we start discussing tool-fix related things in the PR that broke the tool. A dedicated issue gives this process some structure.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Jan 7, 2019

I personally find it really hard to track conversations in closed PRs; I think it'd be better to track "work to be done" in issues.

@oli-obk oli-obk force-pushed the oli-obk:auto_toolstate_issue branch from cf9d5e5 to 488f16a Jan 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment