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

fix auto crlf issue #4915

Merged
merged 5 commits into from
May 28, 2020
Merged

fix auto crlf issue #4915

merged 5 commits into from
May 28, 2020

Conversation

pushkine
Copy link
Contributor

No description provided.

@benmccann
Copy link
Member

Thanks for investigating running on Windows @pushkine. Can you add a bit of description to the PR?

The CI does run on Windows and all the tests pass on Windows, so I'm wondering why this is necessary.

I saw at one point in your earlier PR that you removed the git config --global core.autocrlf false line:

- run: git config --global core.autocrlf false

How does that tie in? If you do that while checking out locally does it make the tests pass on Windows for you? (not that I'm suggesting that would necessarily be the best long-term solution, but I'm just trying to understand the issue you're running into)

@pushkine
Copy link
Contributor Author

pushkine commented May 27, 2020

Basically hundreds of tests will fail on windows machines unless you clone the repository with the core.autocrlf false parameter or something, I know it's written about somewhere in a contributing FAQ/WIKI but I can't find it anymore
This pr removes \r from every test & from css hashing so that every platform produces the same output by default
I believe --global core.autocrlf false can be removed, I just don't know anything about that stuff so I didn't mess with it

@tivac
Copy link
Contributor

tivac commented May 27, 2020

Instructions are in the readme.

https://github.com/sveltejs/svelte/blob/master/README.md#L37-L40

@Conduitry
Copy link
Member

The note in the readme and the core.autocrlf=false in the CI job were both workarounds so we didn't have to deal with normalizing these line endings during tests. If these changes address those problems, that sounds great - but I think we should remove the special Windows handling in the GitHub actions config (to make sure this continues to work as intended) and remove the now-unneeded instructions from the readme.

@pushkine
Copy link
Contributor Author

Done, let me know if there's anything else

@antony antony self-requested a review May 28, 2020 16:26
@Conduitry Conduitry merged commit 0da70f4 into sveltejs:master May 28, 2020
taylorzane pushed a commit to taylorzane/svelte that referenced this pull request Dec 17, 2020
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.

5 participants