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

Workflow XAML compiler is broken with netstandard assemblies #1522

Open
ericstj opened this issue Aug 21, 2017 · 36 comments
Open

Workflow XAML compiler is broken with netstandard assemblies #1522

ericstj opened this issue Aug 21, 2017 · 36 comments
Assignees
Milestone

Comments

@ericstj
Copy link
Member

ericstj commented Aug 21, 2017

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
Copy link

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
Copy link
Member Author

ericstj commented Aug 22, 2017

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

@darrensteadman
Copy link

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
Copy link
Member Author

ericstj commented Aug 22, 2017

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
Copy link

Awesome, thanks for looking into it.

@ericstj
Copy link
Member Author

ericstj commented Aug 22, 2017

Here's what I came up with:

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

      <_libByFileName Include="@(ReferenceCopyLocalPaths->'%(FileName)')">
        <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
Copy link

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

@darrensteadman
Copy link

darrensteadman commented Aug 23, 2017

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 ericstj added this to the 2.1.0 milestone Aug 23, 2017
@ericstj
Copy link
Member Author

ericstj commented Aug 23, 2017

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

@ericstj
Copy link
Member Author

ericstj commented Sep 7, 2017

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
Copy link
Member

This should be fixed with #1612

@darrensteadman
Copy link

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
Copy link

lmagyar commented Apr 11, 2018

@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:

  • lmagyar/Orleans.Activities/tree/samples-xaml "permalink"
  • DO NOT update nuget packages to newer versions, use the referred ones! I will add the targets to later packages until the sdk is repaired!
  • To build: add the necessary targets here to the *.GrainImplementations projects
  • To run: first start manually Arithmetical.DevSiloHost wait 5s to start up, then start manually Arithmetical.DevClusterClient!

@ericstj
Copy link
Member Author

ericstj commented Apr 11, 2018

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, #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
Copy link

lmagyar commented Apr 11, 2018

It depends on what packages are used:

  • system.reflection.typeextensions 4.4.0 - when net461 Orleans.Activities 0.4.0-master-10143 is used, that uses netstandard2.0 Microsoft.Orleans.Core.Abstractions 2.0.0, that refers a great number of mixed packages
  • system.net.http 4.3.0 - when net461 Microsoft.Orleans.Core 1.5.3 is used, that refers a great number of mixed packages

I use VS 15.6.6 now.

@lmagyar
Copy link

lmagyar commented Apr 11, 2018

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
Copy link
Member Author

ericstj commented Apr 11, 2018

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 (dotnet/standard#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
Copy link

lmagyar commented Apr 11, 2018

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
Copy link

AjRoBSeYeR commented May 18, 2018

where can i put the code?

@lmagyar
Copy link

lmagyar commented May 18, 2018

@AjRoBSeYeR Copy-Paste those 2 targets in #1522 (comment) into your project's .csproj file.

@iyhammad
Copy link

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

@poakey
Copy link

poakey commented Aug 23, 2019

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

@ericstj
Copy link
Member Author

ericstj commented Aug 23, 2019

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
Copy link
Member

mconnew commented Aug 27, 2019

@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
Copy link
Member Author

ericstj commented Aug 27, 2019

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
Copy link
Member

mconnew commented Aug 28, 2019

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
Copy link
Member Author

ericstj commented Aug 28, 2019

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
Copy link
Member

mconnew commented Aug 28, 2019

@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
Copy link

@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
Copy link
Member Author

ericstj commented Aug 29, 2019

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
Copy link
Contributor

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
Copy link
Member Author

ericstj commented Aug 30, 2019

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
Copy link
Member

mconnew commented Aug 30, 2019

@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
Copy link
Member Author

ericstj commented Aug 30, 2019

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.

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

No branches or pull requests