Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[master] Update dependencies from dotnet/arcade #243

Merged
merged 4 commits into from
Nov 26, 2019

Conversation

dotnet-maestro[bot]
Copy link
Contributor

@dotnet-maestro dotnet-maestro bot commented Nov 25, 2019

This pull request updates the following dependencies

From https://github.com/dotnet/arcade

  • Build: 20191125.7
  • Date Produced: 11/26/2019 4:27 AM
  • Commit: 549475dff607986e6d9626f0dc8678e9736b7d0c
  • Branch: refs/heads/master
  • Updates:
    • Microsoft.DotNet.XUnitExtensions -> 5.0.0-beta.19575.7
    • Microsoft.DotNet.XUnitConsoleRunner -> 2.5.1-beta.19575.7
    • Microsoft.DotNet.VersionTools.Tasks -> 5.0.0-beta.19575.7
    • Microsoft.DotNet.ApiCompat -> 5.0.0-beta.19575.7
    • Microsoft.DotNet.Arcade.Sdk -> 5.0.0-beta.19575.7
    • Microsoft.DotNet.Build.Tasks.Configuration -> 5.0.0-beta.19575.7
    • Microsoft.DotNet.Build.Tasks.Feed -> 5.0.0-beta.19575.7
    • Microsoft.DotNet.Build.Tasks.Packaging -> 5.0.0-beta.19575.7
    • Microsoft.DotNet.Build.Tasks.SharedFramework.Sdk -> 5.0.0-beta.19575.7
    • Microsoft.DotNet.CodeAnalysis -> 5.0.0-beta.19575.7
    • Microsoft.DotNet.GenAPI -> 5.0.0-beta.19575.7
    • Microsoft.DotNet.GenFacades -> 5.0.0-beta.19575.7
    • Microsoft.DotNet.Helix.Sdk -> 5.0.0-beta.19575.7
    • Microsoft.DotNet.RemoteExecutor -> 5.0.0-beta.19575.7

…124.1

- Microsoft.DotNet.XUnitExtensions - 5.0.0-beta.19574.1
- Microsoft.DotNet.XUnitConsoleRunner - 2.5.1-beta.19574.1
- Microsoft.DotNet.VersionTools.Tasks - 5.0.0-beta.19574.1
- Microsoft.DotNet.ApiCompat - 5.0.0-beta.19574.1
- Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19574.1
- Microsoft.DotNet.Build.Tasks.Configuration - 5.0.0-beta.19574.1
- Microsoft.DotNet.Build.Tasks.Feed - 5.0.0-beta.19574.1
- Microsoft.DotNet.Build.Tasks.Packaging - 5.0.0-beta.19574.1
- Microsoft.DotNet.Build.Tasks.SharedFramework.Sdk - 5.0.0-beta.19574.1
- Microsoft.DotNet.CodeAnalysis - 5.0.0-beta.19574.1
- Microsoft.DotNet.GenAPI - 5.0.0-beta.19574.1
- Microsoft.DotNet.GenFacades - 5.0.0-beta.19574.1
- Microsoft.DotNet.Helix.Sdk - 5.0.0-beta.19574.1
- Microsoft.DotNet.RemoteExecutor - 5.0.0-beta.19574.1
@dotnet-maestro

This comment has been minimized.

@ViktorHofer
Copy link
Member

@safern @tmat what's the status regarding the break in Arcade?

@dotnet-maestro

This comment has been minimized.

@tmat
Copy link
Member

tmat commented Nov 25, 2019

Is this change included? dotnet/arcade#4424

@tmat
Copy link
Member

tmat commented Nov 25, 2019

