Bootstrapper improvements. #1423

tmat · 2018-12-01T01:43:15Z

Memoize results of Initialize* functions.
Move build customization hooks to build.ps1.
Make binary log off by default since it slows down build. It's on in CI build.
Fixes to quiet restore workaround.

Fixes #1251

tmat · 2018-12-04T23:03:40Z

@chcosta PTAL

tmat · 2018-12-04T23:04:34Z

eng/common/tools.ps1

@@ -71,6 +71,11 @@ function InitializeDotNetCli([bool]$install) {
  # Disable first run since we do not need all ASP.NET packages restored.
  $env:DOTNET_SKIP_FIRST_TIME_EXPERIENCE=1

+  # Disable telemetry on CI.
+  if ($ci) {
+    $env:DOTNET_CLI_TELEMETRY_OPTOUT=1


@livarcocc Copied this from Roslyn. Does it make sense?

This means our own repos won't be sending telemetry to us. Is there a particular reason we would want to have this turned off?

I would prefer if we were setting the telemetry profile per repo so that we could still get telemetry but also identify the usage.

cc @KathleenDollard

Do you want telemetry from CI builds? This only applies to CI.

tmat · 2018-12-04T23:04:59Z

eng/common/tools.sh

+  if [[ $ci == true ]]; then
+    export DOTNET_CLI_TELEMETRY_OPTOUT=1
+  fi
+
  # LTTNG is the logging infrastructure used by Core CLR. Need this variable set
  # so it doesn't output warnings to the console.
  export LTTNG_HOME="$HOME"


@livarcocc Copied this from Roslyn. Does it make sense?

See my comment above.

I'm not really sure, what this does. It was something that Roslyn has been doing.
@jaredpar @agocke

Sounds like one of you is talking about telemetry and the other is talking about LTTNG_HOME. As for telemetry I think we turned it off originally because it was causing issues (back in the pre-1.0 days so not relevant anymore) and didn't seem to make sense to get telemetry from CI. Have no objections to turning it back on.

Don't know about LTTNG_HOME.

@livarcocc Up to you. Feel free to send PR if you want telemetry from CI.

tmat · 2018-12-04T23:05:39Z

eng/Versions.props

@@ -48,14 +48,8 @@
    <MicrosoftDotNetBuildTasksFeedVersion>2.2.0-beta.18603.7</MicrosoftDotNetBuildTasksFeedVersion>
    <MicrosoftDotNetMaestroTasksVersion>1.0.0-beta.18603.7</MicrosoftDotNetMaestroTasksVersion>
    <MicrosoftDotNetSignToolVersion>1.0.0-beta.18603.7</MicrosoftDotNetSignToolVersion>
-    <!-- 3rd Part Packages Public Keys -->
-    <DynamicProxyGenAsm2Key>0024000004800000940000000602000000240000525341310004000001000100c547cac37abd99c8db225ef2f6c8a3602f3b3606cc9891605d02baa56104f4cfc0734aa39b93bf7852f7d9266654753cc297e7d2edfe0bac1cdcf9f717241550e0a7b191195b7667bb4f64bcb8e2121380fd1d9d46ad2d92d2d15605093924cceaf74c4861eff62abf69b9291ed0a340e113be11e6a7d3113e92484cf7045cc7</DynamicProxyGenAsm2Key>


This property isn't used

chcosta

Couple of comments but overall looks good. Thanks for these improvements!

chcosta · 2018-12-05T16:16:16Z

eng/common/build.ps1

@@ -31,6 +32,7 @@ function Print-Usage() {
    Write-Host "Common settings:"
    Write-Host "  -configuration <value>  Build configuration Debug, Release"
    Write-Host "  -verbosity <value>      Msbuild verbosity (q[uiet], m[inimal], n[ormal], d[etailed], and diag[nostic])"
+    Write-Host "  -binaryLog              Output binary log"


Can this take an optional value so that you can create multiple binary logs? In particular, if someone decides (for whatever reason), to do a distinct build.cmd -binaryLog -restore followed by a build.cmd -binaryLog -build, then the binlogs would overwrite.

I think that's fine. You get binlog for the latest build action. You can combine always actions build -restore -build -bl and get a log that covers both. I'd not complicate this until it will become an issue.

You can also copy the binlog out before performing the next action.

chcosta · 2018-12-05T16:19:03Z

eng/common/build.ps1

@@ -64,17 +66,45 @@ if ($help -or (($properties -ne $null) -and ($properties.Contains("/help") -or $
  exit 0
 }

+function ConfigureToolset { 


I'm often getting questions about how to do "b" instead of "a" or override some behavior, etc... It would be nice if this was either documented or self-documented in a way that was more discoverable. I suppose it's more for the power user than the standard Arcade SDK consumer, but I think it's interesting.

FYI @garath

Yes, we should document in https://github.com/dotnet/arcade/blob/master/Documentation/ArcadeSdk.md. Filed #1456

chcosta · 2018-12-05T16:25:31Z

eng/common/build.ps1

+    return
+  }
+
+  $script = Join-Path $EngRoot "restore-toolset.ps1"


I know Corefx is using the "configure-toolset" hook, do you have an example of a repo using "restore-toolset"? I'd like to see an example to solidify the difference between these two functions w.r.t. intent.

It's for restoring (installing) additional tools. E.g. https://github.com/dotnet/sdk/blob/master/eng/restore-toolset.ps1

chcosta · 2018-12-05T16:38:44Z

eng/common/tools.ps1

+
+  $finished = $false
+  try {
+    while (-not $process.WaitForExit(100)) { 


It might be nice to allow the wait time to be configured (either by command-line arg or env variable or other) for debugging purposes. I could imagine some repo doing some quirky stuff and then wanting to specify a longer wait time in configure-toolset.ps1 or something.

Couldn't you just modify the script directly if you're debugging some issue?
I'd rather not add switch that will be used once in our lifetime, if at all.

Yes, I had that thought as well. I think we want to discourage this practice as much as possible though because there's already grumbling about the confusion w.r.t. eng/common for people outside of the core Arcade working group and the fact that their "fixes" disappear. I was hoping to get ahead of that.

If it's only for debugging then it's good that the fixes disappear :)

In any case, let's wait until someone actually runs into this. We can always add more switches. It's harder to remove them.

chcosta · 2018-12-05T16:46:57Z

eng/common/tools.ps1

+  }
+}
+
+function GetMSBuildBinaryLogCommandLineArgument($arguments) {  


Oh, neat, I'm interpreting that this lets me do the thing I mentioned above (change binlog name), but in a slicker way? ie, you could do this...

build.cmd -binaryLog -restore /bl:restore.binlog build.cmd -binaryLog -build /bl:build.binlog

Nope, it just extracts the name of the log form the command line arguments (finds the /bl command line arg), so that it can be printed out when the build fails. Useful when msbuild spews a lot of output and you don't want to scroll all the way back and fish for the path to the log.

less neat :P

tmat · 2018-12-05T17:45:19Z

@chcosta BTW, the changes might need an update in CoreFX repo (and maybe other repos).

How should we deal with that? Should I merge this and wait for darc to automatically propagate the change and send auto-merging PRs? Then update CoreFX if needed?

chcosta · 2018-12-05T17:54:28Z

@chcosta BTW, the changes might need an update in CoreFX repo (and maybe other repos).
How should we deal with that? Should I merge this and wait for darc to automatically propagate the change and send auto-merging PRs? Then update CoreFX if needed?

@tmat, can you clarify exactly how this would surface in CoreFx (and other repos)?

@markwilkie thoughts? Sounds like this is potentially a slightly breaking change.

tmat · 2018-12-05T17:59:11Z

Actually, now I'm looking at CoreFX there should be no follow up needed. I thought it's using the MSBuild function directly but it is not.

tmat · 2018-12-05T18:01:11Z

BTW, almost any change in Arcade build scripts or targets files is potentially breaking due to the nature of powershell, bash and msbuild (lack of "internal" vs "public" accessibility modifiers). Some are just more likely to break something than others.

chcosta · 2018-12-05T18:14:06Z

Ack, it wasn't clear how this would break them. Thanks for checking on that. Agreed about the potentially breaking nature of these changes. Additionally, I want to be intentional if we know specifically that we are going to break someone about how we go about that.

tmat · 2018-12-05T18:21:34Z

@chcosta Agreed. If it's breaking API surface change then it should be caught by the CI build of the target repo. So I assume that the PR that darc sends would fail and need the fixes to be applied.

The problem is when a change is breaking in more subtle way - that is the CI passes, but the behavior changes and is not intended. Not sure we can do anything about that. Ultimately if CI passes and no one notices then arguable it does not matter or we have a test hole.

tmat · 2018-12-06T01:01:52Z

@chcosta I have added one more commit. Could you take a look?

Replace & PS operator with custom process launcher. The & operator does not allow to send colorized output to the host without polluting function return value. Automatically terminate script when MSBuild fails. Memoize results of Initialize* functions. Move build customization hooks to build.ps1. Make binary log off by default since it slows down build. It's on in CI build. Fixes to quiet restore workaround. Add set -u to bash script.

tmat force-pushed the MiscImprovements branch 5 times, most recently from e860608 to fc9e917 Compare December 4, 2018 22:41

tmat commented Dec 4, 2018

View reviewed changes

chcosta reviewed Dec 5, 2018

View reviewed changes

tmat force-pushed the MiscImprovements branch from b9dc76f to f47833a Compare December 5, 2018 18:02

tmat force-pushed the MiscImprovements branch 2 times, most recently from 098cffe to 51c03ea Compare December 6, 2018 01:01

tmat force-pushed the MiscImprovements branch from 51c03ea to 2565ef5 Compare December 6, 2018 02:00

chcosta approved these changes Dec 6, 2018

View reviewed changes

tmat force-pushed the MiscImprovements branch from 2565ef5 to 94baa84 Compare December 6, 2018 21:09

tmat closed this Dec 7, 2018

tmat reopened this Dec 7, 2018

tmat added 5 commits December 9, 2018 18:31

Disable .NET Core telemetry on CI.

6154494

Misc cleanup

5012323

Update comment

d2f55e8

Few more fixes

9655d05

tmat added 4 commits December 9, 2018 18:32

Fix eng_root

6009a50

Fixes

b898daa

fixes

bd9953e

tweak

0ee8eb6

tmat force-pushed the MiscImprovements branch from 9bc3851 to 0ee8eb6 Compare December 10, 2018 02:32

tmat merged commit ca315f2 into dotnet:master Dec 10, 2018

tmat deleted the MiscImprovements branch December 10, 2018 03:11

agocke assigned agocke and unassigned agocke Dec 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bootstrapper improvements. #1423

Bootstrapper improvements. #1423

tmat commented Dec 1, 2018 •

edited

Loading

tmat commented Dec 4, 2018

tmat Dec 4, 2018

livarcocc Dec 10, 2018

tmat Dec 10, 2018

tmat Dec 4, 2018

livarcocc Dec 10, 2018

tmat Dec 10, 2018

jaredpar Dec 10, 2018

tmat Dec 12, 2018

tmat Dec 4, 2018 •

edited

Loading

chcosta left a comment

chcosta Dec 5, 2018

tmat Dec 5, 2018

tmat Dec 5, 2018

chcosta Dec 5, 2018

tmat Dec 5, 2018

chcosta Dec 5, 2018

tmat Dec 5, 2018

chcosta Dec 5, 2018

tmat Dec 5, 2018

chcosta Dec 5, 2018

tmat Dec 5, 2018

chcosta Dec 5, 2018

tmat Dec 5, 2018

chcosta Dec 5, 2018

tmat commented Dec 5, 2018

chcosta commented Dec 5, 2018

tmat commented Dec 5, 2018

tmat commented Dec 5, 2018

chcosta commented Dec 5, 2018

tmat commented Dec 5, 2018 •

edited

Loading

tmat commented Dec 6, 2018 •

edited

Loading

Bootstrapper improvements. #1423

Bootstrapper improvements. #1423

Conversation

tmat commented Dec 1, 2018 • edited Loading

tmat commented Dec 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

chcosta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmat commented Dec 5, 2018

chcosta commented Dec 5, 2018

tmat commented Dec 5, 2018

tmat commented Dec 5, 2018

chcosta commented Dec 5, 2018

tmat commented Dec 5, 2018 • edited Loading

tmat commented Dec 6, 2018 • edited Loading

tmat commented Dec 1, 2018 •

edited

Loading

tmat Dec 4, 2018 •

edited

Loading

tmat commented Dec 5, 2018 •

edited

Loading

tmat commented Dec 6, 2018 •

edited

Loading