Potential incompatibility with MSBuild 17.7

onyxmaster commented 1 year ago

Hi.

After upgrading to the latest VS 17.7, (MSBuild 17.7.2+d6990bcfa) the build of the project that references the NuGet package v4.1.0 emits the following messages:

.nuget\packages\microsoft.codedom.providers.dotnetcompilerplatform\4.1.0\build\net472\Microsoft.CodeDom.Providers.DotNetCompilerPlatform.targets(13,9): message : MSB4120: Item 'RoslynCompilerFiles' definition within target references itself via (qualified or unqualified) metadatum 'RecursiveDir'. This can lead to unintended expansion and cross-applying of pre-existing items. More info: https://aka.ms/msbuild/metadata-self-ref
.nuget\packages\microsoft.codedom.providers.dotnetcompilerplatform\4.1.0\build\net472\Microsoft.CodeDom.Providers.DotNetCompilerPlatform.targets(13,9): message : MSB4120: Item 'RoslynCompilerFiles' definition within target references itself via (qualified or unqualified) metadatum 'Filename'. This can lead to unintended expansion and cross-applying of pre-existing items. More info: https://aka.ms/msbuild/metadata-self-ref
.nuget\packages\microsoft.codedom.providers.dotnetcompilerplatform\4.1.0\build\net472\Microsoft.CodeDom.Providers.DotNetCompilerPlatform.targets(13,9): message : MSB4120: Item 'RoslynCompilerFiles' definition within target references itself via (qualified or unqualified) metadatum 'Extension'. This can lead to unintended expansion and cross-applying of pre-existing items. More info: https://aka.ms/msbuild/metadata-self-ref

Should I be worried about this? Maybe this should be fixed?

fabr2004 commented 1 year ago

Same here. Happened with versions 3.6.0, 3.11.0 and 4.1.0.

The mentioned code is:

<Target Name="SetRoslynCompilerFiles" >
    <Message Text="Using Roslyn from '$(RoslynToolPath)' folder" />
    <ItemGroup>
        <RoslynCompilerFiles Include="$(RoslynToolPath)\*">
            <Link>roslyn\%(RecursiveDir)%(Filename)%(Extension)</Link>
        </RoslynCompilerFiles>
    </ItemGroup>
</Target>

AraHaan commented 1 year ago

The fix to that is to change that to the following and it will properly copy in both Build and publish again:

<Target Name="SetRoslynCompilerFiles" >
    <Message Text="Using Roslyn from '$(RoslynToolPath)' folder" />
    <ItemGroup>
        <RoslynCompilerFiles Include="$(RoslynToolPath)\**\*" />
    </ItemGroup>
</Target>

After that then this will work properly again:

  <Target Name="CopyRoslynCompilerFilesToOutputDirectory" AfterTargets="CopyFilesToOutputDirectory" DependsOnTargets="LocateRoslynToolsDestinationFolder;SetRoslynCompilerFiles" Condition="$(RoslynCopyToOutDir) == 'true'">
    <Copy SourceFiles="@(RoslynCompilerFiles)" DestinationFolder="$(RoslynToolsDestinationFolder)" ContinueOnError="true" SkipUnchangedFiles="true" Retries="0" />
    <ItemGroup  Condition="'$(MSBuildLastTaskResult)' == 'True'" >
      <FileWrites Include="$(RoslynToolsDestinationFolder)\*" />
    </ItemGroup>
  </Target>

In the mean time I had to do this to try to work around the issue in the .NET SDK though which sadly is not the best way of doing it (best way is fixing the package so I can re-enable to copy from the package itself and not manually do it):

  <Target Name="FixPublish" AfterTargets="Publish" DependsOnTargets="LocateRoslynToolsDestinationFolder">
    <Message Text="The .NET SDK's Web SDK sort of broke publish by placing the build outputs inside of '$(PublishDir)' and refusing to copy roslyn instead of in '$(PublishDir)bin' and '$(PublishDir)bin\roslyn'." Importance="high" />
    <Message Text="Which resulted in the need for an after publish target that gets ran in Visual Studio and from 'dotnet publish'." Importance="high" />
    <Message Text="I have also filed an issue about it inserting ASP.NET Core stuff in web.config as well even when targeting .NET Framework with the .NET SDK when using the Web SDK and have yet to get a response." Importance="high" />
    <Message Text="For more information go to: https://github.com/dotnet/sdk/issues/34595" Importance="high" />
    <ItemGroup>
      <CustomRoslynFiles Include="$(RoslynToolPath)\**\*" />
    </ItemGroup>
    <Copy SourceFiles="@(CustomRoslynFiles)" DestinationFolder="$(RoslynToolsDestinationFolder)" ContinueOnError="true" SkipUnchangedFiles="true" Retries="0" />

    <!--
        Remove the refs folder from the publish output.
        This is getting annoying that even with ProduceReferenceAssembly and ProduceReferenceAssemblyInOutDir
        set to false that it STILL puts them in the publish directory.
    -->
    <RemoveDir Directories="$(PublishDir)refs" />
    <!-- Move DLL and PDB files to the bin folder. -->
    <ItemGroup>
      <!-- Include *.dll, *.pdb, and *.config files excluding web.config -->
      <WebPublishBinFiles Include="$(PublishDir)*.dll;$(PublishDir)*.pdb;$(PublishDir)*.config" Exclude="$(PublishDir)web.config" />
    </ItemGroup>
    <Copy SourceFiles="@(WebPublishBinFiles)" DestinationFolder="$(PublishDir)\bin" ContinueOnError="true" SkipUnchangedFiles="true" Retries="0" />
    <Delete Files="@(WebPublishBinFiles)" />
  </Target>

Without this fix it would refuse to do the copy, with the fix it will copy again in both non-SDK style (hopefully) and SDK style.

onyxmaster commented 1 year ago

@terrajobst Immo, sorry for summoning you here, but maybe we can get a yes/no reply from someone in charge of this library? There's a lot of "old" ASP.NETters using new version of VS =)

danpetitt commented 1 year ago

Agreed @onyxmaster messing with external target files doesnt solve the problem and shouldnt be an acceptable fix

JanKrivanek commented 1 year ago

While this is genuine missuse of msbuild and ideally the offending target (SetRoslynCompilerFiles) would be updated, we are demoting this message in https://github.com/dotnet/msbuild/pull/9228, which is currently scheduled for 17.8

robmaas commented 1 year ago

@JanKrivanek demoting the message seems fair. Nevertheless I would like the maintainers of this repo to update the code and merge #155 and release new versions fixing the issue itself. They do not seem to react at all, can you maybe escalate or something?

AraHaan commented 1 year ago

Also not to mention the offending target is unable to copy the files at all due to the issue and I feel demoting the message would be a mistake instead as it would fail to motivate people to fix the root issue at all.

NickCraver commented 1 year ago

@terrajobst @jaredpar Is this something we could please route for the next release? I'm coming across it as well - note fix is pending in #155.

jaredpar commented 1 year ago

Is this something we could please route for the next release?

While this does involve Roslyn it's not a project that I'm a maintainer on. Looks like @StephenMolloy is the active contributor here so would need them to comment.

They do not seem to react at all, can you maybe escalate or something?

To be clear: there is no behavior change here. The package is working the same as it did before. The message is calling out a long existing behavior (based on my understanding here). While this is a new message the behavior hasn't changed. Agree a fix would be nice but it doesn't seem critical for the upcoming VS update.

JanKrivanek commented 1 year ago

To be clear: there is no behavior change here. The package is working the same as it did before. The message is calling out a long existing behavior (based on my understanding here)

Yes - this is correct

AraHaan commented 1 year ago

