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

docs: better bootstrapping xp #11124

Merged
merged 5 commits into from
Oct 31, 2024

Conversation

earloc
Copy link
Contributor

@earloc earloc commented Oct 30, 2024

Summary of the changes

  • optimize docs for first-time pull-experience

Fixes:
Some initial confusion when starting to work with the repo

@earloc earloc requested a review from a team as a code owner October 30, 2024 21:29
@earloc
Copy link
Contributor Author

earloc commented Oct 30, 2024

@davidwengier , started to get a hang on this repo and found some smaller areas to improve the docs.

@earloc
Copy link
Contributor Author

earloc commented Oct 30, 2024

@dotnet-policy-service agree

Copy link
Contributor

@ryzngard ryzngard left a comment

Choose a reason for hiding this comment

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

Thank you for the cleanup! Going to wait on approval based on long path recommendation vs using substr

docs/contributing/BuildFromSource.md Outdated Show resolved Hide resolved
@@ -63,6 +69,12 @@ Before opening the `Razor.sln` file in Visual Studio or VS Code, you need to per
of Razor to be deployed. This can be useful if the latest Razor bits depend on a breaking change in
Roslyn that isn't available in the version of Visual Studio being targeted. If you encounter errors
when debugging the Razor bits that you've built and deployed, setting this switch _might_ fix them.
> :bulb: Windows tip: if you encounter errors pointing to `path-too-long` errors, like `The fully qualified file name must be less than 260 characters.`, consider shortening your local path with `subst`, where `R` is a free drive-letter, e.g.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if instead we should add a reg file like roslyn has https://github.com/dotnet/roslyn/blob/main/eng/enable-long-paths.reg

Copy link
Contributor Author

@earloc earloc Oct 30, 2024

Choose a reason for hiding this comment

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

yeah, this variant just got me starting to be actually be able to work with this repo.
Might as well not "fix" the issue, if pathes got any longer - but did the trick in the meantime (as only two files exceded the path-length for me).

Relying on a registry setting might put some additional burden on potential contributors, even if this "seems" to be safely applicable on modern-ish versions of the OS, though. But then, having someone need to apply an (unknown) registry-key import might not benefit overall acceptancy when starting to contribute...so maybe we could just leave a hint to the link you provided? Maybe the roslyn-project has some notes on that in their docs, which we could refer to...

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems logical to me that if we tell people to enable long paths in git, we should tell them to do it for Windows too. Roslyn also has this to detect and warn on build, which we should crib:
https://github.com/dotnet/roslyn/blob/9e862f15fc4d57a479ac45724748a41c87a7d0b6/eng/targets/Imports.targets#L164-L170

BUT I don't see any reason that any of that should hold up a docs update

Copy link
Contributor Author

@earloc earloc Oct 30, 2024

Choose a reason for hiding this comment

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

As far as I understand, roslyn "needs" long-paths to be enabled at build time:
https://github.com/dotnet/roslyn/blob/697989601cb5319af38ca7c36d8727875c4f49c4/eng/targets/Imports.targets#L164C2-L170C13

I´ve got the feeling that this warning might get easily overlooked...but seems more straight forward / error-proof than relying on s.th. like subst.

I'll revisit this part, stay tuned...

Copy link
Contributor

Choose a reason for hiding this comment

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

Roslyn recommends it and checks in their builds https://github.com/dotnet/roslyn/blob/9e862f15fc4d57a479ac45724748a41c87a7d0b6/eng/targets/Imports.targets#L169

@jaredpar have you heard of any potential burden for contributors?

I'm fine with recommending both as an option since it's to unblock people in a way that they're comfortable with. Alternative to a reg file we could add the powershell equivalent

New-ItemProperty -Path "HKLM:\SYSTEM\CurrentControlSet\Control\FileSystem" -Name "LongPathsEnabled" -Value "1" -PropertyType DWORD -Force 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "burden" is just anticipated, as I personally faced it while trying to contribute ;)
I`ll think about how this could be integrated in a way, which would ease up the first-time experience when working with the repo, cool?

Applying suggestion from code-review

Co-authored-by: Andrew Hall <ryzngard@live.com>
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.

Thank you for the contribution <3

@earloc earloc closed this Oct 30, 2024
@earloc earloc reopened this Oct 30, 2024
@davidwengier
Copy link
Contributor

@earloc no idea why CI is failing, but you can leave it with us to this merged

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.

4 participants