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

Potential incompatibility with MSBuild 17.7 #154

Closed
onyxmaster opened this issue Aug 9, 2023 · 21 comments · May be fixed by #155
Closed

Potential incompatibility with MSBuild 17.7 #154

onyxmaster opened this issue Aug 9, 2023 · 21 comments · May be fixed by #155

Comments

@onyxmaster
Copy link

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

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

AraHaan commented Aug 16, 2023

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

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

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

@JanKrivanek
Copy link

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

@robmaas
Copy link

robmaas commented Sep 25, 2023

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

AraHaan commented Sep 27, 2023

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

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

jaredpar commented Oct 5, 2023

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

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

AraHaan commented Oct 6, 2023

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

robmaas commented Oct 20, 2023

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

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

AraHaan commented Oct 24, 2023

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

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

AraHaan commented Oct 24, 2023

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

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

robmaas commented Nov 15, 2023

Warnings no longer show up in VS 17.8.0.

@shadowano
Copy link

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

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

@jaredpar jaredpar closed this as not planned Won't fix, can't repro, duplicate, stale Nov 27, 2023
@AraHaan
Copy link

AraHaan commented Dec 6, 2023

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

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

Successfully merging a pull request may close this issue.