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

CR-LF in the codebase #942

Closed
neheb opened this issue Jul 11, 2019 · 12 comments
Closed

CR-LF in the codebase #942

neheb opened this issue Jul 11, 2019 · 12 comments

Comments

@neheb
Copy link
Contributor

neheb commented Jul 11, 2019

Is there a good reason for using CRLF for the source? I ask since CRLF can cause issues with git.

@metalefty
Copy link
Contributor

+1 to LF, I prefer LF.

@chipitsine
Copy link
Member

I was sure .gitattributes keep us safe. Any example of how it could go wrong?

@neheb
Copy link
Contributor Author

neheb commented Jul 11, 2019

For OpenWrt, we have separate patch files that get applied to the main package.

Using git diff > .patch works with git config core.autocrlf true

When it comes time to commit to the repository though, core.autocrlf must be enabled as well. Otherwise git will change the line endings to LF. No amount of configure changes and git commit --amend will help after that.

@chipitsine
Copy link
Member

that option does not help ?

https://github.com/SoftEtherVPN/SoftEtherVPN/blob/master/.gitattributes

btw, what are those patches ? can you provide a link ?

@chipitsine
Copy link
Member

@neheb
Copy link
Contributor Author

neheb commented Jul 11, 2019

No. https://github.com/neheb/packages/tree/soft5/net/softethervpn5/patches

Last two

edit: note that a slightly different solution was merged.

@chipitsine
Copy link
Member

so, I have several questions

  1. the last two looks like they might be included in upstream, @davidebeatrici ?
  2. if you are talking about CR-LF, you mean only limited number of files ? do you mind to open a PR ?
  3. we definetly will appreciate some help to setup openwrt build in our travis-ci matrix (or maybe openwrt uses some other kind of CI)

@neheb
Copy link
Contributor Author

neheb commented Jul 11, 2019

Well, I'm only asking why CRLF is used in the codebase at all. My guess is for Windows development. I would guess that with Windows 10, Visual Studio supports LF just fine, given that Notepad does.

Travis used to be used. But it had hard to debug build failures (was using an old Ubuntu LTS). OpenWrt uses CircleCI. The configuration file is in the root directory.

Since I got this to build, I noticed it does not compile with OpenSSL lacking deprecated APIs. I might whip up a patch to fix.

edit: Also note that while the first two patches are correct, the last two have mangled end-line characters and thus do not apply. Locally, this is incorrect but they got mangled during git commit.

@Andy2244
Copy link
Contributor

Andy2244 commented Aug 23, 2019

A year ago or so this issue was also noted by me and while the .gitattributes was changed, the conclusion was that "someone" would have to recommit the whole project with LF endings, which never happened.

So yes while you create diffs/patches, the result has mixed endings. This works fine unless some form of auto conversion happens either via texteditor or on commit. So you need to take extra care not to modify the patches.

PS: The first two patches we use are to somewhat mitigate #575.

@neheb
Copy link
Contributor Author

neheb commented Aug 23, 2019

I figured out that vi does not mess with the line endings. Everything else does.

@chipitsine
Copy link
Member

if you feel better about line ending conversion, please provide such patch

@neheb
Copy link
Contributor Author

neheb commented Apr 25, 2020

Meh this is a minor point. It's only really relevant to the OpenWrt workflow where GNU patch and git differ. Closing.

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

No branches or pull requests

4 participants