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

Conver CR/LF line endings to LF style #2784

Closed
wants to merge 1 commit into from

Conversation

ottok
Copy link
Contributor

@ottok ottok commented Feb 3, 2020

Done with command:
find . -type f -print0 | xargs -0 dos2unix

Closes: #2782

Discovered while packaging MariaDB 10.4 for Debian.

Done with command:
  find . -type f -print0 | xargs -0 dos2unix


Closes: wolfSSL#2782
@wolfSSL-Bot
Copy link

Can one of the admins verify this patch?

@kaleb-himes
Copy link
Contributor

Please see comment regarding contributions from non employees of wolfSSL on #2785.

@kaleb-himes kaleb-himes self-assigned this Feb 3, 2020
@kaleb-himes
Copy link
Contributor

kaleb-himes commented Feb 4, 2020

Hi @ottok,

Thank you again for the submission. Initial feedback on these changes:

Similar to the problem being resolved these global changes are going to introduce the same issue on other systems. The example provided was:

modified:   extra/wolfssl/wolfssl/IDE/CRYPTOCELL/main.c
modified:   extra/wolfssl/wolfssl/IDE/CSBENCH/user_settings.h
modified:   extra/wolfssl/wolfssl/IDE/Renesas/e2studio/Projects/test/src/key_data.c
modified:   extra/wolfssl/wolfssl/IDE/Renesas/e2studio/Projects/test/src/key_data.h
modified:   extra/wolfssl/wolfssl/IDE/VS-AZURE-SPHERE/client/client.h
modified:   extra/wolfssl/wolfssl/IDE/VS-AZURE-SPHERE/server/server.h
modified:   extra/wolfssl/wolfssl/mqx/util_lib/Sources/util.h
modified:   extra/wolfssl/wolfssl/mqx/wolfcrypt_benchmark/Sources/main.c
modified:   extra/wolfssl/wolfssl/mqx/wolfcrypt_benchmark/Sources/main.h
modified:   extra/wolfssl/wolfssl/wolfssl/wolfcrypt/port/ti/ti-ccm.h

Similarly with these change now on a windows system we are going to see the below every time we open one of the visual studio solutions because windows will automatically convert these linux line endings to windows CR/LF.

modified: examples/client/client.vcxproj
modified: examples/echoclient/echoclient.vcxproj
modified: examples/echoserver/echoserver.vcxproj
modified: examples/server/server.vcxproj
modified: sslSniffer/sslSniffer.vcxproj
modified: testsuite/testsuite.vcxproj
modified: wolfssl.vcxproj
modified: wolfssl64.sln

We are not opposed to accepting changes to line endings that are not auto-generated files. Many of the IDE-generated files were captured by this change and similarly those would be reverted back any time they are opened by the respective IDE resulting in the same issue as is currently trying to be solved.

I'm not sure there is a true "global" solution to this problem. It is not uncommon for our devs to have to adjust editor settings to prevent automatically changing line feed format when manually opening a .c source file or .h header file but the IDE's can not be stopped from updating the line feeds when loading the project or solution files because they need those line feeds to parse the file.

  • KH

@ottok
Copy link
Contributor Author

ottok commented Feb 8, 2020

I have this in my .git/attributes/info as an intermediate solution:

mariadb-10.4$ cat .git/info/attributes 
extra/wolfssl/** binary

Maybe your developers should review their git settings to not commit text files in wrong format to version control? Ideally the editor would handle it, but the second line of defense would be git settings.

This PR should be accepted to fix the initial situation, and then editors or git settings updated to not re-introduce this issue.

Copy link
Contributor

@kaleb-himes kaleb-himes left a comment

Choose a reason for hiding this comment

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

.c source files, .h header files, Makefiles, READMEs', and include.am files we are open to accepting CR/LF changes in.

All others should be ignored for example (This is not an exhaustive list, subject to review)
.project
.vcxproj
.json
.sln
.edl
.filters
.vcproj
input
anything in the mplabx/ directory
.mem
.tcl
.xml
.jlink
.launch
aes_asm.asm (the windows assembly)
.rc
.config
.cs
.csproj
[ and more ]

Should not be included in this update.

@kaleb-himes
Copy link
Contributor

@ottok,

I'll go over your suggestion with the team in tomorrows meeting and update according to the teams collective feedback also.

Warm Regards,

K

@JacobBarthelmeh
Copy link
Contributor

Can you solve this using the .gitattributes file or setting a global config with git rather than changing 16k + lines of code? Having the local .gitattributes auto use the line endings that make sense for the file committing? In some cases the IDE being used for developing projects has the correct line ending as a Windows line ending. Which for the Windows users building, and also with some embedded IDE's, is not a problem. For them Unix style line endings are a problem. In other cases the line ending is the Unix style one and we use this one a lot for the main source code. I think is best if we can have some .gitattribute file change that can keep both our Windows users happy and Unix users.

@kaleb-himes
Copy link
Contributor

Thank you Jacob and wolfSSL team for going over this with me yesterday.

We found a possible solution here: https://www.rtuin.nl/2013/02/how-to-make-git-ignore-different-line-endings/

This would involve adding a single file .gitattributes to the repo and a single line to that file * -crlf as opposed to changing 16k lines. This would also not in effect demand a change to our coding standard for all developers (something only our CTO could make the call on). Would that proposal be amenable to you @ottok?

  • K

@ottok
Copy link
Contributor Author

ottok commented Feb 14, 2020 via email

@kaleb-himes
Copy link
Contributor

@ottok,

Yes that could also work! Would you like us to pursue that as a solution and open an alternate PR from our side or would you want to make that change to this PR?

  • KH

@ottok
Copy link
Contributor Author

ottok commented Feb 14, 2020 via email

@kaleb-himes
Copy link
Contributor

@ottok,

Sounds good! If we get to it before we see an update here we will notify you and close this PR. We don't expect you to commit to a schedule at all, thank you for your efforts on this so far.

Cheers,

K

@ottok
Copy link
Contributor Author

ottok commented Jan 13, 2021

This was closed with unmerged commits. Sorry I forgot about this. I can still submit you the line ending fixes. I have on my TODO also fixing https://jira.mariadb.org/browse/MCOL-4266 and I would save effort by doing these both at the same time.

PS. Nice to see you hired Daniel the curl author!

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.

Clean away Windows carriage return characters from file
5 participants