Skip to content

Conversation

@donnie-msft
Copy link
Contributor

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/1816

Regression? Last working version:

Description

Similar to #4751, MSBuild does not support runtime scenarios where the Microsoft.Build DLL is placed in an output directory.
See https://docs.microsoft.com/en-us/visualstudio/msbuild/updating-an-existing-application?view=vs-2022#use-nuget-packages-preferred

  • Since PackageReference cannot have use the Condition on Configuration property, I removed it, knowing that the compiler will trim unused references.
  • IlmergeCommand had to be pointed to the NET Framework path when running from .NET Core, because its behavior is to look for assemblies in the specified paths. Now that I've removed MSBuild's runtime assembly, it needs to know where to look.
    • GeneratePathProperty is a pretty cool feature, which enables me to do this :) Thanks @jeffkl !

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A
  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@donnie-msft donnie-msft requested a review from a team as a code owner August 16, 2022 19:32
Comment on lines +126 to +128
/lib:$(PkgMicrosoft_Build_Utilities_Core)\lib\netstandard2.0 ^
/lib:$(PkgMicrosoft_Build_Tasks_Core)\lib\netstandard2.0 ^
/lib:$(PkgMicrosoft_Build_Framework)\lib\netstandard2.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ILMerge must be pointed to the MSBuild assemblies when using .NET Core.

Comment on lines +42 to +44
<PackageReference Include="Microsoft.Build.Framework" ExcludeAssets="runtime" GeneratePathProperty="true" />
<PackageReference Include="Microsoft.Build.Tasks.Core" ExcludeAssets="runtime" GeneratePathProperty="true" />
<PackageReference Include="Microsoft.Build.Utilities.Core" ExcludeAssets="runtime" GeneratePathProperty="true" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

GeneratePathProperty required here due to the ILMerge used later.

dominoFire
dominoFire previously approved these changes Aug 16, 2022
Copy link
Contributor

@dominoFire dominoFire left a comment

Choose a reason for hiding this comment

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

LGTM. If MSBuild.Locator assembly is causing trouble, we will know at dotnet insertion time.

@donnie-msft donnie-msft requested review from jeffkl and nkolev92 August 17, 2022 04:35
jeffkl
jeffkl previously approved these changes Aug 17, 2022
@donnie-msft donnie-msft dismissed stale reviews from jeffkl and dominoFire via 2e6f74b August 17, 2022 19:48
@donnie-msft donnie-msft requested review from nkolev92 and removed request for nkolev92 August 17, 2022 19:48
nkolev92
nkolev92 previously approved these changes Aug 17, 2022
@ghost ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Aug 24, 2022
@ghost
Copy link

ghost commented Aug 24, 2022

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

@donnie-msft donnie-msft force-pushed the dev-donnie-msft-excludeMsBuildAssets branch from 2e6f74b to 7951f94 Compare August 26, 2022 19:38
@ghost ghost removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Aug 26, 2022
@zivkan
Copy link
Member

zivkan commented Aug 26, 2022

@donnie-msft I just want to make sure my comment #4761 (comment) isn't missed.

It's not only about whether the compiler adds a reference to msbuild locator dll in our assembly, but also whether our package lists the locator as a dependency when it's inserted int he .NET SDK.

I think we should re-instate the condition, so our package doesn't have a dependency on MSBuild Locator.

@donnie-msft
Copy link
Contributor Author

donnie-msft commented Aug 26, 2022

I just want to make sure my comment #4761 (comment) isn't missed.

@zivkan I created a meeting for next week so we can discuss this with @jeffkl.

Also replied in the other thread: #4761 (comment)

@donnie-msft donnie-msft force-pushed the dev-donnie-msft-excludeMsBuildAssets branch from 7951f94 to 52a0615 Compare August 30, 2022 00:19
@donnie-msft donnie-msft merged commit 3924481 into dev Aug 30, 2022
@donnie-msft donnie-msft deleted the dev-donnie-msft-excludeMsBuildAssets branch August 30, 2022 05:09
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 this pull request may close these issues.

6 participants