Test for line endings and long paths - #85127
Conversation
Co-authored-by: mwiemer-microsoft <80539004+mwiemer-microsoft@users.noreply.github.com>
Co-authored-by: mwiemer-microsoft <80539004+mwiemer-microsoft@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
@RikkiGibson requesting review :) |
mwiemer-microsoft
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds a new unit test class under compiler test infrastructure intended to validate developer/environment prerequisites: Windows long-path support and platform-appropriate line endings.
Changes:
- Add a Windows-only test that reads
HKLM\SYSTEM\CurrentControlSet\Control\FileSystem\LongPathsEnabledand asserts it is enabled. - Add a cross-platform test that reads a source file and asserts its line endings match the current platform (CRLF on Windows, LF elsewhere).
File summaries
| File | Description |
|---|---|
| src/Compilers/Core/CodeAnalysisTest/DeveloperSettingsTests.cs | Introduces new environment validation tests for long paths (Windows) and line endings (platform-dependent). |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
|
@copilot fix the reported issues in comments one-by-one and have a model from a second family review each change before committing it. Do not worry about the failing CI / PR checks at this point. |
Co-authored-by: mwiemer-microsoft <80539004+mwiemer-microsoft@users.noreply.github.com>
Agent-Logs-Url: https://github.com/dotnet/roslyn/sessions/33d2d0a5-12ce-4863-9c99-dedc7192bee6 Co-authored-by: mwiemer-microsoft <80539004+mwiemer-microsoft@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The changes are limited to a new test file and are straightforward, with only minor suggestions to improve failure diagnostics.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
|
@RikkiGibson requesting re-review :) |
JoeRobich
left a comment
JoeRobich
left a comment
There was a problem hiding this comment.
These tests look fine but consider whether this should be validation which runs once per enlistment when executing ./restore.{sh|ps1}
|
I like this a lot better, thanks for the feedback :) |
There was a problem hiding this comment.
🟡 Changes recommended
The validation cache marker is not platform-specific, which can incorrectly skip validation in mixed-OS workflows (e.g., Windows + WSL) and needs adjustment.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes restore-time build infrastructure in a way that can block restores across environments (especially CI/dev-machine variability), so it warrants final human validation.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The restore-time cache file write can race across projects during restore, risking intermittent failures unless guarded against concurrent creation.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
eng/targets/DeveloperSettings.targets:30
- _ValidateDeveloperSettings can execute in multiple projects during a restore, and the target condition can be true concurrently before the cache file is created. That can lead to concurrent writes to the same cache file under artifacts. Adding an Exists guard on the file-creating tasks avoids file contention if another project/thread creates the cache between target start and these tasks.
eng/targets/DeveloperSettings.targets:7 - The header comment claims "for all platforms, file line endings match platform line endings", but the implementation only probes this single file (and the repo also has per-file EOL rules via .gitattributes, e.g. *.sh is forced LF). Updating the comment to describe the actual sentinel-style check avoids misleading future maintainers.
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
Checks for two critical settings on first
dotnet restore:Validated on local Windows machine: