Add -NoBinaryLog switch to opt-out of binlog in CI by wtgodbe · Pull Request #5421 · dotnet/arcade

wtgodbe · 2020-05-05T20:15:21Z

Some repos may want to opt-out of creating a binlog while in CI. For example currently, the AspNetCore binlog is currently large enough that it's hitting an msbuild error causing the build to OOM occasionally: dotnet/aspnetcore#21478. This PR adds a -NoBinaryLog switch that can be passed along with -CI to opt out of producing a binlog. It's kind of an ugly solution, but I couldn't think of another one that wasn't breaking. Changing the behavior of -CI would break repos that assumed passing -CI would create a binlog, and changing -Bl from a switch to a bool would break folks in a similar way. I'm open to suggestions, though.

dougbu

CI failures are due to my first comment in this review. See https://dev.azure.com/dnceng/public/_build/results?buildId=632262&view=logs&j=1295a90a-32ad-54ea-34e1-74b5e62ea3f1&t=d52efd14-fca6-511e-26d3-b8cb177efb38&l=14

akoeplinger · 2020-05-06T15:09:52Z

-# Binary log must be enabled on CI.
 [bool]$binaryLog = if (Test-Path variable:binaryLog) { $binaryLog } else { $ci }

+# Set to true to opt out of outputing binary log while running in CI


Suggested change

# Set to true to opt out of outputing binary log while running in CI

# Set to true to opt out of outputting binary log while running in CI

akoeplinger · 2020-05-06T15:10:21Z

-# Binary log must be enabled on CI.
 binary_log=${binary_log:-$ci}

+# Set to true to opt out of outputing binary log while running in CI


Suggested change

# Set to true to opt out of outputing binary log while running in CI

# Set to true to opt out of outputting binary log while running in CI

akoeplinger · 2020-05-06T15:14:36Z

  Write-Host "Advanced settings:"
  Write-Host "  -projects <value>       Semi-colon delimited list of sln/proj's to build. Globbing is supported (*.sln)"
  Write-Host "  -ci                     Set when running on CI server"
+  Write-Host "  -noBinaryLog            Don't output binary log (useful when running on CI server) (short: -nobl)"


I'd remove the (useful when running on CI server) since it makes it sound like not using the binlog in CI would be recommended, but we actually want them in CI in most cases.

tmat · 2020-05-06T18:53:04Z

I don't like this. I'd prefer commenting out the condition if it's needed as a temporary workaround until the OOM is fixed:

 if (!$binaryLog) {
      Write-PipelineTelemetryError -Category 'Build' -Message 'Binary log must be enabled in CI build.'	        
      ExitWithExitCode 1	      ExitWithExitCode 1
    }

dougbu · 2020-05-06T19:11:57Z

@tmat even if we didn't want to disable producing binary logs in this repo, the new switch enables us removing hacks like this:

# Workaround Arcade check which asserts BinaryLog is true on CI.
# We always use binlogs on CI, but we customize the name of the log file
$tmpBinaryLog = $BinaryLog
if ($CI) {
    $BinaryLog = $true
}

Commenting out that block in dotnet/aspnetcore would be overridden in every Arcade dependency update.
@rainersigwald doesn't have a fix out yet for OOM when generating binlog for boost.hana msbuild#3577 i.e. we have no idea yet when it will be fixed.

tmat · 2020-05-06T19:19:31Z

Commenting out that block in dotnet/aspnetcore would be overridden in every Arcade dependency update.

I meant commenting it out in Arcade.

dougbu · 2020-05-06T19:23:04Z

I meant commenting it out in Arcade.

How would that work without removing the -ci ==> -binaryLog default in tools.ps1❔ Also, commenting out is confusing and deletion is better (everything that ever existed is in git history😃).

wtgodbe · 2020-05-06T19:25:45Z

How would that work without removing the -ci ==> -binaryLog default in tools.ps1

The same default also exists in build.ps1:

arcade/eng/common/build.ps1

Lines 136 to 139 in a8ee27e

    
           if ($ci) { 
        
             $binaryLog = $true 
        
             $nodeReuse = $false 
        
           }

markwilkie · 2020-05-07T14:38:18Z

(this makes me sad.....however...onward)

Question: Am I reading this correctly that -NoBinaryLog is only relevant for -CI? If so, maybe change the flag to -NoCIBinaryLog or something?

dougbu · 2020-05-07T18:02:53Z

@wtgodbe could you try hacking your local Arcade bits to make the minimal eng/common/tools.ps1|sh change i.e. just remove the error❔ Leave our build.ps1|sh hack mostly in place but include !$BinaryLog in the condition and switch the $BinaryLog setting to false. If that works, it's probably the minimal change we could make.

(As a smaller detail, the block a bit later just redoes what tools.ps1|sh does when $BinaryLog is true.)

Note a change to configure-toolset.ps1 could override an explicit -binaryLog on the build.ps1|sh command line because it runs after the variables are set in tools.ps1|sh.

markwilkie · 2020-05-07T18:12:17Z

Also, I'm not asking for a minimal change or a hack. I'm only asking if the flag is relevant to CI only, then perhaps we should put 'ci' in the name for clarity.

dougbu · 2020-05-07T18:53:49Z

I'm not asking for a minimal change or a hack

Understood but I thought @tmat was looking for the minimal change. I suggested it and the hack would only be a variant of something dotnet/aspnetcore already needs to do. Either way, I'm fine.

