dotnet / dotnet/msbuild

MSBuild project file anti-patterns: unquoted conditions, missing PrivateAssets, TargetFramework in .props

Open
#13,399 0 comments 4 reactions 2 assignees Claimed by @OvesN View on GitHub
Dominant language
C#
Stars
5.5k
Forks
1.5k
Avg merge
1d 8h
Merged PRs (30d)
141

Description

## Summary

An automated scan of the MSBuild project files (`.csproj`, `.props`, `.targets`) identified several anti-patterns that could lead to subtle build issues. The findings are organized by severity.

---

## 🔴 High Severity

### AP-02: Unquoted Condition Expression

**File:** `src/Directory.Build.props`, line 98

```xml

```

Both sides of the `==` comparison should be single-quoted. If `$(IsInnerBuild)` is empty or contains special characters, this condition can parse incorrectly.

**Fix:**
```xml

```

---

### AP-07: Missing `PrivateAssets="all"` on Analyzer Package

**File:** `eng/dependabot/Directory.Packages.props`, line 55

```xml

```

Analyzer packages should use `PrivateAssets="all"` to prevent them from flowing as transitive dependencies to consumers of published NuGet packages. The `StyleCop.Analyzers` entry on the next line already has this — likely an oversight.

**Fix:**
```xml

```

---

### AP-21: Property Conditioned on `TargetFramework` in `.props` Files

**Files:**
- `src/Directory.Build.props` — lines 69, 75, 85
- `eng/BootStrapMsBuild.props` — lines 13, 17

```xml




```

`$(TargetFramework)` is **not reliably set** during `.props` evaluation for single-targeting projects. It is only available as a global property for inner builds of multi-targeting (`TargetFrameworks`) projects. The repo has ~30 single-targeting projects (Package projects, test assets, utilities) where these conditions silently evaluate incorrectly:
- Line 69 condition is always `False` (even for `net472` single-target projects)
- Line 75 condition is always `True` (even for `net472` single-target projects)

**Recommended fix:** Move these `PropertyGroup` blocks to `src/Directory.Build.targets` or `src/Directory.BeforeCommon.targets` (which already contains similar TargetFramework-based conditions).

> **Note:** `ItemGroup` and `Target` conditions on `$(TargetFramework)` in `.props` are safe — only `PropertyGroup`/property conditions are affected.

---

## 🟡 Medium Severity

### AP-11: Custom Target Missing `Inputs`/`Outputs`

**File:** `src/MSBuild/MSBuild.csproj`, line 200

```xml

```

This file-copying target runs on every build. Adding `Inputs`/`Outputs` would allow MSBuild to skip it when files are up-to-date, improving incremental build performance.

### AP-16: PowerShell Exec for JSON Manipulation

**File:** `eng/Tools.props`, line 12

A complex PowerShell one-liner is used for JSON file manipulation in the `UpdateOptProfMetadataJson` target. This is not cross-platform and opaque to MSBuild logging. Low priority since it only runs in specific CI scenarios.

---

## ✅ Clean Areas

The scan found **no issues** for 14 other anti-pattern categories including: no hardcoded paths (AP-03), no legacy `packages.config` (AP-06), proper Central Package Management (AP-09), no side effects during evaluation (AP-19), and platform-guarded Exec commands (AP-20).

---

## Context

- **Scan scope:** 72 `.csproj`, 32 `.props`, 34 `.targets`, 6 `.proj` files
- **Excluded:** Test assets in `src/BuildCheck.UnitTests/TestAssets/` and `src/Build.UnitTests/TestAssets/` (intentionally non-standard for testing)
- **Anti-pattern catalog:** Based on the [MSBuild anti-patterns reference](https://learn.microsoft.com/en-us/visualstudio/msbuild/build-process-overview) covering AP-01 through AP-21

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.