-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Sed compatibility problems #2225
Comments
We will still run into problems when using I think we can replace all
An alternative option to converting anything is to install gnu-sed during the initial installation of dependencies. This could be done with:
This would require the user to add |
I prefer the perl-based solution.
It's good that you seem to be familiar with these issues.
…On Thu, Mar 29, 2018 at 12:08 AM, Patrick L. Lange ***@***.*** > wrote:
We will still run into problems when using sed -r e.g. ls
exp/tri5a/decode_dev/score_*/dev.ctm | xargs -I {} sed -i -r
's:fsp\s1\s:fsp A :g' {}. BSD sed uses -E instead of -r.
I think we can replace all sed -i -r with perl -pi -e. This might be
slightly slower but probably does not matter in these scripts.
sed -i and sed -i'' would need to either use backups with additional
cleanup step or the regexes would need to be converted to extended regexes.
In the sed -ir case, someone with more experience should probably check
if -i -r was intended.
An alternative option to converting anything is to install gnu-sed during
the initial installation of dependencies. This could be done with:
brew install gnu-sed --with-default-names
brew link gnu-sed
This would require the user to add /usr/local/bin in front of their PATH
but most brew users probably have this already. Furthermore, this would
potentially break other scripts the user wrote for BSD sed if they are not
careful about setting up their PATH.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2225 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADJVu2GUlmQL7xLSXdcDMSBKdhtQNSzZks5tjF5LgaJpZM4SH256>
.
|
I found sed compatibility problems in the following file: |
Oh-- where it fail and why?
Any chance you could make a PR?
I know you have other changes to that recipe too-- now might be a good time
to make a PR for that too.
…On Tue, Jul 31, 2018 at 5:28 PM, John J Morgan ***@***.***> wrote:
I found sed compatibility problems in the following file:
egs/heroico/s5/local/prepare_data.sh
I suggest replacing:
sed -e "s/\r//"
with
tr -d "\r"
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2225 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADJVu7UbGCl2g7dmWc2ziyghb_MAHj39ks5uMPY9gaJpZM4SH256>
.
|
I am running the heroico recipe on Mac OSX BSD Unix.
The \r is not filtered out when writing the wav.scp, utt2spk and text lists.
This causes a utf8 encoding check to fail when feature extraction is started.
I'll work on a pr.
j
… On Jul 31, 2018, at 8:30 PM, Daniel Povey ***@***.***> wrote:
Oh-- where it fail and why?
Any chance you could make a PR?
I know you have other changes to that recipe too-- now might be a good time
to make a PR for that too.
On Tue, Jul 31, 2018 at 5:28 PM, John J Morgan ***@***.***>
wrote:
> I found sed compatibility problems in the following file:
> egs/heroico/s5/local/prepare_data.sh
> I suggest replacing:
> sed -e "s/\r//"
> with
> tr -d "\r"
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#2225 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ADJVu7UbGCl2g7dmWc2ziyghb_MAHj39ks5uMPY9gaJpZM4SH256>
> .
>
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#2225 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABK_gHvX_Yt-LKa-rIKk7R-vvRfTooJCks5uMPa2gaJpZM4SH256>.
|
@desh2608 do you think you might have time to work on this issues? You can ask vimal for help. |
I understand that all uses of |
OK, but don't take his word for it-- you'll need to figure out a way to test that it does the right thing (even if it doesn't involve fully running the examples). |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This issue has been automatically closed by a bot strictly because of inactivity. This does not mean that we think that this issue is not important! If you believe it has been closed hastily, add a comment to the issue and mention @kkm000, and I'll gladly reopen it. |
I hope someone can find the time to help with this.
This is a sed compatibility issue that affects Macs and other BSD. Commands like the following:
sed -i'' 's/#1$//g' $lout/lexicon.txt
wil fail on mac, due to the command being interpreted as an extension argument to the -i option. (The '' is not seen by sed). I have looked into this and there appears to be no way to get this to work in a compatible way without adding an extension. I suggest to do this as, for instance:
... or you can remove the backup file later in the script, or perhaps better, explicitly reference the original location so it's not in-place at all.
There are quite a few
sed -ir
commands. I don't know how those arose; 'r' is a strange backup extension and the letter 'r' is certainly not interpreted as an option (and anyway that option doesn't exist on mac). I think those r's should be changed to .bak and the backup removed if posible.I'd like to warn people that sed should be considered HIGHLY SUSPECT from a compatibility point of view, especially if you are using command-line switches. perl is preferable if possible.
See below for affected locations, but please run this yourself with larger maxdepth in case there are more.
The text was updated successfully, but these errors were encountered: