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

dx: warn about long paths disabled #11140

Merged
merged 6 commits into from
Dec 4, 2024

Conversation

earloc
Copy link
Contributor

@earloc earloc commented Nov 2, 2024

Summary of the changes

  • refactored Directory.Targets.props to centralize import of common targets
  • introducing check for enabled long path support on windows
  • added docs to guide contributors on how to enable long path support

Now, when building with build.cmd, the following Warningis generated:

{{PATH-TO-REPOSITORY}}\eng\targets\CheckLongPathSupport.targets(26,5): error LongPathsDisabled: Long paths support is required for this project. See 'docs/contributing/LongPaths.md' on how to overcome this. 

Opening VS will show the warning as well, with clickable link to docs/contributing/LongPaths.md :
image

fixes #11139

@earloc earloc requested review from a team as code owners November 2, 2024 13:14
@earloc
Copy link
Contributor Author

earloc commented Nov 2, 2024

@davidwengier, @ryzngard
Took me some time to recover some long forgotten msbuild wizardry, but finally got a proposal to integrate the check for long path support when working with this repo on a windows machine together.

As the original errors already happen during project load-time (f.e.: in VS, the affected project refuses to load), this was a bit tricky to achieve, but I feel this is unobtrusive enough to ease up the overall first-time experience when working with this repo.

Only "donwside" of the current approach is, that this check might be carried out for every project within the solution - which might put some additional time on top of overall build-time. This may be somehow refactored in a way, that the imported target only gets imported once, but I didn`t manage to achieve this so far.

Curious about your thoughts on this...

Copy link
Contributor

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

No issues from me.

@davidwengier davidwengier requested a review from jaredpar November 4, 2024 00:16
@DustinCampbell
Copy link
Member

I'd be very interested in @jaredpar taking a look to make sure we're following the best practices. I know that dotnet/roslyn does something specific for long paths.

@earloc
Copy link
Contributor Author

earloc commented Nov 4, 2024

@DustinCampbell fyi: this change basically adopts what roslyn does, but integrates it in a slightly different way. Mostly due to the fact that those nasty path-too-long-errors might pop up very early (at load-time). This is why I decided to utilize InitialTargets in favor of BeforeTargets="BeforeBuild" , as the error(s) prevented the build from reaching this target at all.

@DustinCampbell
Copy link
Member

@DustinCampbell fyi: this change basically adopts what roslyn does, but integrates it in a slightly different way. Mostly due to the fact that those nasty path-too-long-errors might pop up very early (at load-time). This is why I decided to utilize InitialTargets in favor of BeforeTargets="BeforeBuild" , as the error(s) prevented the build from reaching this target at all.

I totally understand @earloc! That's why I was hoping @jaredpar would take a look, since he manages so much of the infrastructure in Roslyn and Razor. 😄

@earloc
Copy link
Contributor Author

earloc commented Nov 15, 2024

@jaredpar you probably have a ton of other things on your plate. Would appreciate any feedback on this here if you find the time 👌.

Also, I'm wondering if this might be packaged into a standalone nuget, which projects would be able to just pull in 🤔.

eng/targets/CheckLongPathSupport.targets Outdated Show resolved Hide resolved
eng/targets/Common.targets Show resolved Hide resolved
docs/contributing/BuildFromSource.md Outdated Show resolved Hide resolved
docs/contributing/LongPaths.md Outdated Show resolved Hide resolved
@ryzngard ryzngard merged commit 14238b2 into dotnet:main Dec 4, 2024
12 checks passed
@ryzngard
Copy link
Contributor

ryzngard commented Dec 4, 2024

Thanks for the contribution @earloc !

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.

dx: better guide potential contributors on dealing with 'path-too-long'-errors on windows
6 participants