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

Ensure all Visual Studio files are generated with CRLF #90495

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

shana
Copy link
Contributor

@shana shana commented Apr 10, 2024

Visual Studio has a semi-hard requirement that project files have CRLF line endings, and if they don't, VS will overwrite them/mark them as dirty, potentially causing issues like unexpected project reloads or potential loss of data. Ideally, we want to avoid things that cause VS to mark project files as dirty immediately after detecting a change to project files on disk, because that causes all kinds of nasty confusing reload/save dialog loops.

For example, if a user generates the VS project files as normal and opens the solution, VS will detect the wrong line endings and modify all project files in memory, silently marking them as dirty. There is no notification for this, only an asterisk in the status bar. If the user then does a Rebuild All command (which triggers the regeneration+build scons command), VS will detect that the project files have changed on disk (because scons regenerates them), while it still has a CRLF-modified version of them in memory. This triggers a confusing conflict save dialog where the user is asked if they to "overwrite" or "discard" or "revert" or "skip" (something to that effect).

If the user chooses the "correct" option, which is to load the new versions of the props files (your guess of which one of the options does that is as good as mine), VS will then reload everything, and then immediately mark them all dirty again, requiring another save. This "dirty" state isn't very obvious, so anytime the user does anything with the project files outside of VS, the save conflict will happen again, rinse and repeat. If the user chooses the "wrong" option - which isn't hard, since they aren't expecting a conflict - VS will overwrite the scons changes (which might result in the project becoming out of sync with scons).

PR #89333 erroneously enforced \n for these files, which up until then were being written with the same line endings as the templates (which for the most cases would be \r\n. This PR sets those templates to always be commited with \r\n, and fixes the writes calls during generation to use \r\n.

I added changes to all the templates, just to make sure they get normalized in the repo - VS generates sln files with tab indentation, so changes the sln template to that, and added a comment footer to the other templates so we can use it later for cache detection.

/cc @akien-mga @Repiteo @mhilbrunner

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

Yeah, this is what Visual Studio does so it's best to be consistent with that (unless we can convince Microsoft to see the light and switch to LF). I'll approve but I have not tested.

VS generates sln files with 4 tab indentation

The indentation is tabs, not 4 tabs. Tab size is not encoded in the file.

@shana
Copy link
Contributor Author

shana commented Apr 10, 2024

VS generates sln files with 4 tab indentation

The indentation is tabs, not 4 tabs. Tab size is not encoded in the file.

Yeah that's what I meant, bleh. I'm in the middle of a merge conflict resolution that I've been working through for the past checks clock 4 hours (which is how I caught this line ending change), so I think my brain is getting slightly frazzled :P

@akien-mga akien-mga modified the milestones: 4.x, 4.3 Apr 10, 2024
@akien-mga akien-mga merged commit a8bd519 into godotengine:master Apr 10, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants