Local live builds not picking up runtime config #2093

jaredpar · 2020-01-23T18:45:09Z

The local live builds were not properly picking up the runtime
configuration varaible. This meant that libraries always attempted to
build against a Debug CoreCLR even if the developer specified
-runtimeConfiguration Release

The local live builds were not properly picking up the runtime configuration varaible. This meant that libraries always attempted to build against a Debug CoreCLR even if the developer specified `-runtimeConfiguration Release`

jaredpar · 2020-01-23T18:46:03Z

FYI @ManickaP

jaredpar · 2020-01-23T18:48:30Z

Appears this behavior was broken by #1934

safern · 2020-01-23T18:50:41Z

eng/liveBuilds.targets

  <PropertyGroup>
    <CoreCLROSGroup Condition="'$(CoreCLROSGroup)' == ''">$(OSGroup)</CoreCLROSGroup>
-    <CoreCLRConfiguration Condition="'$(CoreCLRConfiguration)' == ''">$(Configuration)</CoreCLRConfiguration>
+    <CoreCLRConfiguration Condition="'$(CoreCLRConfiguration)' == ''">$(RuntimeConfiguration)</CoreCLRConfiguration>


What if RuntimeConfiguration is not used? CoreCLRConfiguration would be empty in that case.

Also the same should apply for MonoConfiguration, it should take RuntimeConfiguration as the configuration if it was passed globally.

Hmm ... all that logic really needs to be hoisted up so both the live and CI builds share it. Let me do that.

What if RuntimeConfiguration is not used? CoreCLRConfiguration would be empty in that case.

When will this be the case though? All of our build scripts explicitly pass /p:RuntimeConfiguration.

What if I just do: build.cmd (wanting to build everything on debug), or build.cmd -c Release wanting to build all in Release. That will not pass down RuntimeConfiguration. That is actually why the runtime-live-build is failing in CI because, in those builds we don't use -runtimeConfiguration.

Which I mentioned offline, we should exercise that in that build, we should have a leg for combining the subset's configurations around.

That was what I was thinking as well. Still don' tthink it's complete though because same problem applies: what if $(Configuration) is empty?

Again this comes back to what are the defaults. We should fall back to them when the build doesn't specify them.

Configuration will never be empty, Configuration is always passed down by arcade, default is Debug.

https://github.com/dotnet/runtime/blob/master/eng/common/build.ps1#L3

Also the property group in Subsets.props has the same issue fi RuntimeConfiguration is unspecified. Overall it's unclear why we repeat this property group between Directory.Build.props and SubSets.props. Seems much simpler to move this property group above the imports of Subsets.props and avoid repeating this logic

This is the property group I'm referring to that should be moved https://github.com/dotnet/runtime/blob/master/Directory.Build.props#L55

- Ensure that `$(RuntimeConfiguration)` is always set - Remove duplicated logic in Subsets.props

jaredpar · 2020-01-23T20:22:26Z

eng/liveBuilds.targets

+    <CoreCLRConfiguration Condition="'$(CoreCLRConfiguration)' == ''">$(RuntimeConfiguration)</CoreCLRConfiguration>
    <MonoOSGroup Condition="'$(MonoOSGroup)' == ''">$(OSGroup)</MonoOSGroup>
-    <MonoConfiguration Condition="'$(MonoConfiguration)' == ''">$(Configuration)</MonoConfiguration>
+    <MonoConfiguration Condition="'$(MonoConfiguration)' == ''">$(RuntimeConfiguration)</MonoConfiguration>


Actually I think these sets of $(CoreCLRConfiguration) and $(MonoConfiguration) can be deleted entirely. These always flow from Directory.Build.props

safern · 2020-01-23T20:58:37Z

Directory.Build.props

+  <!-- Honor the generic RuntimeConfiguration property. -->
+  <PropertyGroup>
+    <RuntimeConfiguration Condition="'$(RuntimeConfiguration)' == ''">$(Configuration)</RuntimeConfiguration>
+    <RuntimeConfiguration Condition="'$(RuntimeConfiguration)' == ''">Debug</RuntimeConfiguration>


RuntimeConfiguration will not be empty here because $(Configuration) is always passed as Debug by default if it is not specified.

safern

There is 2 other instances of CoreCLRConfiguration which are wrong and shouldn't be there in my opinion:

runtime/src/libraries/shims/manual/Directory.Build.props

