Tegra build fixes #3171
Merged
Tegra build fixes #3171
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
… --cuda-arch overrides to have multiple versions
danpovey reviewed Mar 26, 2019
| } | ||
| | ||
| function read_value { | ||
| local val=`expr "X$1" : '[^=]*=\(.*\)'`; |
Contributor
There was a problem hiding this comment.
Can you explain the purpose of this change please? I wish this read_value function were documented. This expr thing is very obscure.
Contributor Author
There was a problem hiding this comment.
Looking closely at this it appears the difference is $* would allow a multi-word value.
For example:
x=read_value foo bar
In the previous code this would set x to "foo".
In the new code this would set x to "foo bar".
Why is this important?
We use this in read_dirname. Imagine the case where your directory has a space:
CUDATKDIR=read_dirname $1
Imagine the CUDATKDIR sat at "/usr/local/cuda 10.1"
The previous code would set CUDATKDIR to /usr/local/cuda
Contributor Author
| @cliffwoolley these were changes made by cliff. Adding him so he can comment as to why the change was needed. |
Contributor
| CUDATKDIR=read_dirname "$1" ? …On Tue, Mar 26, 2019 at 15:30 Justin Luitjens ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/configure <#3171 (comment)>: > @@ -118,7 +118,7 @@ function rel2abs { } function read_value { - local val=`expr "X$1" : '[^=]*=\(.*\)'`; Looking closely at this it appears the difference is $* would allow a multi-word value. For example: x=read_value foo bar In the previous code this would set x to "foo". In the new code this would set x to "foo bar". Why is this important? We use this in read_dirname. Imagine the case where your directory has a space: CUDATKDIR=read_dirname $1 Imagine the CUDATKDIR sat at "/usr/local/cuda 10.1" The previous code would set CUDATKDIR to /usr/local/cuda — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3171 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKisX7-u4xQZ_2oD2QXAyhF23Kl-dXTuks5vanH9gaJpZM4cMEH0> . |
Contributor Author
That would probably work too. Not sure it is any better that way as we have lots of places that have $1 without quotes. |
Contributor
| The quotes would be safer in case it was, for example, two spaces. On Tue, Mar 26, 2019 at 3:39 PM Justin Luitjens <notifications@github.com> wrote: … CUDATKDIR=read_dirname "$1" ? … <#m_9065514906246889576_> On Tue, Mar 26, 2019 at 15:30 Justin Luitjens ***@***.***> wrote: ***@***.*** commented on this pull request. ------------------------------ In src/configure <#3171 (comment) <#3171 (comment)>>: > @@ -118,7 +118,7 @@ function rel2abs { } function read_value { - local val=expr "X$1" : '[^=]*=\(.*\)'; Looking closely at this it appears the difference is $* would allow a multi-word value. For example: x=read_value foo bar In the previous code this would set x to "foo". In the new code this would set x to "foo bar". Why is this important? We use this in read_dirname. Imagine the case where your directory has a space: CUDATKDIR=read_dirname $1 Imagine the CUDATKDIR sat at "/usr/local/cuda 10.1" The previous code would set CUDATKDIR to /usr/local/cuda — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#3171 (comment) <#3171 (comment)>>, or mute the thread https://github.com/notifications/unsubscribe-auth/AKisX7-u4xQZ_2oD2QXAyhF23Kl-dXTuks5vanH9gaJpZM4cMEH0 . That would probably work too. Not sure it is any better that way as we have lots of places that have $1 without quotes. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3171 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADJVuwyAuDyLCRDwTJ57j7rAb73i4Ikfks5vandlgaJpZM4cMEH0> . |
Contributor Author
| $* says use all parameters so 2 spaces would also work. $* is a bashism which says all parameters. Where as $1 says just the first parameter. On Tue, Mar 26, 2019 at 1:58 PM Daniel Povey <notifications@github.com> wrote: … The quotes would be safer in case it was, for example, two spaces. On Tue, Mar 26, 2019 at 3:39 PM Justin Luitjens ***@***.***> wrote: > CUDATKDIR=read_dirname "$1" ? > … <#m_9065514906246889576_> > On Tue, Mar 26, 2019 at 15:30 Justin Luitjens ***@***.***> wrote: ***@***.*** > commented on this pull request. ------------------------------ In > src/configure <#3171 (comment) > <#3171 (comment)>>: > @@ > -118,7 +118,7 @@ function rel2abs { } function read_value { - local > val=expr "X$1" : '[^=]*=\(.*\)'; Looking closely at this it appears the > difference is $* would allow a multi-word value. For example: > x=read_value foo bar In the previous code this would set x to "foo". In the > new code this would set x to "foo bar". Why is this important? We use this > in read_dirname. Imagine the case where your directory has a space: > CUDATKDIR=read_dirname $1 Imagine the CUDATKDIR sat at "/usr/local/cuda > 10.1" The previous code would set CUDATKDIR to /usr/local/cuda — You are > receiving this because you are subscribed to this thread. Reply to this > email directly, view it on GitHub <#3171 (comment) > <#3171 (comment)>>, or > mute the thread > https://github.com/notifications/unsubscribe-auth/AKisX7-u4xQZ_2oD2QXAyhF23Kl-dXTuks5vanH9gaJpZM4cMEH0 > . > > That would probably work too. Not sure it is any better that way as we > have lots of places that have $1 without quotes. > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#3171 (comment)>, or mute > the thread > < https://github.com/notifications/unsubscribe-auth/ADJVuwyAuDyLCRDwTJ57j7rAb73i4Ikfks5vandlgaJpZM4cMEH0 > > . > — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3171 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGRZcsT8crnj1LWfxXPuqAcvJ4OWYVA3ks5vanwAgaJpZM4cMEH0> . |
Contributor
| I meant two successive spaces, like "a b" On Tue, Mar 26, 2019 at 4:06 PM Justin Luitjens <notifications@github.com> wrote: … $* says use all parameters so 2 spaces would also work. $* is a bashism which says all parameters. Where as $1 says just the first parameter. On Tue, Mar 26, 2019 at 1:58 PM Daniel Povey ***@***.***> wrote: > The quotes would be safer in case it was, for example, two spaces. > > On Tue, Mar 26, 2019 at 3:39 PM Justin Luitjens < ***@***.***> > wrote: > > > CUDATKDIR=read_dirname "$1" ? > > … <#m_9065514906246889576_> > > On Tue, Mar 26, 2019 at 15:30 Justin Luitjens ***@***.***> wrote: ***@***.*** > > commented on this pull request. ------------------------------ In > > src/configure <#3171 (comment) > > <#3171 (comment)>>: > > @@ > > -118,7 +118,7 @@ function rel2abs { } function read_value { - local > > val=expr "X$1" : '[^=]*=\(.*\)'; Looking closely at this it appears the > > difference is $* would allow a multi-word value. For example: > > x=read_value foo bar In the previous code this would set x to "foo". In > the > > new code this would set x to "foo bar". Why is this important? We use > this > > in read_dirname. Imagine the case where your directory has a space: > > CUDATKDIR=read_dirname $1 Imagine the CUDATKDIR sat at "/usr/local/cuda > > 10.1" The previous code would set CUDATKDIR to /usr/local/cuda — You are > > receiving this because you are subscribed to this thread. Reply to this > > email directly, view it on GitHub <#3171 (comment) > > <#3171 (comment)>>, > or > > mute the thread > > > https://github.com/notifications/unsubscribe-auth/AKisX7-u4xQZ_2oD2QXAyhF23Kl-dXTuks5vanH9gaJpZM4cMEH0 > > . > > > > That would probably work too. Not sure it is any better that way as we > > have lots of places that have $1 without quotes. > > > > — > > You are receiving this because you commented. > > Reply to this email directly, view it on GitHub > > <#3171 (comment)>, > or mute > > the thread > > < > https://github.com/notifications/unsubscribe-auth/ADJVuwyAuDyLCRDwTJ57j7rAb73i4Ikfks5vandlgaJpZM4cMEH0 > > > > . > > > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#3171 (comment)>, or mute > the thread > < https://github.com/notifications/unsubscribe-auth/AGRZcsT8crnj1LWfxXPuqAcvJ4OWYVA3ks5vanwAgaJpZM4cMEH0 > > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3171 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADJVuyNfClwc9lWWE-36f9eNyv08IvK6ks5van24gaJpZM4cMEH0> . |
Contributor Author
| Let me write some quick tests to see what the output is. I assume we want everything after the equal sign to stay unchanged. If not let me know. On Tue, Mar 26, 2019 at 2:07 PM Daniel Povey <notifications@github.com> wrote: … I meant two successive spaces, like "a b" On Tue, Mar 26, 2019 at 4:06 PM Justin Luitjens ***@***.***> wrote: > $* says use all parameters so 2 spaces would also work. $* is a bashism > which says all parameters. Where as $1 says just the first parameter. > > On Tue, Mar 26, 2019 at 1:58 PM Daniel Povey ***@***.***> > wrote: > > > The quotes would be safer in case it was, for example, two spaces. > > > > On Tue, Mar 26, 2019 at 3:39 PM Justin Luitjens < > ***@***.***> > > wrote: > > > > > CUDATKDIR=read_dirname "$1" ? > > > … <#m_9065514906246889576_> > > > On Tue, Mar 26, 2019 at 15:30 Justin Luitjens ***@***.***> wrote: ***@***.*** > > > commented on this pull request. ------------------------------ In > > > src/configure <#3171 (comment) > > > <#3171 (comment) >>: > > > @@ > > > -118,7 +118,7 @@ function rel2abs { } function read_value { - local > > > val=expr "X$1" : '[^=]*=\(.*\)'; Looking closely at this it appears the > > > difference is $* would allow a multi-word value. For example: > > > x=read_value foo bar In the previous code this would set x to "foo". In > > the > > > new code this would set x to "foo bar". Why is this important? We use > > this > > > in read_dirname. Imagine the case where your directory has a space: > > > CUDATKDIR=read_dirname $1 Imagine the CUDATKDIR sat at "/usr/local/cuda > > > 10.1" The previous code would set CUDATKDIR to /usr/local/cuda — You > are > > > receiving this because you are subscribed to this thread. Reply to this > > > email directly, view it on GitHub <#3171 (comment) > > > <#3171 (comment) >>, > > or > > > mute the thread > > > > > > https://github.com/notifications/unsubscribe-auth/AKisX7-u4xQZ_2oD2QXAyhF23Kl-dXTuks5vanH9gaJpZM4cMEH0 > > > . > > > > > > That would probably work too. Not sure it is any better that way as we > > > have lots of places that have $1 without quotes. > > > > > > — > > > You are receiving this because you commented. > > > Reply to this email directly, view it on GitHub > > > <#3171 (comment) >, > > or mute > > > the thread > > > < > > > https://github.com/notifications/unsubscribe-auth/ADJVuwyAuDyLCRDwTJ57j7rAb73i4Ikfks5vandlgaJpZM4cMEH0 > > > > > > . > > > > > > > — > > You are receiving this because you authored the thread. > > Reply to this email directly, view it on GitHub > > <#3171 (comment)>, > or mute > > the thread > > < > https://github.com/notifications/unsubscribe-auth/AGRZcsT8crnj1LWfxXPuqAcvJ4OWYVA3ks5vanwAgaJpZM4cMEH0 > > > > . > > > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#3171 (comment)>, or mute > the thread > < https://github.com/notifications/unsubscribe-auth/ADJVuyNfClwc9lWWE-36f9eNyv08IvK6ks5van24gaJpZM4cMEH0 > > . > — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3171 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGRZcv38Kysp14C2AmECiwWb0W0tZMnlks5van4AgaJpZM4cMEH0> . |
Contributor Author
| %> cat test.sh #!/bin/bash function read_value_old { local val=`expr "X$1" : '[^=]*=\(.*\)'`; echo $val } function read_value_new { local val=`expr "X$*" : '[^=]*=\(.*\)'`; echo $val } X=`read_value_old $1` echo "X='$X'" X=`read_value_old "$1"` echo "X='$X'" X=`read_value_new $1` echo "X='$X'" %> ./test.sh a="b a" X='b' X='b a' X='b a' The issue of 2 spaces exists in both the original version with quotes and the new version. To get spaces to appear we have to quote various parts of the function and $1. function read_value_old { local val="`expr "X$1" : '[^=]*=\(.*\)'`"; echo "$val" } function read_value_new { local val="`expr "X$*" : '[^=]*=\(.*\)'`"; echo "$val" } Dan what behavior do you want? Should we preserve spacing? …On Tue, Mar 26, 2019 at 2:24 PM Justin Luitjens ***@***.***> wrote: Let me write some quick tests to see what the output is. I assume we want everything after the equal sign to stay unchanged. If not let me know. On Tue, Mar 26, 2019 at 2:07 PM Daniel Povey ***@***.***> wrote: > I meant two successive spaces, like "a b" > > > On Tue, Mar 26, 2019 at 4:06 PM Justin Luitjens ***@***.*** > > > wrote: > > > $* says use all parameters so 2 spaces would also work. $* is a bashism > > which says all parameters. Where as $1 says just the first parameter. > > > > On Tue, Mar 26, 2019 at 1:58 PM Daniel Povey ***@***.***> > > wrote: > > > > > The quotes would be safer in case it was, for example, two spaces. > > > > > > On Tue, Mar 26, 2019 at 3:39 PM Justin Luitjens < > > ***@***.***> > > > wrote: > > > > > > > CUDATKDIR=read_dirname "$1" ? > > > > … <#m_9065514906246889576_> > > > > On Tue, Mar 26, 2019 at 15:30 Justin Luitjens ***@***.***> wrote: > ***@***.*** > > > > commented on this pull request. ------------------------------ In > > > > src/configure <#3171 (comment) > > > > <#3171 (comment) > >>: > > > > @@ > > > > -118,7 +118,7 @@ function rel2abs { } function read_value { - local > > > > val=expr "X$1" : '[^=]*=\(.*\)'; Looking closely at this it appears > the > > > > difference is $* would allow a multi-word value. For example: > > > > x=read_value foo bar In the previous code this would set x to > "foo". In > > > the > > > > new code this would set x to "foo bar". Why is this important? We > use > > > this > > > > in read_dirname. Imagine the case where your directory has a space: > > > > CUDATKDIR=read_dirname $1 Imagine the CUDATKDIR sat at > "/usr/local/cuda > > > > 10.1" The previous code would set CUDATKDIR to /usr/local/cuda — You > > are > > > > receiving this because you are subscribed to this thread. Reply to > this > > > > email directly, view it on GitHub <#3171 (comment) > > > > <#3171 (comment) > >>, > > > or > > > > mute the thread > > > > > > > > > > https://github.com/notifications/unsubscribe-auth/AKisX7-u4xQZ_2oD2QXAyhF23Kl-dXTuks5vanH9gaJpZM4cMEH0 > > > > . > > > > > > > > That would probably work too. Not sure it is any better that way as > we > > > > have lots of places that have $1 without quotes. > > > > > > > > — > > > > You are receiving this because you commented. > > > > Reply to this email directly, view it on GitHub > > > > < > #3171 (comment)>, > > > or mute > > > > the thread > > > > < > > > > > > https://github.com/notifications/unsubscribe-auth/ADJVuwyAuDyLCRDwTJ57j7rAb73i4Ikfks5vandlgaJpZM4cMEH0 > > > > > > > > . > > > > > > > > > > — > > > You are receiving this because you authored the thread. > > > Reply to this email directly, view it on GitHub > > > <#3171 (comment) > >, > > or mute > > > the thread > > > < > > > https://github.com/notifications/unsubscribe-auth/AGRZcsT8crnj1LWfxXPuqAcvJ4OWYVA3ks5vanwAgaJpZM4cMEH0 > > > > > > . > > > > > > > — > > You are receiving this because you commented. > > Reply to this email directly, view it on GitHub > > <#3171 (comment)>, > or mute > > the thread > > < > https://github.com/notifications/unsubscribe-auth/ADJVuyNfClwc9lWWE-36f9eNyv08IvK6ks5van24gaJpZM4cMEH0 > > > > . > > > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#3171 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AGRZcv38Kysp14C2AmECiwWb0W0tZMnlks5van4AgaJpZM4cMEH0> > . > |
Contributor
| I guess your changes are OK then. If preserving those kind of spaces is a hassle, I don't think it's necessary. The kind of person who would put two successive spaces in a filename probably shouldn't be attempting to build Kaldi anyway. On Tue, Mar 26, 2019 at 4:36 PM Justin Luitjens <notifications@github.com> wrote: … %> cat test.sh #!/bin/bash function read_value_old { local val=`expr "X$1" : '[^=]*=\(.*\)'`; echo $val } function read_value_new { local val=`expr "X$*" : '[^=]*=\(.*\)'`; echo $val } X=`read_value_old $1` echo "X='$X'" X=`read_value_old "$1"` echo "X='$X'" X=`read_value_new $1` echo "X='$X'" %> ./test.sh a="b a" X='b' X='b a' X='b a' The issue of 2 spaces exists in both the original version with quotes and the new version. To get spaces to appear we have to quote various parts of the function and $1. function read_value_old { local val="`expr "X$1" : '[^=]*=\(.*\)'`"; echo "$val" } function read_value_new { local val="`expr "X$*" : '[^=]*=\(.*\)'`"; echo "$val" } Dan what behavior do you want? Should we preserve spacing? On Tue, Mar 26, 2019 at 2:24 PM Justin Luitjens ***@***.***> wrote: > Let me write some quick tests to see what the output is. I assume we want > everything after the equal sign to stay unchanged. If not let me know. > > On Tue, Mar 26, 2019 at 2:07 PM Daniel Povey ***@***.***> > wrote: > >> I meant two successive spaces, like "a b" >> >> >> On Tue, Mar 26, 2019 at 4:06 PM Justin Luitjens < ***@***.*** >> > >> wrote: >> >> > $* says use all parameters so 2 spaces would also work. $* is a bashism >> > which says all parameters. Where as $1 says just the first parameter. >> > >> > On Tue, Mar 26, 2019 at 1:58 PM Daniel Povey < ***@***.***> >> > wrote: >> > >> > > The quotes would be safer in case it was, for example, two spaces. >> > > >> > > On Tue, Mar 26, 2019 at 3:39 PM Justin Luitjens < >> > ***@***.***> >> > > wrote: >> > > >> > > > CUDATKDIR=read_dirname "$1" ? >> > > > … <#m_9065514906246889576_> >> > > > On Tue, Mar 26, 2019 at 15:30 Justin Luitjens ***@***.***> wrote: >> ***@***.*** >> > > > commented on this pull request. ------------------------------ In >> > > > src/configure <#3171 (comment) >> > > > < #3171 (comment) >> >>: >> > > > @@ >> > > > -118,7 +118,7 @@ function rel2abs { } function read_value { - local >> > > > val=expr "X$1" : '[^=]*=\(.*\)'; Looking closely at this it appears >> the >> > > > difference is $* would allow a multi-word value. For example: >> > > > x=read_value foo bar In the previous code this would set x to >> "foo". In >> > > the >> > > > new code this would set x to "foo bar". Why is this important? We >> use >> > > this >> > > > in read_dirname. Imagine the case where your directory has a space: >> > > > CUDATKDIR=read_dirname $1 Imagine the CUDATKDIR sat at >> "/usr/local/cuda >> > > > 10.1" The previous code would set CUDATKDIR to /usr/local/cuda — You >> > are >> > > > receiving this because you are subscribed to this thread. Reply to >> this >> > > > email directly, view it on GitHub <#3171 (comment) >> > > > < #3171 (comment) >> >>, >> > > or >> > > > mute the thread >> > > > >> > > >> > >> https://github.com/notifications/unsubscribe-auth/AKisX7-u4xQZ_2oD2QXAyhF23Kl-dXTuks5vanH9gaJpZM4cMEH0 >> > > > . >> > > > >> > > > That would probably work too. Not sure it is any better that way as >> we >> > > > have lots of places that have $1 without quotes. >> > > > >> > > > — >> > > > You are receiving this because you commented. >> > > > Reply to this email directly, view it on GitHub >> > > > < >> #3171 (comment)>, >> > > or mute >> > > > the thread >> > > > < >> > > >> > >> https://github.com/notifications/unsubscribe-auth/ADJVuwyAuDyLCRDwTJ57j7rAb73i4Ikfks5vandlgaJpZM4cMEH0 >> > > > >> > > > . >> > > > >> > > >> > > — >> > > You are receiving this because you authored the thread. >> > > Reply to this email directly, view it on GitHub >> > > < #3171 (comment) >> >, >> > or mute >> > > the thread >> > > < >> > >> https://github.com/notifications/unsubscribe-auth/AGRZcsT8crnj1LWfxXPuqAcvJ4OWYVA3ks5vanwAgaJpZM4cMEH0 >> > > >> > > . >> > > >> > >> > — >> > You are receiving this because you commented. >> > Reply to this email directly, view it on GitHub >> > <#3171 (comment) >, >> or mute >> > the thread >> > < >> https://github.com/notifications/unsubscribe-auth/ADJVuyNfClwc9lWWE-36f9eNyv08IvK6ks5van24gaJpZM4cMEH0 >> > >> > . >> > >> >> — >> You are receiving this because you authored the thread. >> Reply to this email directly, view it on GitHub >> <#3171 (comment)>, >> or mute the thread >> < https://github.com/notifications/unsubscribe-auth/AGRZcv38Kysp14C2AmECiwWb0W0tZMnlks5van4AgaJpZM4cMEH0 > >> . >> > — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3171 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADJVuzN-XuAzC10GV9S5vvBa0xUxyTedks5vaoS_gaJpZM4cMEH0> . |
danpovey pushed a commit to danpovey/kaldi that referenced this pull request Jun 19, 2019
…a; allow --cuda-arch overrides to have multiple versions (kaldi-asr#3171)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
Don't build for Tegra sm_XX versions on x86/ppc and vice versa; allow…
--cuda-arch overrides to have multiple versions