Repository navigation
Enforce BOM configuration in CI - #85810
mwiemer-microsoft wants to merge 18 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
|
dotnet-format will validate BOM and also fix if needed. Validate |
Updated usage instructions and default mode for validation. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
RE using |
UseDotNet ignores preview versions when resolving a global.json patch range. Install the exact SDK pinned by global.json instead so the PR correctness job can run the file-based BOM validator. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Might I suggest putting something in the copilot instructions too. BOM drift stopped entirely for me when I added a simple "Make sure the BOM of files doesn't change when making edits" to the general instructions in my GitHub Copilot App settings. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7e15d596-4d62-4a20-a6e8-7013c94a6aaa
| throw new InvalidOperationException(message); | ||
| } | ||
|
|
||
| file sealed class EditorConfig(bool isRoot, List<(string Pattern, string Charset)> rules) |
There was a problem hiding this comment.
Parsing editorconfigs here seems like too much work. Why do we need to do this and not just make some simple assumptions?
There was a problem hiding this comment.
I worry we're splitting hairs here, the whole run takes a total of 5 seconds and is deterministic and matches editorconfig, I think that'll be more future-proof than assumptions and won't save much time.
There was a problem hiding this comment.
I'm not worried about time complexity. I'm worried about having a 3rd editorconfig parser when none is needed.
There was a problem hiding this comment.
AI-generated:
Understood—this is about maintaining another parser, not runtime. I investigated reuse: the shipped Microsoft.CodeAnalysis.Common package exposes AnalyzerConfig.Parse and AnalyzerConfigSet.GetOptionsForSourcePath, so the validator could keep following EditorConfig while removing its custom parser. That introduces a package restore dependency for the currently BCL-only script; referencing compiler source instead would introduce a compiler build dependency.
Would reusing the shipped parser address your concern, or do you specifically prefer a standalone check with a fixed encoding policy? I haven't changed the implementation while that tradeoff is unresolved.
There was a problem hiding this comment.
Making the suggested change to use an existing parser, sry for the spam
There was a problem hiding this comment.
Moved BOM validation into the existing BuildBoss correctness tool and deleted the standalone script/custom EditorConfig parser. The checker uses the source-built Roslyn AnalyzerConfig / AnalyzerConfigSet APIs. Following the requested simplification, --check-bom is opt-in like --check-package-install, enabled only in Correctness_Build_Artifacts / Validate Build Artifacts. The latest BOM scan took approximately 14.4 seconds (the whole artifact-validation step took 80 seconds).
There was a problem hiding this comment.
Ref Jan's comment about being able to run this new check without a full build (resolved in favor of this thread)
There was a problem hiding this comment.
If this isn't a script that agents can run cheaply on changed files locally, it feels like it defeats the point. I really, truly think this should be an extremely simple script with a simple set of fixed rules and no dependencies that runs in a second or less on a set of changed files.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: de2a5cd9-3011-4137-a792-73888684eb49
Cover unexpected UTF-8 BOMs, charset-independent shebang checks, synced-source fallback, inherited unset overrides, and early eng/common exclusion. Keep synthetic repositories under test output and isolate Git failure fixtures from the enclosing checkout. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: de2a5cd9-3011-4137-a792-73888684eb49
This comment was marked as low quality.
This comment was marked as low quality.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 57cb40cd-77c8-4e5d-a730-6ebd04f40b39
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 57cb40cd-77c8-4e5d-a730-6ebd04f40b39
Remove the dedicated BuildBoss test project and enable BOM checking only in the artifact correctness job, alongside package installation validation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 57cb40cd-77c8-4e5d-a730-6ebd04f40b39
|
Most recent Copilot-recommended changes are all false positives :) |
Previously, no automated check enforced
.editorconfigcharset settings (utf-8versusutf-8-bom), allowing files to drift from their configured BOM policy.This corrects existing BOM mismatches and adds an opt-in
--check-bomcheck to BuildBoss. It checks tracked files with configured charset settings, rejects BOM-prefixed shebangs, and excludes eng/common.CI enables the check only in Correctness_Build_Artifacts → Validate Build Artifacts, alongside
--check-package-install. The BOM portion took approximately 17 seconds in build 1635887; the entire artifact-validation step took 94 seconds.Main file: src/Tools/BuildBoss/BomCheckerUtil.cs. About 350 additions (and the same number of deletions) are just fixing BOM drift across that many files. Some editorconfigs have been updated to match the most popular actual BOM setting in their directories.
Microsoft Reviewers: Open in CodeFlow