Seems like it is :(.

@ViktorHofer
Copy link
Member

@tmat
Copy link
Member

tmat commented Nov 25, 2019

Is there a binlog for the failed build? I see just a text log file

@dotnet-maestro

This comment has been minimized.

@safern
Copy link
Member

safern commented Nov 25, 2019

I've been trying to repro this locally and haven't been able to. It seems like a race condition because of the BeforeTargets as the TargetPath doesn't exist yet as the Exec task logs Couldn't find the specified path.

@dotnet-maestro

This comment has been minimized.

@safern
Copy link
Member

safern commented Nov 25, 2019

Also note that it doesn't fail for the second inner build (net46) but it does for the first one (netstandard2.0). This project cross compiles for those two target frameworks.

@tmat
Copy link
Member

tmat commented Nov 25, 2019

The whole trouble with this target are the race conditions (or rather correct build target ordering).

@tmat
Copy link
Member

tmat commented Nov 25, 2019

I am running out of ideas :( We do chain after CoreBuild precisely to have the PDB available before the target runs.

@dotnet-maestro

This comment has been minimized.

@safern
Copy link
Member

safern commented Nov 25, 2019

In the meantime I set the installer.tasks as IsShipping=false so that this target doesn't run. We can re-enable once this is fixed.

a911e7c

Based on my understanding of the binlog the chaining is not happening correctly

image

CoreCompile seems to me to be running in parallel.

@dotnet-maestro

This comment has been minimized.

@safern
Copy link
Member

safern commented Nov 25, 2019

Is it because PrepareForRun is the one that actually copies over the output files to bin from obj?

image

And it is part of Compile not part of CoreCompile?

EDIT: I got confused you're chaining to CoreBuild not CoreCompile

@safern
Copy link
Member

safern commented Nov 25, 2019

I also see this message in the evaluation of the project:

The target "AfterBuild" listed in a BeforeTargets attribute at "D:\a\1\s.packages\microsoft.dotnet.arcade.sdk\5.0.0-beta.19574.1\tools\SymStore.targets (12,11)" does not exist in the project, and will be ignored.

@dotnet-maestro

This comment has been minimized.

@dagood
Copy link
Member

dagood commented Nov 25, 2019

Based on my understanding of the binlog the chaining is not happening correctly

CoreCompile seems to me to be running in parallel.

When an error happens, the binlog parenting gets really messed up, in my experience. But something to note is that targets within a single project never run in parallel, only projects can run in parallel to each other in MSBuild.

Setting IsShipping false makes sense to me. This may be another thing that is missing now that installer.tasks was moved and doesn't inherit src/installer/Directory.Build.* anymore. (It may have been fixed a different way there?)

@dotnet-maestro

This comment has been minimized.

@tmat
Copy link
Member

tmat commented Nov 25, 2019

Well, seems like more things are gonna be broken:
image

@safern
Copy link
Member

safern commented Nov 25, 2019

Setting IsShipping false makes sense to me. This may be another thing that is missing now that installer.tasks was moved and doesn't inherit src/installer/Directory.Build.* anymore. (It may have been fixed a different way there?)

I just searched for that in core-setup and it didn't define it. This target I believe was running differently. This is the change that caused this issue to show up: dotnet/arcade#4379

@tmat
Copy link
Member

tmat commented Nov 25, 2019

This might be one of the problems:

<MSBuild Projects="@(RepoTaskProjects)"
             Properties="Configuration=Debug;Platform=AnyCPU"
             Targets="Restore;Build"/>

AFAIK, it is not ok to run Restore and Build targets at the same time.

@dotnet-maestro

This comment has been minimized.

@ViktorHofer
Copy link
Member

ViktorHofer commented Nov 25, 2019

As was stated, you cannot parallelise at the task level or even at the target level. MSBuild only will build projects (i.e. MSBuild project files) in parallel.

from https://stackoverflow.com/questions/997047/how-to-run-tasks-in-parallel-in-msbuild

@tmat
Copy link
Member

tmat commented Nov 25, 2019

Can we move this project to Arcade repo to avoid this pre-build logic, as the comment in Build.props suggests?

 <!--
    Use this extensibility point to build custom tasks during Build.proj, before any restoring or
    building happens in the repo. These DLLs would ideally live in Arcade and be restored as tools,
    so the idea is building them here is somewhat equivalent.

    Also create the host RID props file so the main build can use it statically.

    Use synthetic inputs/outputs to avoid building it all the time. This should let devs build with
    MSBuild node reuse enabled (the Arcade default). If it were built every time, it would hit file
    locking issues vs. the persistent nodes that loaded the task DLL for the previous build. It
    isn't particularly accurate, but better than nothing.
  -->

@dotnet-maestro

This comment has been minimized.

@safern
Copy link
Member

safern commented Nov 25, 2019

Note that this is also happening for some of coreclr projects:

F:\workspace.5_work\1\s.packages\microsoft.dotnet.arcade.sdk\5.0.0-beta.19574.1\tools\SymStore.targets(57,5): error MSB3073: The command ""F:\workspace.5_work\1\s.packages\microsoft.diasymreader.pdb2pdb\1.1.0-beta2-19521-03\tools\Pdb2Pdb.exe" "F:\workspace.5_work\1\s\artifacts\bin\coreclr\Windows_NT.x86.Debug\runincontext.dll" /out "F:\workspace.5_work\1\s\artifacts\SymStore\Debug\runincontext\netcoreapp3.0\runincontext.pdb" /srcsvrvar SRC_INDEX=public" exited with code 3. [F:\workspace.5_work\1\s\src\coreclr\src\tools\runincontext\runincontext.csproj]

https://github.com/dotnet/runtime/blob/be980b71efadc622b5720a36867696758e59e71c/src/coreclr/src/build.proj#L10

@ViktorHofer
Copy link
Member

Can we move this project to Arcade repo to avoid this pre-build logic, as the comment in Build.props suggests?

Sure we can (and we should at some point) but how would that solve the issue?

@dotnet-maestro

This comment has been minimized.

@tmat
Copy link
Member

tmat commented Nov 25, 2019

Sure we can (and we should at some point) but how would that solve the issue?

I think the problem is in the way how build targets are invoked in Build.props. So, this would eliminate that.

I'd suggest separating Restore and Build for now.

 <MSBuild Projects="@(RepoTaskProjects)"
             Properties="Configuration=Debug;Platform=AnyCPU;__BuildPhase=Restore"
             Targets="Restore"/>
 <MSBuild Projects="@(RepoTaskProjects)"
             Properties="Configuration=Debug;Platform=AnyCPU;__BuildPhase=Build"
             Targets="Build"/>

Like we do in Arcade's build.proj. I remember keeping these together used to cause problems.

@dotnet-maestro

This comment has been minimized.

@safern safern force-pushed the darc-master-f0dd7523-7206-4385-9cd1-33e61a9ca531 branch from a911e7c to 3c6fe5f Compare November 25, 2019 19:25
@ViktorHofer
Copy link
Member

I'd suggest separating Restore and Build for now. I remember keeping these together used to case problems.

Thanks but I don't think that's the issue. I just opened the log in sequential order (a new feature in msbuild log viewer):

image

Also as stated above, there is no parallelism on a target level.

@safern
Copy link
Member

safern commented Nov 25, 2019

I'd suggest separating Restore and Build for now.

What about the coreclr projects that are also failing that I pointed above? Those don't have that behavior, so I think that's not the problem.

Should those not be ProjectReferences of build.proj?

@dotnet-maestro

This comment has been minimized.

@tmat
Copy link
Member

tmat commented Nov 25, 2019

If that is not the problem then I don't know what is. Up to you to figure out now. I am out of ideas.

@dotnet-maestro
Copy link
Contributor Author

dotnet-maestro bot commented Nov 25, 2019

Auto-Merge Status

This pull request has not been merged because Maestro++ is waiting on the following merge policies.

  • Waiting on checks: runtime-libraries, runtime-coreclr, runtime-coreclr (CoreCLR Pri0 Test Run Windows_NT arm checked), runtime-coreclr (CoreCLR Pri0 Test Run Windows_NT x86 checked), runtime-coreclr (CoreCLR Pri0 Test Run R2R Windows_NT x86 checked), runtime-coreclr (CoreCLR Pri0 Test Run Windows_NT arm64 checked), runtime-coreclr (CoreCLR Pri0 Test Run Linux arm64 checked), runtime-coreclr (CoreCLR Pri0 Test Run Windows_NT x64 checked), runtime-coreclr (CoreCLR Pri0 Test Run R2R Windows_NT x64 checked), runtime-coreclr (CoreCLR Pri0 Test Run Linux arm checked), runtime-coreclr (CoreCLR Pri0 Test Run Linux_musl x64 checked), runtime-coreclr (Test crossgen-comparison Linux arm checked), runtime-libraries (Windows Build x64_Debug)
  • ✔️ Standard Merge Policies Succeeded - No reviews have requested changes.

@safern safern force-pushed the darc-master-f0dd7523-7206-4385-9cd1-33e61a9ca531 branch from a1b3623 to 4f71435 Compare November 26, 2019 08:22
…125.7

- Microsoft.DotNet.XUnitExtensions - 5.0.0-beta.19575.7
- Microsoft.DotNet.XUnitConsoleRunner - 2.5.1-beta.19575.7
- Microsoft.DotNet.VersionTools.Tasks - 5.0.0-beta.19575.7
- Microsoft.DotNet.ApiCompat - 5.0.0-beta.19575.7
- Microsoft.DotNet.Arcade.Sdk - 5.0.0-beta.19575.7
- Microsoft.DotNet.Build.Tasks.Configuration - 5.0.0-beta.19575.7
- Microsoft.DotNet.Build.Tasks.Feed - 5.0.0-beta.19575.7
- Microsoft.DotNet.Build.Tasks.Packaging - 5.0.0-beta.19575.7
- Microsoft.DotNet.Build.Tasks.SharedFramework.Sdk - 5.0.0-beta.19575.7
- Microsoft.DotNet.CodeAnalysis - 5.0.0-beta.19575.7
- Microsoft.DotNet.GenAPI - 5.0.0-beta.19575.7
- Microsoft.DotNet.GenFacades - 5.0.0-beta.19575.7
- Microsoft.DotNet.Helix.Sdk - 5.0.0-beta.19575.7
- Microsoft.DotNet.RemoteExecutor - 5.0.0-beta.19575.7
@ViktorHofer ViktorHofer merged commit 3a9f8ae into master Nov 26, 2019
@ViktorHofer ViktorHofer deleted the darc-master-f0dd7523-7206-4385-9cd1-33e61a9ca531 branch November 26, 2019 14:35
@ViktorHofer
Copy link
Member

I'd suggest separating Restore and Build for now.

@tmat just wanted to say thank you for the recommendation, this helped in another case in coreclr where the design time properties and targets weren't imported correctly. I also found the issue that discusses the problem: dotnet/msbuild#2811

@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
@danmoseley danmoseley added area-codeflow for labeling automated codeflow and removed area-Infrastructure-libraries labels Jul 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-codeflow for labeling automated codeflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants