dotnet / sdk

Core functionality needed to create .NET Core projects, that is shared between Visual Studio and CLI
https://dot.net/core
MIT License
2.68k stars 1.06k forks source link

Workflow XAML compiler is broken with netstandard assemblies #1522

Open ericstj opened 7 years ago

ericstj commented 7 years ago

Repro:

  1. Create a workflow console application targeting net461.
  2. Add a reference to a netstandard1.5 or later library.

Expected: Compiles w/o warnings and runs successfully.

Actual: Warnings

Severity    Code    Description Project File    Line    Suppression State
Warning     Could not compile workflow expressions because file 'file:///C:\Program Files (x86)\Microsoft Visual Studio\IntPreview\Enterprise\MSBuild\Microsoft\Microsoft.NET.Build.Extensions\net461\ref\netfx.force.conflicts.dll' has an incorrect format. Workflows in this project may still run, if they do not require expression compilation. If the file is a platform-specific library or executable, consider building the project using MSBuild.exe from a command prompt of the targeted platform. WorkflowConsoleApplication1 C:\windows\Microsoft.NET\Framework\v4.0.30319\Microsoft.Xaml.targets    347 
Warning     Could not run workflow validation because file 'netfx.force.conflicts, PublicKeyToken=cc7b13ffcd2ddd51' has an incorrect format. This will not prevent workflows from running; but any workflow that has a validation error will fail at runtime. If the file is a platform-specific library or executable, consider building the project using MSBuild.exe from a command prompt of the targeted platform. WorkflowConsoleApplication1 C:\windows\Microsoft.NET\Framework\v4.0.30319\Microsoft.Xaml.targets    347 

Applying the netfx.force.conflicts workaround merely causes the warning to occur on the next reference dll (system.data.common).

When in this state workflows will not execute, see https://github.com/dotnet/corefx/issues/23439.

We may need to give up on passing reference assemblies to desktop projects. If that's the case we'd switch to just passing in the libs to the compiler and we'd need to build a version of netfx.force.conflicts.dll that doesn't have the reference assembly attribute.

/cc @dsplaisted @weshaggard

darrensteadman commented 7 years ago

The work around posted on my original bug is only partially working.

The netstandard2.0 assembly I'm referencing has a dependency on System.IO.Ports which isn't included in the msbuild refs folder and instead is brought down as a nuget package.

After applying the workaround I now get the following.

C:\WINDOWS\Microsoft.NET\Framework\v4.0.30319\Microsoft.Xaml.targets(347,5): warning : Could not run workflow validation because file 'System.IO.Ports, PublicKeyToken=cc7b13ffcd2ddd51' has an incorrect format. This will not prevent workflows from running; but any workflow that has a validation error will fail at runtime. If the file is a platform-specific library or executable, consider building the project using MSBuild.exe from a command prompt of the targeted platform. 1>C:\WINDOWS\Microsoft.NET\Framework\v4.0.30319\Microsoft.Xaml.targets(347,5): warning : Could not compile workflow expressions because file 'file:///C:\Users\username_here\.nuget\packages\System.IO.Ports\4.4.0\ref\net461\System.IO.Ports.dll' has an incorrect format. Workflows in this project may still run, if they do not require expression compilation. If the file is a platform-specific library or executable, consider building the project using MSBuild.exe from a command prompt of the targeted platform.

ericstj commented 7 years ago

I see. Yes, any package might use reference assemblies and cause this problem.

darrensteadman commented 7 years ago

Any chance you have a work around for this problem as it's going to cause us some big issues in the next few days if we can't get around it somehow?

ericstj commented 7 years ago

The big hammer would be to do something similar to what I did here, but for all NuGet references, right after the nuget target raises them to items. That can be limited to the items that have duplicates in Ref/CopyLocal with different files. I'll try to get to this later today.

darrensteadman commented 7 years ago

Awesome, thanks for looking into it.

ericstj commented 7 years ago

