Skip to content

Conversation

@BrennanConroy
Copy link
Member

Hopefully this will have some changes for the ARM issue we're seeing in #24841

@BrennanConroy BrennanConroy added the tell-mode Indicates a PR which is being merged during tell-mode label Aug 20, 2020
@BrennanConroy BrennanConroy added this to the 5.0.0-rc1 milestone Aug 20, 2020
@BrennanConroy BrennanConroy requested review from a team and pranavkm August 20, 2020 17:50
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Aug 20, 2020
@pranavkm
Copy link
Contributor

💥

Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the latest SDK still has problems. Or, at least it can't work on the agents we're using.

/cc @dsplaisted @marcpopMSFT @sfoslund @clairernovotny @nkolev92

@dougbu
Copy link
Contributor

dougbu commented Aug 20, 2020

Put another way, dotnet/sdk#12897 didn't help us.

@BrennanConroy
Copy link
Member Author

Put another way, dotnet/sdk#12897 didn't help us.

Are we sure that change is in this version we're grabbing?

@dougbu
Copy link
Contributor

dougbu commented Aug 20, 2020

Not certain though the change was merged 4 days ago and the SDK used here is from this morning.

@marcpopMSFT
Copy link
Member

What version of VS is being used here? The problem may be that you need 16.8.p2 to work with this sdk. @nkolev92 , does that sound right? What is their path forward until that's available?

@BrennanConroy
Copy link
Member Author

What version of VS is being used here?

Shouldn't matter for the Unix machines that are failing with the same error.

@dougbu
Copy link
Contributor

dougbu commented Aug 20, 2020

What version of VS is being used here?

For example, from the macOS build:

/Users/runner/work/1/s/.dotnet/sdk/5.0.100-rc.1.20420.10/NuGet.targets(128,5): error : Invalid restore input. Invalid target framework 'unsupported'. Input files: /Users/runner/work/1/s/.packages/microsoft.dotnet.arcade.sdk/5.0.0-beta.20417.6/tools/Tools.proj. [/Users/runner/work/1/s/.packages/microsoft.dotnet.arcade.sdk/5.0.0-beta.20417.6/tools/Tools.proj]

It appears the .NET SDK contains a breaking change that required the dotnet/arcade@f8d1331#diff-49032945af4968a76e1ae76f37234d19 workaround. We need a new Arcade SDK to get things working with a newer .NET SDK.

@BrennanConroy I suggest triggering our Arcade subscription because they just promoted a new build. According to dotnet/arcade@83fda4b...56a95cc, the build includes the above workaround.

@BrennanConroy BrennanConroy changed the title Update SDK Update SDK and Arcade Aug 20, 2020
Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Withdrawing my changes request in hopes this works.

nit: I meant trigger-subscriptions not update-dependencies but am fine w/ keeping everything in this PR.

@BrennanConroy
Copy link
Member Author

Looks like some extra checks have been added. I'll fix the taskcontinuationoptions, not sure about netcoreapp3.0 yet.

@dougbu
Copy link
Contributor

dougbu commented Aug 20, 2020

Oops, new errors about targeting netcoreapp3.0 and a breaking change somewhere. But, looks better than the last run❕

@BrennanConroy BrennanConroy requested review from a team and SteveSandersonMS as code owners August 20, 2020 20:30
@BrennanConroy
Copy link
Member Author

This is going to be a fun PR 😰

{
var taskId = Interlocked.Increment(ref _nextPendingTaskId);
var tcs = new TaskCompletionSource<TValue>(TaskContinuationOptions.RunContinuationsAsynchronously);
var tcs = new TaskCompletionSource<TValue>(TaskCreationOptions.RunContinuationsAsynchronously);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a bunch of tests relied on this being inline instead of async

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests like Microsoft.JSInterop.Infrastructure.DotNetDispatcherTest.EndInvokeJS_WithNullValue are failing w/ expected true, got false. @pranavkm @captainsafia should we remove the async continuation, or fix up the tests?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove https://github.com/dotnet/aspnetcore/pull/25081/files#diff-799ff3281d6420f4ff7469e3402a2d25R29 whenever this question gets answered. #25111 fixed the netcoreapp3.0 issue.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @pranavkm @captainsafia can we get this resolved today?

@wtgodbe
Copy link
Member

wtgodbe commented Aug 21, 2020

IntrospectRazorTasksDllPath_DesktopMsBuild fails with:

error MSB4186: Invalid static method invocation syntax: "[MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')". Method '[MSBuild]::GetTargetFrameworkIdentifier' not found. Static method invocation should be of the form: $([FullTypeName]::Method()), e.g. $([System.IO.Path]::Combine(a, b)). Check that all parameters are defined, are of the correct type, and are specified in the right order.

failure is here

https://github.com/dotnet/sdk/blob/86ba2c3bdc5ab3eb71dfeebda5c51487ecce1328/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.TargetFrameworkInference.targets#L54

@wtgodbe
Copy link
Member

wtgodbe commented Aug 21, 2020

@sfoslund @wli3 looks like there was some churn there recently, could it be related to dotnet/sdk#12897?

@sfoslund
Copy link
Member

@wtgodbe that failure likely means the version of MSBuild that's being used isn't recent enough. Is it possible you might need to update the version of xcopy-msbuild that's being used here? It's 16.5.0-alpha right now which definitely doesn't have the necessary intrinsic functions.

@wtgodbe
Copy link
Member

wtgodbe commented Aug 21, 2020

Is it possible you might need to update the version of xcopy-msbuild that's being used here? It's 16.5.0-alpha right now which definitely doesn't have the necessary intrinsic functions.

Seems very possible - how can I know what a good version to use would be?

@dougbu is there a strong reason we're on that particular version of xcopy-msbuild?

@sfoslund
Copy link
Member

We're using 16.8.0-preview2.1, which was generated from last Friday's dogfood, so it has all the necessary nuget and MSBuild changes.

@wtgodbe
Copy link
Member

wtgodbe commented Aug 21, 2020

I will give that a try, thanks!

@dougbu
Copy link
Contributor

dougbu commented Aug 21, 2020

Is it possible you might need to update the version of xcopy-msbuild that's being used here?

We're not using the xcopy version of msbuild at all. That's there only to provide a fallback for users that don't have VS installed at all and are fine with excluding the native builds.

In fact, that version won't work at all for our builds due to our need for Microsoft.VisualStudio.Component.VC.ATL and Microsoft.VisualStudio.Component.VC.Tools.x86.x64. I tried extending the xcopy version's extension path and just ended up in a world of assembly load failures because VS on the agents was 16.5 or 16.6 at that point.

@wtgodbe your change isn't going to change anything because the build will successfully find the 16.5 VS installation and use msbuild from that.

Further, this whole conversation is about the desktop msbuild and we use dotnet msbuild except when building native projects and Wix projects. If that test invokes desktop msbuild, I recommend disabling it 'til the agents are updated.

@BrennanConroy BrennanConroy changed the title Update SDK and Arcade Update SDK Aug 22, 2020
@dougbu
Copy link
Contributor

dougbu commented Aug 23, 2020

  1. This PR still contains an Arcade SDK update. Should it❔
  2. Should this PR target release/5.0 at this point in the release❔

@BrennanConroy
Copy link
Member Author

BrennanConroy commented Aug 23, 2020

1. This PR still contains an Arcade SDK update. Should it❔

That's just because git isn't showing the PR rebased on release/5.0 correctly since the arcade update went in from a different PR.

2\. Should this PR target release/5.0 at this point in the release❔

Why not? Unless we're planning on indefinitely targeting an old SDK in 5.0, then we'll need to update it at some point and the earlier the better.

@dougbu
Copy link
Contributor

dougbu commented Aug 24, 2020

Why not? Unless we're planning on indefinitely targeting an old SDK in 5.0, then we'll need to update it at some point and the earlier the better.

Because we're in ask mode, the change is unreasonably painful, and moving to something other than (say) the stable / released RC1 SDK after that's out seems nice-to-have but not required.

@wtgodbe
Copy link
Member

wtgodbe commented Aug 24, 2020

We could go for the preview8 SDK once it releases

@wtgodbe wtgodbe mentioned this pull request Aug 24, 2020
@wtgodbe
Copy link
Member

wtgodbe commented Aug 24, 2020

We're going to punt this work until we get the RC1 SDK: #25214

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework tell-mode Indicates a PR which is being merged during tell-mode

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants