Skip to content

Fix toolset package failing to build from clean state in VS #78251

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Apr 22, 2025
Merged

Fix toolset package failing to build from clean state in VS #78251

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4950cbe
Fix toolset package not building from clean state in VS
jjonescz Apr 22, 2025
ccbab07
Simplify the build targets for copying files
jasonmalinowski Apr 22, 2025
2d939b2
Fix indenting
jasonmalinowski Apr 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions ...Get/Microsoft.Net.Compilers.Toolset/AnyCpu/Microsoft.Net.Compilers.Toolset.Package.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,13 @@

This package is primarily intended as a method for rapidly shipping hotfixes to customers. Using it as a long term solution for providing newer compilers on older MSBuild installations is explicitly not supported. That can and will break on a regular basis.

The supported mechanism for providing new compilers in a build enviroment is updating to the newer .NET SDK or Visual Studio Build Tools SKU.
The supported mechanism for providing new compilers in a build environment is updating to the newer .NET SDK or Visual Studio Build Tools SKU.
Copy link
Member

Choose a reason for hiding this comment

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

Not that there was any doubt, but I did a git blame and yeah this was my fault.


This package requires either MSBuild 16.3 and .NET Desktop 4.7.2+ or .NET Core 2.1+

$(RoslynPackageDescriptionDetails)
</PackageDescription>
<TargetsForTfmSpecificContentInPackage>$(TargetsForTfmSpecificContentInPackage);_GetFiles</TargetsForTfmSpecificContentInPackage>
<!-- Remove NU5128 once https://github.com/NuGet/Home/issues/8583 is fixed -->
<NoWarn>$(NoWarn);NU5100;NU5128</NoWarn>

Expand Down Expand Up @@ -61,10 +62,12 @@
<TfmSpecificPackageFile Include="@(_FileWithPath)" PackagePath="%(_FileWithPath.TargetPath)" />
</ItemGroup>

<!-- Used for debugging parts of the toolset (e.g., build tasks). -->
<!-- Used for debugging parts of the toolset (e.g., build tasks).
Doesn't work in VS (where `<ProjectReference Targets="Publish" />` doesn't have any effect), hence we set `ContinueOnError="true"`. -->
Copy link
Member

Choose a reason for hiding this comment

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

D'oh I forgot to delete this comment.

<Copy SourceFiles="@(_FileWithPath)"
DestinationFiles="$(BaseOutputPath)$(Configuration)\%(_FileWithPath.TargetPath)"
SkipUnchangedFiles="true" />
SkipUnchangedFiles="true"
ContinueOnError="true" />
Copy link
Member

Choose a reason for hiding this comment

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

Should we instead condition on Condition="'$(DesignTimeBuild)' != 'true'" - or does it actually need to run?

Copy link
Member

Choose a reason for hiding this comment

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

Ah we don't want this running in design time builds either. That AfterTargets="Build" really needs to go since that's still going to cause issues here. I'm going to make another push to this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Condition="'$(DesignTimeBuild)' != 'true'" sounds like a good idea. I'm not sure why AfterTargets="Build" is bad (in this scenario), but feel free to improve.

Copy link
Member

@jasonmalinowski jasonmalinowski Apr 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

why AfterTargets="Build" is bad

It's easier to delete it than it is to convince ourselves it's correct in all situations. 😄 The problem is there's enough special types of builds that do strange things with project references that it's just not really safe to make the assumption. For example, you can also do a command line build passing a property to not build dependencies, and I'm not sure what that would do.

So yes, we could add a condition to remove DesignTimeBuilds, but if we just delete the AfterTargets="Build" we don't have to do anything in the first place. Less is more.

Copy link
Member

Choose a reason for hiding this comment

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

There is one potential workflow change if you did like the ability to just do a build and see the files; that said we're adding I/O for copies for the 99.9% of the time people don't need that. If we wanted to refactor this a bit we could have an explicit target do the copy and you can do that.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not just that I like the workflow, I've added it so it can be used for debugging, e.g.:

roslyn/.vscode/launch.json

Line 103 in 489d016

"-p:RoslynTargetsPath=${workspaceFolder}/artifacts/bin/Microsoft.Net.Compilers.Toolset.Package/Debug/tasks/netcore",

That being said, there could probably be a property to opt in to it when needed.

</Target>

<Import Project="..\DesktopCompilerArtifacts.targets" Condition="'$(TargetFramework)' == 'net472'" />
Expand Down