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

Move LongPath check to MSBuild target #68217

Merged
merged 1 commit into from
May 30, 2023
Merged

Move LongPath check to MSBuild target #68217

merged 1 commit into from
May 30, 2023

Conversation

jaredpar
Copy link
Member

The Roslyn repository effectively depends on having long paths enabled at this point. Between NuGet and the VSSDK, both outside our control, our repository runs right up against MAX_PATH limits even with a very short user name. A user name of any significant length will quickly run into issues in our repo.

The repository already warns about long path support, and has for some time, but it does so in command line builds. That was a fine experience when it was added as long path support was new and was only needed in a few cases. Now though it's effectivtely needed for everyone so need to give it more visibility by elevating it to an MSBuild warning.

This warning is deliberately not configurable. Having long paths disabled at this point means your inner loop dev experience is broken in strange, hard to diagnose ways.

The Roslyn repository effectively depends on having long paths enabled
at this point. Between NuGet and the VSSDK, both outside our control,
our repository runs right up against `MAX_PATH` limits even with a very
short user name. A user name of any significant length will quickly run
into issues in our repo.

The repository already warns about long path support, and has for some
time, but it does so in command line builds. That was a fine experience
when it was added as long path support was new and was only needed in a
few cases. Now though it's effectivtely needed for everyone so need to
give it more visibility by elevating it to an MSBuild warning.

This warning is _deliberately_ not configurable. Having long paths
disabled at this point means your inner loop dev experience is broken in
strange, hard to diagnose ways.
@jaredpar jaredpar requested a review from a team as a code owner May 16, 2023 22:00
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Issues and PRs which have not yet been triaged by a lead labels May 16, 2023
@@ -154,6 +154,14 @@
Condition="$(_VersionComparisonResult) < 0"/>
</Target>

<Target Name="_CheckLongPathSupport" BeforeTargets="BeforeBuild" Condition="'$(MSBuildRuntimeType)' == 'Full'">
Copy link
Member

Choose a reason for hiding this comment

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

💭 Should this be an OS check instead?

<_RoslynLongPathsEnabled>$([MSBuild]::GetRegistryValueFromView('HKEY_LOCAL_MACHINE\System\CurrentControlSet\Control\FileSystem', 'LongPathsEnabled', null, RegistryView.Registry64, RegistryView.Registry32))</_RoslynLongPathsEnabled>
</PropertyGroup>

<Warning Condition="'$(_RoslynLongPathsEnabled)' != '1'" Text="Long paths are required for this project. Please run eng\enable-long-paths.reg" />
Copy link
Member

Choose a reason for hiding this comment

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

💡 Consider assigning Code here as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you elaborate on that? Not quite sure what that means.

Copy link
Member

Choose a reason for hiding this comment

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

something like

<Warning Code="JARED001" Condition="'$(_RoslynLongPathsEnabled)' != '1'" Text="Long paths are required for this project. Please run eng\enable-long-paths.reg" />

personally I don't see particular value unless you're going to write help content for it or expect people to suppress it

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I specifically don't want this warning to be suppressible.

@carlossanlop
Copy link
Member

carlossanlop commented May 26, 2023

For my own education - if this is merged, what would the experience now be for those of us who reported issues with longpath? Would the error change and become more meaningful?

dotnet/roslyn-analyzers#6632
dotnet/msbuild#8123

@sharwell
Copy link
Member

@carlossanlop This won't automatically apply to other repositories, but it may help establish a standard practice for dealing with this situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants