Split unix test builds in slices #17785
Conversation
|
@sdmaclea That's great you have ported it to *nixes. The 102 min build time on Skylake laptop is pretty good as I would expect that CI build would be around 50 - 60 minutes or even shorter. I will go through the code review tomorrow. @BruceForstall @janvorli @AndyAyersMS @RussKeldorph Perhaps if we would confirm that CI test build performance is at satisfactory level we should consider switching to native *nix build in CI instead of using Windows cross build step. |
build-test.sh
Outdated
|
|
||
| # Invoke MSBuild | ||
| # $__ProjectRoot/run.sh build -Project=$projectName -MsBuildLog="$__msbuildLog" -MsBuildWrn="$__msbuildWrn" -MsBuildErr="$__msbuildErr" $extraBuildParameters $__RunArgs $__UnprocessedBuildArgs | ||
| for (( slice=1 ; slice < __BuildLoopCount; slice = slice + 1 )) |
There was a problem hiding this comment.
Doesn't this need to be <= or else you'll not build the final slice?
There was a problem hiding this comment.
I would guess you are right, but I looked at the build logs, not sure how I missed that the last slice wasn't being build
build-test.sh
Outdated
|
|
||
| echo "Building step '$stepName' via $buildCommand" | ||
| export __SkipPackageRestore=false |
There was a problem hiding this comment.
Can you add a comment that references tests/src/dirs.proj, and says that these variables are consumed there?
| fi | ||
| export __SkipPackageRestore=true | ||
| export __SkipTargetingPackBuild=true | ||
| __AppendToLog=true |
There was a problem hiding this comment.
Can you initialize __AppendToLog=false before the slice loop? Even if passing Append= (empty) to msbuild works, it seems like being explicit would be better (it looks like build-test.cmd does this).
build-test.sh
Outdated
| @@ -615,6 +660,10 @@ while :; do | |||
| msbuildonunsupportedplatform) | |||
| __msbuildonunsupportedplatform=1 | |||
| ;; | |||
| priority) | |||
There was a problem hiding this comment.
Can you update the usage() function?
Ideally, I'd think priority would take a number, which would default to zero, but if you decide not to do that, at least make the argument "priority_one", as just "priority" seems ambiguous to me.
|
@BruceForstall done |
build-test.sh
Outdated
|
|
||
| # Invoke MSBuild | ||
| eval $buildCommand | ||
| if [ -n __Priority ]; then |
There was a problem hiding this comment.
Are variables case-sensitive? You define this as:
__priority=1
using lower case. No matter the answer, it's probably better to use the same casing.
Since the script argument is not "priority1" maybe you can change this to __priority1 to match.
There was a problem hiding this comment.
Fixed. I think my tree must have been dirty when I first pushed this...
Any way. I built locally with and without priority1 with the most recently pushed version and it looks OK.
* Split unix test builds in slices Ports dotnet#17161 to linux * Address review feedback
Ports #17161 to linux
@4creators @AndyAyersMS @weshaggard @BruceForstall @RussKeldorph @jkotas
I can confirm this allows tests to be built on my Ubuntu SkyLake desktop. As it hung before this is infinite improvement. It took 102 minutes to build the priority 1 Linux Arm64 Checked managed tests.