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

Implement ExecCliBuild build check to warn if the Exec task is used to build a project #11523

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

IliaShuliatikov
Copy link

Implement ExecCliBuild build check to warn if the Exec task is used to build a project.

Fixes #11125

Context

It's usually a bad practice to build a project using an external tool from the Exec task (e.g. dotnet build) and the MSBuild task should be used instead, so that the MSBuild engine can track the process and files.

Testing

Implemented automated tests covering various build commands, possible false positive cases and verified functionality manually using the build output.

@IliaShuliatikov
Copy link
Author

@dotnet-policy-service agree

@dsplaisted
Copy link
Member

Thanks, great to see this! I'll let others who are more familiar with writing build checks actually review the code. At a glance I wonder if the string splitting and reformatting and the regex are going to have enough of a performance impact that we'd want to optimize that code more.

@IliaShuliatikov
Copy link
Author

At a glance I wonder if the string splitting and reformatting and the regex are going to have enough of a performance impact that we'd want to optimize that code more.

I assumed that running build checks (AFAIK, they are only run when building with the /check switch) and hitting the Exec target is a pretty rare case, so code simplicity would be preferable over performance. But I looked at the BuildCheck Design Spec one more time and if I got it right, it will be enabled by default, so it makes sense to optimize it as much as possible. Thanks for pointing this out, I optimized the implementation in the latest iteration to have better performance and zero allocation, please take a look.

I also prepared a performance benchmark to compare the original implementation with the latest iteration and confirm that we have zero allocations, here are the results:
Screenshot 2025-03-18 232323

Copy link
Member

@JanProvaznik JanProvaznik left a comment

Choose a reason for hiding this comment

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

lgtm, thank you for your contribution

@JanProvaznik JanProvaznik requested a review from Copilot March 20, 2025 12:46
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request introduces a new build check (ExecCliBuildCheck) that warns when the Exec task is improperly used to build projects instead of using the MSBuild task. The key changes include:

  • Adding unit tests to verify the ExecCliBuildCheck behavior.
  • Implementing the ExecCliBuildCheck which analyzes Exec task commands.
  • Updating documentation and the build check manager to include the new rule.

Reviewed Changes

Copilot reviewed 4 out of 18 changed files in this pull request and generated 3 comments.

File Description
src/BuildCheck.UnitTests/ExecCliBuildCheck_Tests.cs Adds tests validating the warning for build commands executed via the Exec task.
src/Build/BuildCheck/Checks/ExecCliBuildCheck.cs Implements the new build check logic to detect misuse of the Exec task for builds.
documentation/specs/BuildCheck/Codes.md Updates documentation to include the new BC0109 code.
src/Build/BuildCheck/Infrastructure/BuildCheckManagerProvider.cs Registers the new ExecCliBuildCheck in the list of built-in checks.
Files not reviewed (14)
  • src/Build/Resources/Strings.resx: Language not supported
  • src/Build/Resources/xlf/Strings.cs.xlf: Language not supported
  • src/Build/Resources/xlf/Strings.de.xlf: Language not supported
  • src/Build/Resources/xlf/Strings.es.xlf: Language not supported
  • src/Build/Resources/xlf/Strings.fr.xlf: Language not supported
  • src/Build/Resources/xlf/Strings.it.xlf: Language not supported
  • src/Build/Resources/xlf/Strings.ja.xlf: Language not supported
  • src/Build/Resources/xlf/Strings.ko.xlf: Language not supported
  • src/Build/Resources/xlf/Strings.pl.xlf: Language not supported
  • src/Build/Resources/xlf/Strings.pt-BR.xlf: Language not supported
  • src/Build/Resources/xlf/Strings.ru.xlf: Language not supported
  • src/Build/Resources/xlf/Strings.tr.xlf: Language not supported
  • src/Build/Resources/xlf/Strings.zh-Hans.xlf: Language not supported
  • src/Build/Resources/xlf/Strings.zh-Hant.xlf: Language not supported

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.

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