Lines 16 to 19 in cd622cd

    
           <CoreCLROSGroup>$(_bc_OSGroup)</CoreCLROSGroup> 
        
           <CoreCLRConfiguration>$(_bc_ConfigurationGroup)</CoreCLRConfiguration> 
        
           <MonoOSGroup>$(_bc_OSGroup)</MonoOSGroup> 
        
           <MonoConfiguration>$(_bc_ConfigurationGroup)</MonoConfiguration>

runtime/src/libraries/Directory.Build.targets

Lines 3 to 6 in cd622cd

    
           <CoreCLROSGroup Condition="'$(CoreCLROSGroup)' == ''">$(_bc_OSGroup)</CoreCLROSGroup> 
        
           <CoreCLRConfiguration Condition="'$(CoreCLRConfiguration)' == ''">$(_bc_ConfigurationGroup)</CoreCLRConfiguration> 
        
           <MonoOSGroup Condition="'$(MonoCLROSGroup)' == ''">$(_bc_OSGroup)</MonoOSGroup> 
        
           <MonoConfiguration Condition="'$(MonoConfiguration)' == ''">$(_bc_ConfigurationGroup)</MonoConfiguration>

jaredpar · 2020-01-23T21:53:17Z

@safern

Yeah those locations seem to be dead code to. Going to delete them.

Change the live / live jobs so they mix release and debug configurations. This will help ensure we avoid future breaks in this area

src/libraries/Directory.Build.targets

jaredpar · 2020-01-24T17:00:25Z

Interesting. Builds are all working locally just fine with this change and the build is succeeding in CI in that the debug and release are combining together. Yet somehow I've broken the loading of the Roslyn compiler. Lots of the following:

C:\Users\vsagent.nuget\packages\microsoft.net.compilers.toolset\3.5.0-beta1-19606-04\tasks\netcoreapp2.1\Microsoft.CSharp.Core.targets(59,5): error MSB4062: The "Microsoft.CodeAnalysis.BuildTasks.Csc" task could not be loaded from the assembly C:\Users\vsagent.nuget\packages\microsoft.net.compilers.toolset\3.5.0-beta1-19606-04\build..\tasks\netcoreapp2.1\Microsoft.Build.Tasks.CodeAnalysis.dll. Assembly with same name is already loaded Confirm that the declaration is correct, that the assembly and all its dependencies are available, and that the task contains a public class that implements Microsoft.Build.Framework.ITask. [F:\workspace_work\1\s\src\libraries\System.Security.Principal.Windows\ref\System.Security.Principal.Windows.csproj]
C:\Users\vsagent.nuget\packages\microsoft.net.compilers.toolset\3.5.0-beta1-19606-04\tasks\netcoreapp2.1\Microsoft.CSharp.Core.targets(59,5): error MSB4062: The "Microsoft.CodeAnalysis.BuildTasks.Csc" task could not be loaded from the assembly C:\Users\vsagent.nuget\packages\microsoft.net.compilers.toolset\3.5.0-beta1-19606-04\build..\tasks\netcoreapp2.1\Microsoft.Build.Tasks.CodeAnalysis.dll. Assembly with same name is already loaded Confirm that the declaration is correct, that the assembly and all its dependencies are available, and that the task contains a public class that implements Microsoft.Build.Framework.ITask. [F:\workspace_work\1\s\src\libraries\System.ComponentModel.Composition\ref\System.ComponentModel.Composition.csproj]

Generally that means we're attempting to load the Roslyn build task from two different locations during the build. Unsure how my changes could impact where Roslyn is loading from. Hoping I can reproduce this on my machine, or at least the binary log will give me more insight into where we do the different loads so I can track it down. There is no explicit <UsingTask ...> in the runtime code though which should lead to this.

safern

Thanks for fixing!

jaredpar · 2020-01-25T18:18:01Z

Got the live / live build job to upload the binary log from the failures here and that's helping narrowing down the problem a bit. What is apparently happening here is there are multiple invocations of dotnet build which end up using a different version of the Microsoft.Net.Compilers.Toolset package.

The standard build is using the package specified in Tools.props:

