[src] changes to make kaldi can be installed on cygwin#2304
[src] changes to make kaldi can be installed on cygwin#2304danpovey merged 13 commits intokaldi-asr:masterfrom
Conversation
src/base/kaldi-utils.cc
Outdated
| #if defined(_MSC_VER) || defined(MINGW) | ||
| ::Sleep(static_cast<int>(seconds * 1000.0)); | ||
| #elif defined(__CYGWIN__) | ||
| kaldi::Sleep(static_cast<int>(seconds * 1000.0)); |
There was a problem hiding this comment.
this makes me think you haven't run the tests ("make test"). It would just recurse and crash when it exceeds the max stack depth.
Try doing "man usleep" or "man -k sleep" in cygwin, assuming man pages are installed, to see what sleep functions are available and what you have to include to find them.
src/util/kaldi-io.cc
Outdated
|
|
||
| #ifdef __CYGWIN__ | ||
| static pclose(FILE* file) { | ||
| //FILE *file = (FILE*)_file; |
There was a problem hiding this comment.
I don't see the purpose of this function -- seems almost like a recursive call or somethign
y.
There was a problem hiding this comment.
Sorry, this change(about 'popen' and 'pclose') is a mistake. The original code was right. I find that what I need to change is in the /usr/include/stdio.h rather than in kaldi.
|
@hainan-xv Can you review this? |
src/base/get_version.sh
Outdated
| # Overwrite ./version.h with the temporary file if they are different. | ||
| if ! cmp -s $temp version.h; then | ||
| cp $temp version.h | ||
| sed -i 's/\r//g' version.h |
There was a problem hiding this comment.
try to avoid sed -i (or sed altogether)
There was a problem hiding this comment.
actually, try to figure out where the linefeed character comes from originally (does 'echo' add it?) and I'd like to know what problems it causes.
There was a problem hiding this comment.
but there would be '\r' in the 'version.h' file since it is on windows. should I leave it to users to delete '\r' themselves?
There was a problem hiding this comment.
kaldi itself does not have '\r'. this character is automatically generated when I download kaldi to a windows machine. '\r' in windows means 'end of line' like '\n' in Unix and the Unix environment(cygwin) cannot recognize it.
There was a problem hiding this comment.
Actually, I think the logic of your change is wrong, your code will cause
the if ! cmd test always succeed, so you will always replace the file --
you should modify the $temp file instead
|
LGTM (besides the sed issue)
BTW, there was (used to be) some compatibility feature to make the
commands actiing like file (ie. those "command|command2|") specifiers work
correctly within CYGWIN, make sure you didn't break those by
adding/changing the ifdefs -- I think that relates to kaldi-io file.
Also, did you run all tests successfully (`make test`)?
y
…On Fri, Mar 30, 2018 at 4:00 PM, Kun Qian ***@***.***> wrote:
@hainan-xv <https://github.com/hainan-xv> Can you review this?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2304 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AKisX-NnVUuWY-CG_MTWjgCq8ztaT-H6ks5tjo7cgaJpZM4S4HvE>
.
|
|
Yes, testing should include running a setup like "yesno" on cygwin.
…On Fri, Mar 30, 2018 at 4:39 PM, jtrmal ***@***.***> wrote:
LGTM (besides the sed issue)
BTW, there was (used to be) some compatibility feature to make the
commands actiing like file (ie. those "command|command2|") specifiers work
correctly within CYGWIN, make sure you didn't break those by
adding/changing the ifdefs -- I think that relates to kaldi-io file.
Also, did you run all tests successfully (`make test`)?
y
On Fri, Mar 30, 2018 at 4:00 PM, Kun Qian ***@***.***>
wrote:
> @hainan-xv <https://github.com/hainan-xv> Can you review this?
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <#2304 (comment)>,
or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AKisX-NnVUuWY-CG_
MTWjgCq8ztaT-H6ks5tjo7cgaJpZM4S4HvE>
> .
>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2304 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ADJVu_SH9AITfhUi_P0P6ETVvxnNGx6kks5tjpgFgaJpZM4S4HvE>
.
|
|
Actually, I think the logic of your change is wrong, your code will cause
the if ! cmd code always succeed, so you will always replace the file --
you should modify the $temp file instead
y.
…On Fri, Mar 30, 2018 at 4:42 PM, Kun Qian ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/base/get_version.sh
<#2304 (comment)>:
> @@ -89,6 +89,7 @@ fi
# Overwrite ./version.h with the temporary file if they are different.
if ! cmp -s $temp version.h; then
cp $temp version.h
+ sed -i 's/\r//g' version.h
but there would be '\r' in the 'version.h' file since it is on windows.
should I leave it to users to delete '\r' themselves?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2304 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AKisX2bleM_OZCvX8p7i-ZijCYSqZehmks5tjpjNgaJpZM4S4HvE>
.
|
|
does gcc/g++ mind the \r character there?
y.
…On Fri, Mar 30, 2018 at 4:54 PM, Jan Trmal ***@***.***> wrote:
Actually, I think the logic of your change is wrong, your code will cause
the if ! cmd code always succeed, so you will always replace the file --
you should modify the $temp file instead
y.
On Fri, Mar 30, 2018 at 4:42 PM, Kun Qian ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In src/base/get_version.sh
> <#2304 (comment)>:
>
> > @@ -89,6 +89,7 @@ fi
> # Overwrite ./version.h with the temporary file if they are different.
> if ! cmp -s $temp version.h; then
> cp $temp version.h
> + sed -i 's/\r//g' version.h
>
> but there would be '\r' in the 'version.h' file since it is on windows.
> should I leave it to users to delete '\r' themselves?
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <#2304 (comment)>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AKisX2bleM_OZCvX8p7i-ZijCYSqZehmks5tjpjNgaJpZM4S4HvE>
> .
>
|
|
No, gcc/g++ seems ok with \r
Sent from Mail for Windows 10
From: jtrmal
Sent: Friday, March 30, 2018 4:56 PM
To: kaldi-asr/kaldi
Cc: Kun Qian; Author
Subject: Re: [kaldi-asr/kaldi] [src] changes to make kaldi can be installed oncygwin (#2304)
does gcc/g++ mind the \r character there?
y.
On Fri, Mar 30, 2018 at 4:54 PM, Jan Trmal ***@***.***> wrote:
Actually, I think the logic of your change is wrong, your code will cause
the if ! cmd code always succeed, so you will always replace the file --
you should modify the $temp file instead
y.
On Fri, Mar 30, 2018 at 4:42 PM, Kun Qian ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In src/base/get_version.sh
> <#2304 (comment)>:
>
> > @@ -89,6 +89,7 @@ fi
> # Overwrite ./version.h with the temporary file if they are different.
> if ! cmp -s $temp version.h; then
> cp $temp version.h
> + sed -i 's/\r//g' version.h
>
> but there would be '\r' in the 'version.h' file since it is on windows.
> should I leave it to users to delete '\r' themselves?
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <#2304 (comment)>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AKisX2bleM_OZCvX8p7i-ZijCYSqZehmks5tjpjNgaJpZM4S4HvE>
> .
>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
OK, sorry, in that case I don't understand if the edit is needed at all? I
might be missing somehting, tho.
y.
…On Fri, Mar 30, 2018 at 4:57 PM, Kun Qian ***@***.***> wrote:
No, gcc/g++ seems ok with \r
Sent from Mail for Windows 10
From: jtrmal
Sent: Friday, March 30, 2018 4:56 PM
To: kaldi-asr/kaldi
Cc: Kun Qian; Author
Subject: Re: [kaldi-asr/kaldi] [src] changes to make kaldi can be
installed oncygwin (#2304)
does gcc/g++ mind the \r character there?
y.
On Fri, Mar 30, 2018 at 4:54 PM, Jan Trmal ***@***.***> wrote:
> Actually, I think the logic of your change is wrong, your code will cause
> the if ! cmd code always succeed, so you will always replace the file --
> you should modify the $temp file instead
> y.
>
>
> On Fri, Mar 30, 2018 at 4:42 PM, Kun Qian ***@***.***>
> wrote:
>
>> ***@***.**** commented on this pull request.
>> ------------------------------
>>
>> In src/base/get_version.sh
>> <#2304 (comment)>:
>>
>> > @@ -89,6 +89,7 @@ fi
>> # Overwrite ./version.h with the temporary file if they are different.
>> if ! cmp -s $temp version.h; then
>> cp $temp version.h
>> + sed -i 's/\r//g' version.h
>>
>> but there would be '\r' in the 'version.h' file since it is on windows.
>> should I leave it to users to delete '\r' themselves?
>>
>> —
>> You are receiving this because you commented.
>> Reply to this email directly, view it on GitHub
>> <#2304 (comment)>,
or mute
>> the thread
>> <https://github.com/notifications/unsubscribe-
auth/AKisX2bleM_OZCvX8p7i-ZijCYSqZehmks5tjpjNgaJpZM4S4HvE>
>> .
>>
>
>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2304 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AKisX4j-lWfRftTZGUIvPdyvPlGRSimwks5tjpxPgaJpZM4S4HvE>
.
|
|
Yes, I have run the ‘make test’ and there is no error.
Kun
From: jtrmal
Sent: Friday, March 30, 2018 4:39 PM
To: kaldi-asr/kaldi
Cc: Kun Qian; Author
Subject: Re: [kaldi-asr/kaldi] [src] changes to make kaldi can be installed oncygwin (#2304)
LGTM (besides the sed issue)
BTW, there was (used to be) some compatibility feature to make the
commands actiing like file (ie. those "command|command2|") specifiers work
correctly within CYGWIN, make sure you didn't break those by
adding/changing the ifdefs -- I think that relates to kaldi-io file.
Also, did you run all tests successfully (`make test`)?
y
On Fri, Mar 30, 2018 at 4:00 PM, Kun Qian ***@***.***> wrote:
@hainan-xv <https://github.com/hainan-xv> Can you review this?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2304 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AKisX-NnVUuWY-CG_MTWjgCq8ztaT-H6ks5tjo7cgaJpZM4S4HvE>
.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
src/fstext/lattice-weight-test.cc
Outdated
| } else if (tmp == 1) { | ||
| return LatticeWeight( 1, 2); // sometimes return special values.. | ||
| } else if (tmp == 2) { | ||
| return LatticeWeight( 2, 1); // this tests more thoroughly certain properties... |
There was a problem hiding this comment.
you're not supposed to use \tab in the code; use 2 spaces instead.
|
Yes, please read the Google Style Guide for C++ |
|
Thanks! This guide seems useful!
Best,
Kun
From: jtrmal
Sent: Monday, April 2, 2018 3:25 PM
To: kaldi-asr/kaldi
Cc: Kun Qian; Author
Subject: Re: [kaldi-asr/kaldi] [src] changes to make kaldi can be installed oncygwin (#2304)
Yes, please read the Google Style Guide for C++
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
danpovey
left a comment
There was a problem hiding this comment.
A couple of comments. please check with (e.g.) Hainan if you're unsure what I mean or how to do something.
src/base/kaldi-utils.cc
Outdated
| #if defined(_MSC_VER) || defined(MINGW) | ||
| ::Sleep(static_cast<int>(seconds * 1000.0)); | ||
| #elif defined(__CYGWIN__) | ||
| sleep(static_cast<int>(seconds * 1.0)); |
There was a problem hiding this comment.
the * 1.0 here is unnecessary.
src/base/kaldi-utils.h
Outdated
| # define KALDI_MEMALIGN_FREE(x) _aligned_free(x) | ||
| #elif defined(__CYGWIN__) | ||
| # define KALDI_MEMALIGN(align, size, pp_orig) \ | ||
| (*(pp_orig) = aligned_alloc(align, size)) |
There was a problem hiding this comment.
Actually I now see that we don't need this macro at all any more, as aligned_alloc is part of the C++11 standard. So please remove the macro and replace all instances of
foo = KALDI_MEMALIGN(x, y, &z)
with
foo = aligned_alloc(x, y);
and remove the temporary variables that previously bound to z, and replace KALDI_MEMALIGN_FREE with free (there are instances of this in the cudamatrix/ directory as well, that should also be replaced).
|
OK I would change it.
By the way, the test of ‘yesno’ on both Cygwin and Linux are all finished with no error.
Best,
Kun
… On Apr 4, 2018, at 18:52, Daniel Povey ***@***.***> wrote:
@danpovey commented on this pull request.
A couple of comments. please check with (e.g.) Hainan if you're unsure what I mean or how to do something.
In src/base/kaldi-utils.cc:
> @@ -45,6 +45,8 @@ std::string CharToString(const char &c) {
void Sleep(float seconds) {
#if defined(_MSC_VER) || defined(MINGW)
::Sleep(static_cast<int>(seconds * 1000.0));
+#elif defined(__CYGWIN__)
+ sleep(static_cast<int>(seconds * 1.0));
the * 1.0 here is unnecessary.
In src/base/kaldi-utils.h:
> # define KALDI_MEMALIGN(align, size, pp_orig) \
(*(pp_orig) = _aligned_malloc(size, align))
# define KALDI_MEMALIGN_FREE(x) _aligned_free(x)
+#elif defined(__CYGWIN__)
+# define KALDI_MEMALIGN(align, size, pp_orig) \
+ (*(pp_orig) = aligned_alloc(align, size))
Actually I now see that we don't need this macro at all any more, as aligned_alloc is part of the C++11 standard. So please remove the macro and replace all instances of
foo = KALDI_MEMALIGN(x, y, &z)
with
foo = aligned_alloc(x, y);
and remove the temporary variables that previously bound to z, and replace KALDI_MEMALIGN_FREE with free (there are instances of this in the cudamatrix/ directory as well, that should also be replaced).
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
and do they have comparable scores?
y.
…On Wed, Apr 4, 2018 at 7:59 PM, Kun Qian ***@***.***> wrote:
OK I would change it.
By the way, the test of ‘yesno’ on both Cygwin and Linux are all finished
with no error.
Best,
Kun
> On Apr 4, 2018, at 18:52, Daniel Povey ***@***.***> wrote:
>
> @danpovey commented on this pull request.
>
> A couple of comments. please check with (e.g.) Hainan if you're unsure
what I mean or how to do something.
>
> In src/base/kaldi-utils.cc:
>
> > @@ -45,6 +45,8 @@ std::string CharToString(const char &c) {
> void Sleep(float seconds) {
> #if defined(_MSC_VER) || defined(MINGW)
> ::Sleep(static_cast<int>(seconds * 1000.0));
> +#elif defined(__CYGWIN__)
> + sleep(static_cast<int>(seconds * 1.0));
> the * 1.0 here is unnecessary.
>
> In src/base/kaldi-utils.h:
>
> > # define KALDI_MEMALIGN(align, size, pp_orig) \
> (*(pp_orig) = _aligned_malloc(size, align))
> # define KALDI_MEMALIGN_FREE(x) _aligned_free(x)
> +#elif defined(__CYGWIN__)
> +# define KALDI_MEMALIGN(align, size, pp_orig) \
> + (*(pp_orig) = aligned_alloc(align, size))
> Actually I now see that we don't need this macro at all any more, as
aligned_alloc is part of the C++11 standard. So please remove the macro and
replace all instances of
> foo = KALDI_MEMALIGN(x, y, &z)
> with
> foo = aligned_alloc(x, y);
> and remove the temporary variables that previously bound to z, and
replace KALDI_MEMALIGN_FREE with free (there are instances of this in the
cudamatrix/ directory as well, that should also be replaced).
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub, or mute the thread.
>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2304 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AKisXzxf77lNDaZvK693IsmrF6JzTenrks5tlV5WgaJpZM4S4HvE>
.
|
|
I remember it is 0% WER.
From: jtrmal
Sent: Wednesday, April 4, 2018 8:16 PM
To: kaldi-asr/kaldi
Cc: Kun Qian; Author
Subject: Re: [kaldi-asr/kaldi] [src] changes to make kaldi can be installed oncygwin (#2304)
and do they have comparable scores?
y.
On Wed, Apr 4, 2018 at 7:59 PM, Kun Qian ***@***.***> wrote:
OK I would change it.
By the way, the test of ‘yesno’ on both Cygwin and Linux are all finished
with no error.
Best,
Kun
> On Apr 4, 2018, at 18:52, Daniel Povey ***@***.***> wrote:
>
> @danpovey commented on this pull request.
>
> A couple of comments. please check with (e.g.) Hainan if you're unsure
what I mean or how to do something.
>
> In src/base/kaldi-utils.cc:
>
> > @@ -45,6 +45,8 @@ std::string CharToString(const char &c) {
> void Sleep(float seconds) {
> #if defined(_MSC_VER) || defined(MINGW)
> ::Sleep(static_cast<int>(seconds * 1000.0));
> +#elif defined(__CYGWIN__)
> + sleep(static_cast<int>(seconds * 1.0));
> the * 1.0 here is unnecessary.
>
> In src/base/kaldi-utils.h:
>
> > # define KALDI_MEMALIGN(align, size, pp_orig) \
> (*(pp_orig) = _aligned_malloc(size, align))
> # define KALDI_MEMALIGN_FREE(x) _aligned_free(x)
> +#elif defined(__CYGWIN__)
> +# define KALDI_MEMALIGN(align, size, pp_orig) \
> + (*(pp_orig) = aligned_alloc(align, size))
> Actually I now see that we don't need this macro at all any more, as
aligned_alloc is part of the C++11 standard. So please remove the macro and
replace all instances of
> foo = KALDI_MEMALIGN(x, y, &z)
> with
> foo = aligned_alloc(x, y);
> and remove the temporary variables that previously bound to z, and
replace KALDI_MEMALIGN_FREE with free (there are instances of this in the
cudamatrix/ directory as well, that should also be replaced).
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub, or mute the thread.
>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#2304 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AKisXzxf77lNDaZvK693IsmrF6JzTenrks5tlV5WgaJpZM4S4HvE>
.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
…fixes kaldi-asr#2347 This reverts commit 72c32fd.
mostly the job is to add "defined(CYGWIN)" when needed and choose correct function as compiled on cygwin.
One thing worth noticing is that there is a line of code should be changed in "/usr/include/ctype.h". (still the problem of add "defined(CYGWIN)"). But this seems a problem of cygwin itself. Maybe user need to add it manually.