Here's what I came up with:

  <Target Name="ReplaceRefWithLib" BeforeTargets="ResolveAssemblyReferences">
    <ItemGroup>
      <_noCopyRefs Include="@(Reference)" Condition="'%(Reference.Private)' == 'false'"/>
      <_noCopyRefsByFileName Include="@(_noCopyRefs->'%(FileName)%(Extension)')">
        <OriginalItem>%(Identity)</OriginalItem>
      </_noCopyRefsByFileName>

      <_libByFileName Include="@(ReferenceCopyLocalPaths->'%(FileName)%(Extension)')">
        <OriginalItem>%(Identity)</OriginalItem>
      </_libByFileName>

      <_overlappingRefByFileName Include="@(_noCopyRefsByFileName)" Condition="'@(_noCopyRefsByFileName)' == '@(_libByFileName)' AND '%(Identity)' != ''" />
      <_overlappingLibByFileName Include="@(_libByFileName)" Condition="'@(_noCopyRefsByFileName)' == '@(_libByFileName)' AND '%(Identity)' != ''" />

      <_overlappingRef Include="@(_overlappingRefByFileName->'%(OriginalItem)')" />
      <_overlappingLib Include="@(_overlappingLibByFileName->'%(OriginalItem)')" />
    </ItemGroup>

    <ItemGroup Condition="'@(_overlappingRef)' != ''">
      <Reference Remove="@(_overlappingRef)" />
      <Reference Include="@(_overlappingLib)">
        <Private>false</Private>
      </Reference>
    </ItemGroup>
  </Target>

  <Target Name="RemoveNetFxForceConflicts" AfterTargets="ResolveAssemblyReferences">
    <ItemGroup>
      <ReferencePath Remove="@(ReferencePath)" Condition="'%(FileName)' == 'netfx.force.conflicts'" />
    </ItemGroup>
  </Target>

This is a bit of hammer, but its close to the behavior you get anyways with packages.config (barring any bugs in my target).

darrensteadman commented 7 years ago

Thanks, I'll give it a go first thing in the morning (UK) when I'm back in the office.

darrensteadman commented 7 years ago

It looks like that has solved our problem.

Will a global fix be put into a release at some point?

Thanks for all the help

ericstj commented 7 years ago

I think that's up to the SDK team. Follow this issue to determine what the resolution will be.

ericstj commented 7 years ago

I'm making the changes to the support package. Let this issue track ingesting those changes into the extensions. @dsplaisted can you help with that?

Let https://github.com/dotnet/corefx/issues/23830 track removing more reference assemblies from stand-alone packages (like System.IO.Ports).

dsplaisted commented 6 years ago

This should be fixed with #1612

darrensteadman commented 6 years ago

I'm not completely sure this is fixed.

If I remove the workaround from the csproj files it now compiles without any problems but it just throws exceptions at run time about netfx.force.conflicts. If I remove ReplaceRefWithLib I then get run time errors saying it can't load assemblies. Looking at those assemblies it appears they are ones that reference netstandard 2.0.

It kind of looks like the problem has moved from compile time to run time.

I'm running VS 2017 15.5.1

lmagyar commented 6 years ago

VS 15.6.6

@ericstj It's definitively not solved with #1612, this bug is still in the sdk! This issue should be reopened!

See dotnet/standard#704 dotnet/standard#706

Adding @ericstj's 'hammer' to the .csproj solves both of the issues above!!!

But this is still a bug in the sdk! Please reopen this issue!

ericstj commented 6 years ago

Based on the discussion in https://github.com/dotnet/standard/issues/704 the issue here is now that WorkflowXamlCompiler emits calls to load every assembly passed to it:

assemblyList.Add(Load("netfx.force.conflicts, Version=0.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51"));

So the minimal workaround for that issue is only the last portion of my previous change. This is less of a hammer:

  <Target Name="RemoveNetFxForceConflicts" AfterTargets="ResolveAssemblyReferences">
    <ItemGroup>
      <ReferencePath Remove="@(ReferencePath)" Condition="'%(FileName)' == 'netfx.force.conflicts'" />
    </ItemGroup>
  </Target>

We could add the same thing to the SDK targets. Alternatively we could try and get the Workflow compiler fixed to use assemblies which end up in the resultant assembly (or only assemblies with TypeDefs) but that feels like a harder thing to solve.

lmagyar commented 6 years ago

@ericstj This shorter hack doesn't solves dotnet/standard#706 (Could not compile workflow expressions because file <random .dll> has an incorrect format.) errors.