wtgodbe · 2020-05-07T19:27:58Z

 # Set to true to output binary log from msbuild. Note that emitting binary log slows down the build.
-# Binary log must be enabled on CI.
-binary_log=${binary_log:-$ci}
+binary_log=${binary_log:-$ci && -n $no_ci_binary_log}


I'm not sure if this is the right syntax

markwilkie · 2020-05-07T19:37:56Z

This change makes sense to me.

chcosta · 2020-05-07T19:49:36Z

  [switch] $publish,
  [switch] $clean,
  [switch][Alias('bl')]$binaryLog,
+  [switch][Alias('nobl')]$noCIBinaryLog,


nit: maybe "excludeCiBinlog", "no ci binary log" is slightly confusing as to what it means.

dougbu

Please merge as soon as you're sure the tools.sh change is correct unless there are new objections.

dougbu · 2020-05-08T03:26:03Z

 # Set to true to output binary log from msbuild. Note that emitting binary log slows down the build.
-# Binary log must be enabled on CI.
-binary_log=${binary_log:-$ci}
+binary_log=${binary_log:-$ci && -n $exclude_ci_binary_log}


Did you find a way to test this @wtgodbe❔

If not, Arcade doesn't use --excludeCIBinlog or anything similar, you'll need to apply this change in dotnet/aspnetcore and use it in dotnet/aspnetcore (under WSL, on a Linux VM, or in your PR). A quick check would include --projects rc/Html/Abstractions/src/Microsoft.AspNetCore.Html.Abstractions.csproj on the build.sh command line.

Just tested it, this didn't work - I couldn't figure out a way to get expressions working while setting default parameters, so I initialized the default above.

dougbu · 2020-05-08T03:27:46Z

  Write-Host "Advanced settings:"
  Write-Host "  -projects <value>       Semi-colon delimited list of sln/proj's to build. Globbing is supported (*.sln)"
  Write-Host "  -ci                     Set when running on CI server"
+  Write-Host "  -excludeCIBinlog          Don't output binary log (short: -nobl)"


@chcosta do these descriptions need to line up perfectly❔ (I'm hoping this is a nit we can fix later.)

chcosta · 2020-05-08T15:47:11Z

I don't want to delay unblocking people, but another nit is that the naming is still a bit inconsistent. Sometimes it's "binary log" and sometimes it's "binlog". I only mention it because if it's not inconsistent now, then it probably never will be as it may become a breaking change.

@dougbu , things like descriptions can be easily fixed whenever.

tmat · 2020-05-08T20:59:26Z

So, you could have already done this without adding another parameter by setting the default for $binaryLog variable. The default can be overridden in eng\configure-toolset.ps1.

See dotnet/aspnetcore#21478 (comment).

dougbu · 2020-05-10T03:48:15Z

The default can be overridden in eng\configure-toolset.ps1.

As I mentioned before, the default can't be changed. Instead the final version can be overridden -- without any information about the binaryLog value before tools.ps1 is invoked.

markwilkie · 2020-05-11T20:55:23Z

To close on this discussion, let's leave this in for now but add a tracking issue to do a follow up later for possible clean up, or an improved approach.

cc/ @dougbu

Add -NoBinaryLog switch to opt-out of binlog in CI

5684960

wtgodbe requested review from JunTaoLuo, chcosta, dougbu and tmat May 5, 2020 20:15

dougbu reviewed May 5, 2020

View reviewed changes

Comment thread eng/common/build.sh Outdated

Comment thread eng/common/tools.ps1 Outdated

Comment thread eng/common/tools.ps1 Outdated

Comment thread eng/common/tools.sh

akoeplinger reviewed May 6, 2020

View reviewed changes

wtgodbe added 2 commits May 6, 2020 11:06

respond to feedback

7dfbda9

Fixup

cf5eefb

dougbu mentioned this pull request May 7, 2020

Don't produce binlogs in CI dotnet/aspnetcore#21478

Closed

rainersigwald approved these changes May 7, 2020

View reviewed changes

wtgodbe added 2 commits May 7, 2020 12:26

Respond to feedback

36feef5

One more fixup

9c04562

wtgodbe commented May 7, 2020

View reviewed changes

chcosta reviewed May 7, 2020

View reviewed changes

dougbu mentioned this pull request May 7, 2020

Use a dedicated thread for timers in rather than a Timer dotnet/aspnetcore#21490

Merged

Change prop name

5e1277a

dougbu approved these changes May 8, 2020

View reviewed changes

Fixup .sh, fix syntax

b539815

wtgodbe merged commit 8078d8f into master May 8, 2020

dougbu deleted the wtgodbe/NoBinLog branch May 10, 2020 00:10

	# Set to true to opt out of outputing binary log while running in CI
	# Set to true to opt out of outputting binary log while running in CI

Conversation

wtgodbe commented May 5, 2020

Uh oh!

dougbu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmat commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dougbu commented May 6, 2020

Uh oh!

tmat commented May 6, 2020

Uh oh!

dougbu commented May 6, 2020

Uh oh!

wtgodbe commented May 6, 2020

Uh oh!

markwilkie commented May 7, 2020

Uh oh!

dougbu commented May 7, 2020

Uh oh!

markwilkie commented May 7, 2020

Uh oh!

dougbu commented May 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markwilkie commented May 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dougbu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chcosta commented May 8, 2020

Uh oh!

tmat commented May 8, 2020

Uh oh!

dougbu commented May 10, 2020

Uh oh!

markwilkie commented May 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tmat commented May 6, 2020 •

edited

Loading