kill build processes before perf run ALSO fail build if we can't kill build processes #15120
Conversation
|
@dotnet/roslyn-infrastructure This change makes our process-killing more robust. First, it will print errors if we can't kill the build processes. Next, it will fail the build if we can't kill some process. This is important because the build that failed to kill processes should be the one that shows up as a failed process, not subsequent builds that might be effected by a broken server that stayed around. |
|
cc @jaredpar |
| taskkill /F /IM vbcscompiler.exe 2> nul | ||
| taskkill /F /IM msbuild.exe 2> nul | ||
| @REM An error-level of 1 means that the process was found, but could not be killed. | ||
| taskkill /F /IM vbcscompiler.exe > nul |
There was a problem hiding this comment.
Consider the case of multiple users on the same machine. How will taskkill react if a different user is running msbuild / vbcscompiler? Will it attempt to kill the process and issue an error code?
There was a problem hiding this comment.
I haven't been able to test this. However, this script only runs on the CI machines, and it's been working for months without issue.
| @@ -137,6 +138,7 @@ exit /b 0 | |||
| :BuildFailed | |||
| echo Build failed with ERRORLEVEL %ERRORLEVEL% | |||
| call :TerminateBuildProcesses | |||
| :ActuallyFail | |||
There was a problem hiding this comment.
Please use a more descriptive name here. For example: TerminateBuildProcessesFailed.
There was a problem hiding this comment.
I've removed this label entirely.
| if %ERRORLEVEL% == 1 GOTO ActuallyFail | ||
|
|
||
| taskkill /F /IM csc.exe > nul | ||
| if %ERRORLEVEL% == 1 GOTO ActuallyFail |
There was a problem hiding this comment.
I don't think we should add csc / vbc to the list here. Killing vbcscompiler is safe. Killing msbuild is dubious but there are flaws in their process model that really force our hand here. Killing vbc / csc is always dangerous and there is no reason we should be doing it.
If we are concerned about these processes then I think we need to go the other direction: fail the build if csc / vbc are still running in our present session.
There was a problem hiding this comment.
These lines have been removed. Do you want me to add the "is csc/vbc running" check?
| taskkill /F /IM msbuild.exe 2> nul | ||
| @REM An error-level of 1 means that the process was found, but could not be killed. | ||
| taskkill /F /IM vbcscompiler.exe > nul | ||
| if %ERRORLEVEL% == 1 exit /b 1 |
There was a problem hiding this comment.
Should add a message explaining why this terminated. Otherwise the log file will be hard to process in this case .
There was a problem hiding this comment.
I've added a message "killing all build related processes"
I think this is generic enough, because we can call this TerminateBuildProcesses function on both a failure-path or just the normal "time to do a perf run" path.
| taskkill /F /IM vbcscompiler.exe > nul | ||
| if %ERRORLEVEL% == 1 exit /b 1 | ||
|
|
||
| taskkill /F /IM msbuild.exe > nul |
There was a problem hiding this comment.
As long as we're changing this code we should kill msbuild first, then vbcscompiler. Otherwise there is a subtle change msbuild could respawn a new vbcscompiler.
No description provided.