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

Enforcing unix linebreaks for shell scripts #538

Merged
merged 17 commits into from Dec 26, 2021

Conversation

Verequus
Copy link
Contributor

Shell scripts do not work unless they have unix-style linebreaks. This enforces this checkout behavior regardless of he environment. Any environment that already uses unix-style linebreaks is not affected by this change.

Verequus and others added 8 commits December 10, 2021 18:25
Co-authored-by: Michael Förderer <michael.foerderer@gmx.de>
* Replacing duplicated code with singular function
* Removing incorrect code
Co-authored-by: Lukas Atkinson <opensource@LukasAtkinson.de>
Copy link
Member

@Spacetown Spacetown left a comment

Choose a reason for hiding this comment

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

I prefer keeping lineendings as they are except for the Git internal files like .gitignore and .gitattributes. With this configuration it depends on the user how the file is saved.

.gitattributes Outdated Show resolved Hide resolved
@latk
Copy link
Member

latk commented Dec 17, 2021

As already discussed in chat (starting around https://gitter.im/gcovr/gcovr?at=61a638b98853d31640226423), I'm not the biggest fan of overriding native file endings. By default, Git checks out text files with native file endings. On Windows, you'll get CRLF. On Linux/Unix/MacOS, you'll get LF. When Bash is installed on Windows, it will understand CRLF endings. See in particular: the tests do currently pass on native Windows in the CI environment.

The problem that led to this PR seemed to be that the repository was checked out on Windows, but then directly mounted into a WSL (Linux) environment. Due to the Windows checkout, the text files received CRLF endings, which the Linux tools couldn't handle. The solution isn't to force Unix-specific line endings. Instead:

  1. check out the repository on the same system where the tests should be run instead of moving the files between systems, and
  2. be wary of /mtn/ paths in a WSL environment.

So my preference would be to close this as wontfix.

On the other hand, WSL is becoming a very popular development environment. If this makes setup easier for other people who might also make the mistake of running the tests from within WSL on files that were checked out on Windows, then this could still be a reasonable change. Thoughts on this?

Looking for line ending problems also showed that some CSV files in our test reference data were committed with CRLF endings. But since we explicitly scrub CRLF endings before comparing the files, this doesn't affect the tests.

@Verequus
Copy link
Contributor Author

I experienced the same issue with a cygwin installation in my project, so I'm not sure which bash works on Windows with CRLF instead of LF.

@latk
Copy link
Member

latk commented Dec 17, 2021

I experienced the same issue with a cygwin installation in my project

That is a very good point that suggests that your proposed change is reasonable. Do we only have to fix .sh shell scripts, or also line endings for Makefiles then?

@Verequus
Copy link
Contributor Author

I only had to change shell scripts when trying to run make qa so this suggests that makefiles work also with CRLF.

@Spacetown
Copy link
Member

What about removing the normalisation in the tests? Should we also configure the line endiings and remove the normalisation? An exception should be the checksums because they will differ if a project use Unix and Windows for testing.

@Spacetown Spacetown added this to the 5.1 milestone Dec 26, 2021
@Spacetown
Copy link
Member

Please can you update the changeling and the contributors list?

@Verequus
Copy link
Contributor Author

Please can you update the changeling and the contributors list?

Shall I commit .gitattributes as following then or as originally proposed?

# Declare files that will always have LF line endings on checkout.
*.sh text eol=lf

# Convert line endings to platform
.git* text=auto

# Keep line endings as checked in
* -text

@Spacetown
Copy link
Member

After the discussion I prefer to use the original one and extend the comment with a hint to cygwin and WSL.

@Verequus
Copy link
Contributor Author

Pushed an update, is that what you wanted?

CHANGELOG.rst Outdated Show resolved Hide resolved
.gitattributes Outdated Show resolved Hide resolved
Verequus and others added 2 commits December 26, 2021 18:59
Co-authored-by: Michael Förderer <michael.foerderer@gmx.de>
Co-authored-by: Michael Förderer <michael.foerderer@gmx.de>
@Verequus
Copy link
Contributor Author

Not sure why the deploy fails.

@Spacetown
Copy link
Member

Approval of admin was missing.

@Verequus
Copy link
Contributor Author

Still fails. :(

@latk
Copy link
Member

latk commented Dec 26, 2021

It seems one of the merges went wrong and parts of the #535 pull request have appeared. To recover, the easiest way might be to:

  • fetch the latest master branch from this repository
  • start a new branch from master
  • copy the desired changes to this new branch (gitattributes, changelog, authors)
  • create a new commit and force-push to the linebreaks branch in your GitHub repository

Alternatively, an interactive rebase onto our master branch would work. That way, undesired commits can be removed.

@Spacetown
Copy link
Member

You merged your master branch instead of doing a rebase. On your master branch you did changes for #535.
You should always use a dedicated branch.

@Verequus
Copy link
Contributor Author

I did use a dedicated branch, but I messed up updating it somehow. That github client is less obvious than I'd like to. In any case, it should be hopefully now clean.

@Spacetown
Copy link
Member

I was talking about the other PR which was done on your master.

@codecov
Copy link

codecov bot commented Dec 26, 2021

Codecov Report

Merging #538 (101f669) into master (447c878) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #538   +/-   ##
=======================================
  Coverage   96.07%   96.07%           
=======================================
  Files          21       21           
  Lines        2855     2855           
  Branches      532      532           
=======================================
  Hits         2743     2743           
  Misses         49       49           
  Partials       63       63           
Flag Coverage Δ
ubuntu-18.04 95.12% <ø> (ø)
windows-2019 95.90% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 447c878...101f669. Read the comment docs.

Copy link
Member

@Spacetown Spacetown left a comment

Choose a reason for hiding this comment

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

LGFM

@Spacetown Spacetown merged commit 5fb2acd into gcovr:master Dec 26, 2021
@Verequus Verequus deleted the linebreaks branch December 26, 2021 21:14
@Spacetown Spacetown mentioned this pull request Dec 29, 2021
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.

None yet

3 participants