Which is odd because on my project the message explains why the compiler files refused to copy to the proper location (bin\roslyn) until my fix is applied (and this is with a .NET Framework ASP.NET project migrated in place to the .NET SDK.

Specifically project was initially made with:

ASP.NET MVC5 (I think 5) .NET Framework 4.7.2 with authentication and identity.

And then migrated in place to sdk style csproj.

robmaas commented 1 year ago

Agree a fix would be nice but it doesn't seem critical for the upcoming VS update.

Critical? No, it's not critical but I don't see why it being critical should be a requirement for this to be resolved. Is there a reason why this isn't getting merged? Especially since the fix has already been provided weeks ago by @AraHaan .

jzabroski commented 1 year ago

Not sure but I think this is causing issues with our ASP.NET Classic Website-style project. It's annoying at minimum, but it may be causing spurious build issues I can't consistently reproduce when switching git branches where transitive dependencies are different.

AraHaan commented 1 year ago

Not sure but I think this is causing issues with our ASP.NET Classic Website-style project. It's annoying at minimum, but it may be causing spurious build issues I can't consistently reproduce when switching git branches where transitive dependencies are different.

This is another reason why I submitted a patch, it is insane how one must hack together a temporary .NET SDK for projects to an in place upgrade to SDK style of projects just to fix this issue. Which I feel is a hack itself. https://www.nuget.org/packages/Elskom.NetFX.Sdk.Web/ For those who are interested in this temporary solution which also fixes a few other issues I faced when doing an in place migration to SDK style of projects. Feel free to leave feedback here if this helped resolve your issues for now.

Also I am tempted to just simply copy the C# code from here and release my own version of this package that:

implements the 2 pull requests
has a system in place to automatically update the compiler via pull request automation and deployments 😄. Because I really would love a newer compiler and the option to do /langver:latest for my razor pages in ASP.NET MVC that targets .NET Framework.
can be used as a drop in replacement to this which looks to be unmaintained now and low priority for MS to fix any and all issues in it.

CZEMacLeod commented 1 year ago

@AraHaan Interesting you have an SDK for this - can you confirm if there is anything in there that is not captured in https://github.com/CZEMacLeod/MSBuild.SDK.SystemWeb which seems to cover most of these issues already. I'm happy to take feedback and PRs for any specific problems.

AraHaan commented 1 year ago

@AraHaan Interesting you have an SDK for this - can you confirm if there is anything in there that is not captured in https://github.com/CZEMacLeod/MSBuild.SDK.SystemWeb which seems to cover most of these issues already. I'm happy to take feedback and PRs for any specific problems.

You looked at every line in the repository associated with the package I linked above? I find it kinda hard to compare between the 2 SDKs.

CZEMacLeod commented 1 year ago

@AraHaan I've scanned through it and I think it mostly matches up with the basics covered in the one I published about 2 years ago. I haven't included the automatic switch to 4.7.0 of the compilers yet (for frameworks that support it), but you can manually select the version number of the compilers package with a single property. It also better supports launch profiles and some other stuff, and was curious why you would create your own, when a solution already exists? If there is something that is not included in MSBuild.SDK.SystemWeb then I'd like to add it so that all the users of it can benefit.

robmaas commented 1 year ago

Warnings no longer show up in VS 17.8.0.

shadowano commented 1 year ago

Even Azure team's own .Net Framework example does not run in VS 17.7.7.

I've seen that (before changing target file) if doing a CLEAN in Visual studio and then build/rebuild, it generates the Roslyn files in the bin folder, but when running the app the files get's deleted so the app fails because it can't find csc.exe. In order to run this example I had to modify the target file as per @AraHaan comment

jaredpar commented 12 months ago

Closing this issue as there is no regression here and the warning was removed in 17.8.

AraHaan commented 11 months ago

Ot is still a problem however as there is still no files that gets copied to the expected output location.

aspnet / RoslynCodeDomProvider

Potential incompatibility with MSBuild 17.7 #154