These are real compilation/build errors disguised as ~warnings~: the app can't run! We need the other <Target Name="ReplaceRefWithLib" BeforeTargets="ResolveAssemblyReferences">... also!

I don't know why, I don't know the build internals, but tested it just now.

Repro:

ericstj commented 6 years ago

Could not compile workflow expressions because file <random .dll> has an incorrect format.) errors.

Which DLLs are failing in this way? It is not random, https://github.com/dotnet/sdk/pull/1612 fixed some of them, but its possible you either don't have that fix or that other NuGet packages are contributing to the same issue.

lmagyar commented 6 years ago

It depends on what packages are used:

I use VS 15.6.6 now.

lmagyar commented 6 years ago

But what is really strange, if you check my repro project above, that even with the same nuget and direct assembly references within 2 projects in the solution, one project (HelloWorld.GrainImplementations) builds without these errors, but the other project (Arithmetical.GrainImplementations) (referring the same nugets, same assemblies, even inside the xaml, even the same namespaces in the xaml) have these build errors. And both have C# expressions inside the xaml.

It's trivial, there is some difference, but I couldn't figure it out.

ericstj commented 6 years ago

Looks to me like only Arithmetical references Microsoft.Orleans.Core.Legacy. That's likely the difference that causes the failure. I see that package references TypeExtensions in its closure.

Both of those packages you mention have reference assemblies that differ from implementation and use the ReferenceAssembly attribute which prevents them from loading for execution as the workflow compiler does. Http will be fixed by the SDK change, but only if you have a netstandard2.0 library in the closure.

I'm leaving this issue open as it seems that netfx.force.conflicts may need to be removed after RAR based on your observation that its causing a runtime failure (https://github.com/dotnet/standard/issues/704).

For the other packages that don't have newer versions without reference assemblies I've opened https://github.com/dotnet/corefx/issues/29037 to track a fix (at least for our CoreFx packages).

I think for now the workaround from this issue should still be used. If you wanted to constrain it a bit to make it less of a hammer, you could omit the ReplaceRefWithLib target, and instead use <PackageReference Include="System.Reflection.TypeExtensions" Version="4.4.0" ExcludeAssets="Compile" /> and do that for each package that caused a problem. This would only work if your Workflow project didn't need to use those assemblies directly (eg: may not be an option for Net.Http).

lmagyar commented 6 years ago

Really thank you for the fast investigation! It's nearly unbelievable to get a root cause so fast!

I've recompiled Orleans with <PackageReference Include="System.Reflection.TypeExtensions" Version="4.4.0" ExcludeAssets="Compile" />, and you were right!

The TypeExtensions error went away, but I've got the same with System.Runtime.CompilerServices.Unsafe. This is only indirectly referred by Orleans. So I think the big hack should be kept for a while.

AjRoBSeYeR commented 6 years ago

where can i put the code?

lmagyar commented 6 years ago

@AjRoBSeYeR Copy-Paste those 2 targets in https://github.com/dotnet/sdk/issues/1522#issuecomment-324141767 into your project's .csproj file.

iyhammad commented 6 years ago

Related issue https://github.com/dotnet/corefx/issues/30422

poakey commented 5 years ago

Will this be fixed any time soon? I notice it's been "Urgent" for ~2 years.

ericstj commented 5 years ago

We've done some work in CoreFx to limit our exposure of reference assemblies to .NET framework, but that's just a bandaid. Others still use reference assemblies and the Roslyn compiler now emits them. Ultimately I think a better fix here is for the workflow xaml compiler to take a fix similar to what WPF did to the xaml compiler this release: stop using runtime reflection that loads reference assemblies for execution and switch to using MetadataLoadContext. That would let the workflow compiler keep using System.Reflection APIs but they'd be backed with the metadata reader that only opens the files, not loading them for execution. @mconnew isn't Workflow open source now? Does that include WF Xaml compiler?

mconnew commented 5 years ago

@ericstj, the scenario here is running a netfx application with a workflow where some types referenced in the xaml file are located in assemblies targeting netstandard. My understanding is that we can't use MetadataLoadContext on .NET Framework as it's a .NET Core api.

The open source workflow implementation isn't a Microsoft owned project. You can find it here: https://github.com/UiPath/corewf. I don't know much about that project, whether it can even be used in a NetFx application or if it's only for use in .NET Core.

ericstj commented 5 years ago

My understanding is that we can't use MetadataLoadContext on .NET Framework as it's a .NET Core api.

This is incorrect. MetadataLoadContext is available as a package that targets .NETStandard2.0. This is what WPF is using in their latest Xaml compiler which runs on .NET Framework in the Visual studio process & MSBuild. https://github.com/dotnet/wpf/blob/7c5f0fdb9be097c478dfe77d70764eac0b43300c/src/Microsoft.DotNet.Wpf/src/PresentationBuildTasks/PresentationBuildTasks.csproj#L3 https://github.com/dotnet/wpf/blob/7c5f0fdb9be097c478dfe77d70764eac0b43300c/src/Microsoft.DotNet.Wpf/src/PresentationBuildTasks/PresentationBuildTasks.csproj#L311

mconnew commented 5 years ago

The Workflow compiler is part of the .NET Framework as WF xaml files can be created at runtime and consumed dynamically. I didn't think we had the ability to use nuget packages from .NET Framework code?

ericstj commented 5 years ago

I'm just giving a suggestion to update the Workflow XAML compiler to avoid loading assemblies for execution. There are a number of ways you can solve this, one of them is to use this new assembly. There are many other methods (ReflectionOnly load, CCI, Cecil, COM metadata APIs, LMR). A change could be done in the runtime implementation of the WF Xaml compiler and build task, or just build task (where the problem exists). @mconnew do you have a place to move this issue to? I don't think its technically a problem in the SDK.

mconnew commented 5 years ago

@ericstj, you are right, the problem isn't the SDK (other than the SDK is causing a scenario which isn't currently supported). The problem is in the .NET Framework itself as that's where the compiler is. This means most of your suggested methods aren't an option.
@dasetser and @HongGit, can you work out where this issue belongs (I'm not actually the owner of WF).

dasetser commented 5 years ago

@ericstj As Matt said, this is part of the .NET Framework. Since .NET Framework 4.8 is already released we don't have a release vehicle for a change like this. Unfortunately I don't think we'll be able to fix this at the WF XAML compiler level.

ericstj commented 5 years ago

You could port the WF XAML compiler to https://github.com/microsoft/msbuild just like other legacy tasks and targets that were burned into desktop.

livarcocc commented 5 years ago

Not sure I am supportive of that idea. I have been trying to avoid make the msbuild repo a place for these legacy task/targets, as the maintenance burden ends up landing on the msbuild team on the long term.

ericstj commented 5 years ago

My point is to illustrate that you don't need to update desktop in order to deliver fixes to the desktop build process. Where the code lives is a detail, could be another repo, could be that UIPath/corewf might deliver the fix and some tactical extension points are added to the MSBuild targets that import desktop WF tasks to let them do so. I bet this is going to matter when they start look at compiling workflow projects on .NETCore as well. /cc @dmetzgar

mconnew commented 5 years ago

@ericstj, as I said earlier:

WF xaml files can be created at runtime and consumed dynamically

So you can fix the build system to use an OOB Xaml compiler all you like, that would only fix the build time errors. You still would fail with loading a Xaml workflow at runtime. As we have the rehostable Workflow Designer that someone can include in their application, it's a very legitimate and often used use case to have raw Xaml files to be compiled at runtime.

Someone could have an existing compiled .NET Framework application today and they are given a Xaml file which references a .NET Standard targeted library. There's no build environment in this scenario. No SDK needs to be present. You will get the same problems compiling the Xaml file. Any fix to this needs to happen in .NET Framework, not the build system.

ericstj commented 5 years ago

You still would fail with loading a Xaml workflow at runtime.

It won't: runtime doesn't use reference assemblies.

You will get the same problems compiling the Xaml file. Any fix to this needs to happen in .NET Framework, not the build system.

You won't, at runtime you get runtime assemblies which can be loaded for execution.

ericstj commented 2 months ago

I updated the workaround above to make sure it works with some of the latest WCF packages, that seem to include PDBs and broke the old workaround.