C:\Users\vsagent.nuget\packages\microsoft.net.compilers.toolset\3.5.0-beta1-19606-04\tasks\netcoreapp2.1\Microsoft.CSharp.Core.targets(59,5): error MSB4062: The "Microsoft.CodeAnalysis.BuildTasks.Csc" task could not be loaded from the assembly C:\Users\vsagent.nuget\packages\microsoft.net.compilers.toolset\3.5.0-beta1-19606-04\build..\tasks\netcoreapp2.1\Microsoft.Build.Tasks.CodeAnalysis.dll. Assembly with same name is already loaded Confirm that the declaration is correct, that the assembly and all its dependencies are available, and that the task contains a public class that implements Microsoft.Build.Framework.ITask. [F:\workspace_work\1\s\src\libraries\System.Security.Principal.Windows\ref\System.Security.Principal.Windows.csproj]

This error indicates MSBuild is loading the C# targets from the Tools.props version of Microsoft.Net.Compilers.Toolset. The targets file is then erroring because the task it's trying to load is conflicting with one that is already loaded into the node. Likely what's happening here is the current dotnet build is re-using a node from a previous dotnet build which does not override Microsoft.Net.Compilers.Toolset.

Looking at some other builds I can see the non-custom version being loaded here

/__w/3/s/.dotnet/sdk/5.0.100-alpha1-015772/Roslyn/Microsoft.Managed.Core.targets(104,5): error MSB4062: The "Microsoft.CodeAnalysis.BuildTasks.MapSourceRoots" task could not be loaded from the assembly /__w/3/s/.dotnet/sdk/5.0.100-alpha1-015772/Roslyn/Microsoft.Build.Tasks.CodeAnalysis.dll. Assembly with same name is already loaded Confirm that the declaration is correct, that the assembly and all its dependencies are available, and that the task contains a public class that implements Microsoft.Build.Framework.ITask. [/__w/3/s/src/mono/netcore/System.Private.CoreLib/System.Private.CoreLib.csproj]
##[error].dotnet/sdk/5.0.100-alpha1-015772/Roslyn/Microsoft.Managed.Core.targets(104,5): error MSB4062: (NETCORE_ENGINEERING_TELEMETRY=) The "Microsoft.CodeAnalysis.BuildTasks.MapSourceRoots" task could not be loaded from the assembly /__w/3/s/.dotnet/sdk/5.0.100-alpha1-015772/Roslyn/Microsoft.Build.Tasks.CodeAnalysis.dll. Assembly with same name is already loaded Confirm that the declaration is correct, that the assembly and all its dependencies are available, and that the task contains a public class that implements Microsoft.Build.Framework.ITask.

Trying to locate these other invocations of dotnet build to see why they're not using our version of Microsoft.Net.Compilers.Toolset.

Still quite unsure how my changes could cause this node re-use problem though. 😦

trylek · 2020-01-27T18:00:24Z

@janvorli - haven't you been recently looking into a similar issue? The last time I saw this fail was during test build because the Common folder wasn't excluded from the wrapper folder list but I believe that has been fixed already.

janvorli · 2020-01-27T18:15:04Z

Yes, this has been broken for me locally since December for build.cmd / build-test.cmd . It happens because some phases of the build pick the Microsoft.Build.Tasks.CodeAnalysis.dll from the .dotnet subfolder of the repo and some from a nuget package installed on my machine.
I've merged in a partial fix for the problem that fixed most of the problems and got me to the state where I am able to build the tests - the only catch is that it always fails during the first build after I clean up the repo, the following builds work. I was able to identify that the issue is triggered when test_dependencies.csproj needs to be rebuilt. That pulls in the incorrect Microsoft.Build.Tasks.CodeAnalysis.dll (from the .dotnet subfolder). Next build finds that the output already exists, so it doesn't invoke the csc task, the wrong Microsoft.Build.Tasks.CodeAnalysis.dll doesn't get pulled in and the rest of the test build stuff works.

AFAIK @ViktorHofer's pending change #901 is supposed to ultimately fix that.

safern · 2020-01-27T18:45:28Z

eng/pipelines/common/global-build-job.yml


    # Build
-    - script: $(Build.SourcesDirectory)$(dir)build$(scriptExt) -c ${{ parameters.buildConfig }} -runtimeConfiguration ${{ parameters.runtimeConfig }} -arch ${{ parameters.archType }}
+    - script: $(Build.SourcesDirectory)$(dir)build$(scriptExt) -c ${{ parameters.buildConfig }} -runtimeConfiguration ${{ parameters.runtimeConfig }} -arch ${{ parameters.archType }} -bl


I would instead recommend using the -ci flag which does extra logging, clean-up, makes sure the dotnet we acquired is on the path and isolate the nuget cache, plus, it generates binlogs.

safern · 2020-01-27T18:48:12Z

eng/pipelines/common/global-build-job.yml

+    - script: $(Build.SourcesDirectory)$(dir)build$(scriptExt) -c ${{ parameters.buildConfig }} -runtimeConfiguration ${{ parameters.runtimeConfig }} -arch ${{ parameters.archType }} -bl -ci
      displayName: Build product
+
+    - task: PublishBuildArtifacts@1


Instead of having to add this step, you can just pass down a parameter to the template we use which is job.yml from arcade. That parameter is enablePublishBuildArtifacts: true.

https://github.com/dotnet/arcade/blob/master/eng/common/templates/job/job.yml#L180

Note that you also need to declare a variable _BuildConfig: ${{ parameters.buildConfig }} because it uses it to find the logs:
https://github.com/dotnet/arcade/blob/master/eng/common/templates/job/job.yml#L184

This job though deliberately builds multiple configurations though hence seems like it can't directly hook up to that infrastructure

It can hook up to that infrastructure, the binlog will be created under the global buildConfig which is passed through the -configuration argument. The binlog name and location is calculated on the build script in arcade, which doesn't know about runtimeConfiguration. However if you want to produce binlogs for other steps in the coreclr build scripts, then we will definitely not be able to use this.

Mono currently relies on ArtifactsObjDir when setting BaseIntermediateOutputPath. Not setting it leads to restore issues.

ViktorHofer · 2020-01-28T17:13:28Z

eng/Configurations.props

  <!-- Honor the generic RuntimeConfiguration property. -->
  <PropertyGroup>
    <RuntimeConfiguration Condition="'$(RuntimeConfiguration)' == ''">$(ConfigurationGroup)</RuntimeConfiguration>
+    <RuntimeConfiguration Condition="'$(RuntimeConfiguration)' == ''">$(Configuration)</RuntimeConfiguration>


This isn't necessary as ConfigurationGroup and Configuration are always passed in together. In future we will probably remove the former.

Not necessarily. If you build the projects directly then ConfigurationGroup is not specified.

Also looking through the logs @jkoritzinsky and I observed places where $(ConfigurationGroup) was not set but $(Configuration) was. This change was inspired by that observation. It's deep in the restore graph where we encountered this.

Concrete example where only $(Configuration) is passed to the build

https://teams.microsoft.com/l/message/19:b27b36ecd10a46398da76b02f0411de7@thread.skype/1580491671080?tenantId=72f988bf-86f1-41af-91ab-2d7cd011db47&groupId=014ca51d-be57-47fa-9628-a15efcc3c376&parentMessageId=1580481868103&teamName=dotnet%2Fruntime%20repo&channelName=Infrastructure&createdTime=1580491671080

jaredpar · 2020-01-30T22:12:07Z

@jkoritzinsky @rainersigwald and I finally got to the bottom of why restore started failing in the face of my change. It really comes down to the following block of code (as existed before my PR):

  <ItemDefinitionGroup Condition="'$(CoreCLRConfiguration)' != ''">
    <CoreClrProjectToBuild>
      <Properties>Configuration=$(CoreCLRConfiguration)</Properties>
    </CoreClrProjectToBuild>
  </ItemDefinitionGroup>

The problem my PR is addressing is that $(CoreCLRConfiguration) was not guaranteed to be set in the local developer live builds. Upon closer inspection it turns out the property is almost never set in that scenario which means this ItemDefinitionGroup never actually executed in the context of MSBuild.

My PR though enforced that $(CoreCLRConfiguration) was always set to a value, even if it was the default one. That means the ItemDefinitionGroup finally started executing. The key though is this portion:

      <Properties>Configuration=$(CoreCLRConfiguration)</Properties>

This does not add the value $(Configuration) to the built projects. Instead it makes $(Configuration) the only property that is set for the build. All of the values being passed down from arcade, including $(__BuildPhase) were effectively being deleted. That meant MSBuild began caching evaluations where arcade explicitly depended on them not being cached and lead to all the failures that we saw.

The correct way to do this is to use AdditionalProperties instead of Properties:

  <ItemDefinitionGroup Condition="'$(CoreCLRConfiguration)' != ''">
    <CoreClrProjectToBuild>
      <AdditionalProperties>Configuration=$(CoreCLRConfiguration)</AdditionalProperties>
    </CoreClrProjectToBuild>
  </ItemDefinitionGroup>

safern · 2020-01-30T22:59:06Z

Oh wow... that was an interesting one.

Thanks for the detailed explanation.

safern

Nice, thanks for all the hard work to getting this properly fixed

safern · 2020-01-31T00:35:20Z

Merging since test failures are known failures.

Local live builds not picking up runtime config

a962c75

The local live builds were not properly picking up the runtime configuration varaible. This meant that libraries always attempted to build against a Debug CoreCLR even if the developer specified `-runtimeConfiguration Release`

Dotnet-GitSync-Bot added the area-Infrastructure-libraries label Jan 23, 2020

safern reviewed Jan 23, 2020

View reviewed changes

PR Feedback:

976b7ea

- Ensure that `$(RuntimeConfiguration)` is always set - Remove duplicated logic in Subsets.props

jaredpar commented Jan 23, 2020

View reviewed changes

Remove unnecessary properties

95495dc

safern reviewed Jan 23, 2020

View reviewed changes

jaredpar added 2 commits January 23, 2020 15:24

Progress

5740d55

Mix configurations

e641b28

Change the live / live jobs so they mix release and debug configurations. This will help ensure we avoid future breaks in this area

safern reviewed Jan 24, 2020

View reviewed changes

src/libraries/Directory.Build.targets Show resolved Hide resolved

jaredpar added 2 commits January 23, 2020 20:50

Undo MonoOsGorup delete

9977e65

Correct the import order

46d25fe

safern approved these changes Jan 24, 2020

View reviewed changes

Publish binary logs

9dbbce8

Pass -ci as this is a CI build

3e634eb

safern reviewed Jan 27, 2020

View reviewed changes

jaredpar and others added 3 commits January 27, 2020 11:18

Progress

f0dbb6f

Test

84727f5

Set ArtifactsObjDir early enough for Mono

47f84a0

Mono currently relies on ArtifactsObjDir when setting BaseIntermediateOutputPath. Not setting it leads to restore issues.

ViktorHofer reviewed Jan 28, 2020

View reviewed changes

Don't overwrite the properties

0164e80

safern approved these changes Jan 30, 2020

View reviewed changes

safern merged commit 33a6ed5 into master Jan 31, 2020

safern deleted the live-config branch January 31, 2020 00:35

jaredpar mentioned this pull request Jan 31, 2020

Broke ability to use RuntimeFlavor in local builds #23800

Closed

ghost locked as resolved and limited conversation to collaborators Dec 11, 2020

	<CoreCLROSGroup>$(_bc_OSGroup)</CoreCLROSGroup>
	<CoreCLRConfiguration>$(_bc_ConfigurationGroup)</CoreCLRConfiguration>
	<MonoOSGroup>$(_bc_OSGroup)</MonoOSGroup>
	<MonoConfiguration>$(_bc_ConfigurationGroup)</MonoConfiguration>

	<CoreCLROSGroup Condition="'$(CoreCLROSGroup)' == ''">$(_bc_OSGroup)</CoreCLROSGroup>
	<CoreCLRConfiguration Condition="'$(CoreCLRConfiguration)' == ''">$(_bc_ConfigurationGroup)</CoreCLRConfiguration>
	<MonoOSGroup Condition="'$(MonoCLROSGroup)' == ''">$(_bc_OSGroup)</MonoOSGroup>
	<MonoConfiguration Condition="'$(MonoConfiguration)' == ''">$(_bc_ConfigurationGroup)</MonoConfiguration>

Local live builds not picking up runtime config #2093

Local live builds not picking up runtime config #2093

Uh oh!

Conversation

jaredpar commented Jan 23, 2020

Uh oh!

jaredpar commented Jan 23, 2020

Uh oh!

jaredpar commented Jan 23, 2020

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

safern left a comment

Choose a reason for hiding this comment

Uh oh!

jaredpar commented Jan 23, 2020

Uh oh!

Uh oh!

jaredpar commented Jan 24, 2020

Uh oh!

safern left a comment

Choose a reason for hiding this comment

Uh oh!

jaredpar commented Jan 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trylek commented Jan 27, 2020

Uh oh!

janvorli commented Jan 27, 2020

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!

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!

jaredpar commented Jan 30, 2020

Uh oh!

safern commented Jan 30, 2020

Uh oh!

safern left a comment

Choose a reason for hiding this comment

Uh oh!

safern commented Jan 31, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

jaredpar commented Jan 25, 2020 •

edited

Loading