Skip to content

Force LF line endings for Windows#12266

Merged
balloob merged 2 commits intohome-assistant:devfrom
kellerza:windows-line-endings
Feb 12, 2018
Merged

Force LF line endings for Windows#12266
balloob merged 2 commits intohome-assistant:devfrom
kellerza:windows-line-endings

Conversation

@kellerza
Copy link
Copy Markdown
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
Copy Markdown
Member

balloob commented Feb 9, 2018

Long time no see Johann! 🎉

Comment thread .gitattributes Outdated
setup_docker_prereqs eol=lf
/virtualization/Docker/scripts/* eol=lf No newline at end of file
# Ensure "git config --global core.autocrlf input" before you clone
setup_docker_prereqs text eol=lf
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Comment thread .gitattributes Outdated
setup_docker_prereqs text eol=lf
/virtualization/Docker/scripts/* text eol=lf
/script/* text eol=lf
*.sh text eol=lf
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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