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

Windows line endings #444

Merged
merged 3 commits into from
Jan 13, 2016
Merged

Windows line endings #444

merged 3 commits into from
Jan 13, 2016

Conversation

Timmmm
Copy link
Contributor

@Timmmm Timmmm commented Jan 11, 2016

There's an issue with github if you try to download the repository as a zip (rather than cloning it with git), then it runs git archive on the repository.

Unfortunately they run in on a Linux machine, and git changes all the line endings in text files to their native format (LF on Linux). Patch requires CRLF endings on Windows so it fails.

The change to .gitattributes stops git mangling any patch files. The change to the .patch file makes the line endings CRLF. Other files may require changing to CRLF.

There's no really nice way to fix this in general, since when git archive is run it doesn't know if the target is Windows or Linux, and Windows patch always requires CRLF, whereas Linux patch requires the patch file to match whatever it is patching. I'd suggest keeping separate patch files for Windows and Linux (which there is already in this case fortunately).

Tim Hutt added 3 commits January 11, 2016 11:07
Patch on windows requires CRLF. This should leave them unchanged.
Previous version would only look in the root dir which obviously doesn't work.
The previous commits stop git mangling the file endings, but the file endings in the repository were LF, where they should have been CRLF.
@jtrmal
Copy link
Contributor

jtrmal commented Jan 11, 2016

Timmmm, first of thanks for the PR.
@kkm000 , can you please verify the suggested changes? The priority should be preventing the linux checkout from breaking -- if there would be some issue, I'd rather keep windows checkouts/archives broken.

@jtrmal jtrmal mentioned this pull request Jan 11, 2016
@danpovey
Copy link
Contributor

I really don't like the idea of changing to CRLF endings in any of the patch files to keep Windows happy. Remember 95% or more of use of Kaldi is on UNIX. I'd prefer to modify the Windows scripts, e.g. to call unix2dos on those files (if that program exists on Windows).

@Timmmm
Copy link
Contributor Author

Timmmm commented Jan 12, 2016

Apparently there is a way to convert line endings on the Windows command line, but it seems to me that it would be nice to avoid adding an extra step to the installation process.

Fortunately in this case the patch I changed (openfstwin-1.3.4.patch) is Windows-specific so it should never be used on Unix. All the other patches should still have Unix line endings.

@danpovey
Copy link
Contributor

Oh, you're right. I didn't notice that there was 'win' in the filename.
Yenda, if you don't see any problems with this you can merge it.
Dan

On Tue, Jan 12, 2016 at 12:25 AM, Tim notifications@github.com wrote:

Apparently there is a way to convert line endings
http://stackoverflow.com/a/27844521/265521 on the Windows command line,
but it seems to me that it would be nice to avoid adding an extra step to
the installation process.

Fortunately in this case the patch I changed (openfstwin-1.3.4.patch) is
Windows-specific so it should never be used on Unix. All the other patches
should still have Unix line endings.


Reply to this email directly or view it on GitHub
#444 (comment).

@jtrmal
Copy link
Contributor

jtrmal commented Jan 13, 2016

OK, merging.

jtrmal added a commit that referenced this pull request Jan 13, 2016
@jtrmal jtrmal merged commit 1017986 into kaldi-asr:master Jan 13, 2016
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