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

Force LF line endings for Windows #12266

Merged
merged 2 commits into from
Feb 12, 2018

Conversation

kellerza
Copy link
Member

@kellerza kellerza commented Feb 9, 2018

Description:

Closer to a working Docker dev environment on Windows... Ensure no CRLFs in the scripts & force LF on most files

Related issue (if applicable): fixes #

Checklist:

  • The code change is tested and works locally.

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass

@balloob
Copy link
Member

balloob commented Feb 9, 2018

Long time no see Johann! 🎉

.gitattributes Outdated
# Ensure "git config --global core.autocrlf input" before you clone
setup_docker_prereqs text eol=lf
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we just force this for all files instead of pick and choose?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is indeedwhat github recommends, but then you should EXCLUDE all binary files

Grepping through our repo, it seems that this should work

*     text eol=lf
*.py  whitespace=error

*.ico binary
*.jpg binary
*.png binary
*.zip binary

Copy link
Member

Choose a reason for hiding this comment

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

do we even have ico, jpg, png or zip files in our repo?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed

Docs/source/static: favicon & png
Tests/resources: zip
Components/camera: jpg

Copy link
Member

Choose a reason for hiding this comment

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

Let's follow the GitHub recommendations including * text=auto

Copy link
Member

Choose a reason for hiding this comment

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

So could we do * text=lf ?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it must be * text eol=lf according to git-scm

It controls different parts:

  • *: Applied to all files, hence the need to remove it on binary files, like images&zip later
  • text: Ensure LF in the repo
  • eol=lf: Attribute to ensure LF in the working directory (This should fix Windows)

Copy link
Member

Choose a reason for hiding this comment

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

So confusing. Okay, so I guess this PR is fine then?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just pushed the version at the top of our discussion, so read to go now

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed confusing, hopefully we can change it once and just forget about it again.... it only really troubles Windows users

.gitattributes Outdated
/virtualization/Docker/scripts/* text eol=lf
/script/* text eol=lf
*.sh text eol=lf
Copy link
Member

Choose a reason for hiding this comment

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

There are no shell or Python scripts in the root? or does this also match **/*.sh ?

Copy link
Member Author

Choose a reason for hiding this comment

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

*.py matches all **/*.sh:

λ git check-attr --all homeassistant\scripts\db_migrator.py
"homeassistant\\scripts\\db_migrator.py": text: set
"homeassistant\\scripts\\db_migrator.py": eol: lf
"homeassistant\\scripts\\db_migrator.py": whitespace: error

@kellerza
Copy link
Member Author

Quite a long time yes... my son started crawling, walking & climbing things so he takes up a lot more of my free time 😄

@balloob balloob merged commit f28fa74 into home-assistant:dev Feb 12, 2018
@balloob
Copy link
Member

balloob commented Feb 12, 2018

Okay so we missed mp3 for homeassistant/components/tts/demo.mp3. We also didn't apply the current settings to everything in the repo.

@balloob balloob mentioned this pull request Feb 12, 2018
@kellerza kellerza deleted the windows-line-endings branch February 12, 2018 07:38
@balloob balloob mentioned this pull request Feb 22, 2018
@home-assistant home-assistant locked and limited conversation to collaborators May 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants