Skip to content

[BuildCheck Suggestion]: Detect use of Exec task to run "dotnet build" or similar commands #11125

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

Jump to bottom
Closed
dsplaisted opened this issue Dec 11, 2024 · 8 comments · Fixed by #11523
Closed

[BuildCheck Suggestion]: Detect use of Exec task to run "dotnet build" or similar commands #11125

dsplaisted opened this issue Dec 11, 2024 · 8 comments · Fixed by #11523
Labels
Area: BuildCheck backlog BuildCheck Suggestion Suggestion for a built in MSBuild analyzer. Label should be applied together with 'Area: BuildCheck' help wanted Issues that the core team doesn't plan to work on, but would accept a PR for. Comment to claim. internal-team-onboarding triaged

Comments

@dsplaisted
Copy link
Member

Summary

We should warn when an Exec task is used to spawn a new build via dotnet build, dotnet publish, etc.

Background and Motivation

It's usually a bad pattern to run dotnet build, dotnet publish, etc from an Exec task, since this spawns an entirely separate build that the MSBuild engine doesn't have visibility over. We should recommend using an MSBuild task instead.

Sample issue or antipattern that the check should be flagging

  <Target Name="BuildVer2022">
    <Exec Command="dotnet build -p:C3DVersion=Ver2022 -c $(Configuration)"/>
  </Target>

Source: dotnet/sdk#45034 (comment)

Sample output

No response
@dsplaisted dsplaisted added Area: BuildCheck BuildCheck Suggestion Suggestion for a built in MSBuild analyzer. Label should be applied together with 'Area: BuildCheck' labels Dec 11, 2024
@surayya-MS surayya-MS added help wanted Issues that the core team doesn't plan to work on, but would accept a PR for. Comment to claim. backlog internal-team-onboarding triaged labels Dec 16, 2024
@IliaShuliatikov
Copy link
Contributor

Hi @dsplaisted, @surayya-MS, I am interested in working on this issue. Considering it has been open for some time now and has both help wanted and internal-team-onboarding labels, could you please let me know if it is available for contribution? If yes, could you please assign it to me? Thank you

@dsplaisted
Copy link
Member Author

@baronfel @rainersigwald Do we have guidance for folks that want to contribute build checks? Ilya would like to help out with this one.

@IliaShuliatikov
Copy link
Contributor

@dsplaisted, thanks. Considering that BuildCheck is a relatively new feature, this seems pretty straightforward to me, so while any guidance would be appreciated, it's not necessary. I can start right away, just wanted to confirm that it's available for external contribution, given that it has an "internal" label.

@baronfel
Copy link
Member

I think the main things that will be interesting are:

  • where the checks should live in this repo, how they get distributed in the overall SDK layout
  • where in the SDK layout we should insert the calls to the RegisterBuildCheck intrinsic function to load the check(s) in the specified assembly.

After that, it should just be a matter of trial-and-error to get the semantics we want on the check itself!

@IliaShuliatikov
Copy link
Contributor

@baronfel, not sure if I got this right, but do you mean that this check should not be one of the built-in build checks which are being registered by default in the RegisterBuiltInChecks internal method?

@baronfel
Copy link
Member

Oh I'm sorry - I thought I was in the SDK repo. You're exactly right - there are no barriers at all to creating a built-in buildcheck. Carry on!

@IliaShuliatikov IliaShuliatikov mentioned this issue Feb 28, 2025
Merged
@IliaShuliatikov
Copy link
Contributor

@dsplaisted, @baronfel, I prepared PR #11523 implementing the new build check, could you please take a look?

@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Apr 1, 2025 •
edited
Loading

#11523 detects nuget restore too. That is a bit of a special case because nuget restore is how to restore NuGet packages referenced by a packages.config file in a .NET Framework ASP.NET web site that does not have any MSBuild project files (unlike an ASP.NET web application project). I have solution-level MSBuild code in which <Target Name="Restore"> runs nuget restore on such web sites. I don't agree that MSBuild should warn about this use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area: BuildCheck backlog BuildCheck Suggestion Suggestion for a built in MSBuild analyzer. Label should be applied together with 'Area: BuildCheck' help wanted Issues that the core team doesn't plan to work on, but would accept a PR for. Comment to claim. internal-team-onboarding triaged
Projects
None yet
Milestone
No milestone
5 participants
@dsplaisted @baronfel @IliaShuliatikov @KalleOlaviNiemitalo @surayya-MS