-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Use PackageDownloadAndReference to avoid including msbuild dependencies #75561
Conversation
@ericstj ptal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like we're missing a feature in NuGet or the SDK somewhere for this...it'll work though!
...Workspaces/Core/MSBuild.BuildHost/Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.csproj
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please double check the package content before and after this change.
One trick I use is
build -pack /p:NuSpecOutputPath=artifacts\specs\before
git checkout myBranch
build -pack /p:NuSpecOutputPath=artifacts\specs\after
Then I diff the folders. While you're at it, also double check the deps file. If you'd like another set of eyes ping me and I'll have a look.
Source build isn't happy :( |
Yup, I did verify it's the same content (modulo SRM before last commit) |
</PropertyGroup> | ||
<ItemGroup Condition="'$(DotNetBuildSourceOnly)' == 'true'"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no reason why you need PackageReference
for sourcebuild. It should work just the same there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what happened, in SBRP the net472
asset is missing: https://github.com/dotnet/source-build-reference-packages/tree/main/src/referencePackages/src/microsoft.build.framework/17.3.4/ref
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind filing an issue? We can remove the condition here once fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I learned that source build isn't really meant to have .NETFramework targets so that might not have been the root cause, but I bet there is something like that - where the SBRP packages differ from the real packages in a way that causes an error
|
||
<ItemDefinitionGroup> | ||
<PackageDownloadAndReference> | ||
<Folder>lib/$(TargetFramework)</Folder> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is guaranteed to be lower case so no need for a defensive ToLowerInvariant()
call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We set the TFM, so I'd hope we use lower case.
...Workspaces/Core/MSBuild.BuildHost/Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.csproj
Show resolved
Hide resolved
...Workspaces/Core/MSBuild.BuildHost/Microsoft.CodeAnalysis.Workspaces.MSBuild.BuildHost.csproj
Show resolved
Hide resolved
Remaining test failure was flaky, merged over it so we can get .NET 9 SDK moving. |
No description provided.