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

Check out dumps with LF line endings, matches Dumper #1953

Open
wants to merge 1 commit into
base: release
Choose a base branch
from

Conversation

jan-varecka-signageos-io
Copy link
Contributor

Fix for tests using dumps on Windows with Git set to use core.autocrlf=true. Tests are expecting LF, but the files after checkout are with CRLF.

This fix sets that the dumps will be checked out with LF.

@icbaker icbaker self-assigned this Dec 4, 2024
@icbaker icbaker self-requested a review December 4, 2024 09:49
@icbaker
Copy link
Collaborator

icbaker commented Dec 4, 2024

Can you provide a bit more detail on why you need to have core.autocrlf=true in your local git settings for this repo? What about using an IDE/editor that can just deal with LF endings, and avoid introducing CRLF anywhere.

@jan-varecka-signageos-io
Copy link
Contributor Author

I have core.autocrlf=true as my global git settings, because it is the default behavior of git installer on Windows.
Git installer EOF default

I am using multiple editors so it is comfortable to not take care of how they support line endings. Actually, until this issue I never need to think about it at all :-)

@icbaker
Copy link
Collaborator

icbaker commented Dec 4, 2024

I think my stance is that if you decide to munge the line endings of a project, you should take responsibility for handling the cases where that doesn't work - so this config should live alongside your autocrlf setting, rather than in our repo (where it isn't needed, since we don't set autocrlf).

@consp1racy
Copy link

consp1racy commented Dec 4, 2024

This is removing an obstacle for anyone contributing from Windows. Media3 generates LF line endings in a specific place, so it should make best effort to keep those on the consuming side without making assumptions about contributors' global git config. Mind you you're fighting against the world default.

I've contributed more aggressive fixes before, this is fine.

@icbaker
Copy link
Collaborator

icbaker commented Dec 4, 2024

Media3 generates LF line endings in a specific place, so it should make best effort to keep those on the consuming side without making assumptions about contributors' global git config.

The content of these dump files are sometimes generated on the dev machine (for robolectric tests) and sometimes on an Android device (for instrumentation tests). So even if we generated the content with 'host specific' line endings, it would still generate only LF on the instrumentation tests - hence assuming LF everywhere.

This project uses LF everywhere. The (reasonable imo) assumption is that contributors stick to that consistently. If they decide to do that by converting to CRLF on their side only (with autocrlf), and back again to LF when committing/uploading, then it's up to them to handle the cases (like this) where that conversion causes issues.

@consp1racy
Copy link

So even if we generated the content with 'host specific' line endings

Nobody wants that. Produce LF, consume LF. Best effort to keep LF in transit.

This project uses LF everywhere.

Would you like to set autocrlf = false for the entire project to make it explicit? That would address the issue, too.

It doesn't play a role anywhere else. apart from CMD interpreting *.bat which requires CRLF. javac doesn't care. kotlinc doesn't care. It specifically does play a role when it comes to testing these dumps. So it has a scope other than global.

If they decide to do that by converting to CRLF on their side only

It's the world default on Windows, that's a fact, not a conscious choice for the majority, that's my assumption.


It's a free win in terms of usability, benefits contributors, doesn't harm maintainers. That's it. Happy Holidays.

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.

3 participants