Sed compatibility problems

[danpovey]

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:

sed -i.bak 's/#1$//g' $lout/lexicon.txt
rm $lout/lexicon.txt.bak 

... 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.

cd egs/wsj/s5
find ../.. -maxdepth 6 -name '*.sh' -exec grep 'sed -i' {} \; -print 2>/dev/null
     sed -ir "s!$p1!$p2!" $dir/segments
../../ami/s5/local/ami_ihm_scoring_data_prep.sh
     sed -ir "s:$p1:$p2:" $tmpdir/segments
../../ami/s5/local/ami_mdm_scoring_data_prep.sh
     sed -ir "s:$p1:$p2:" $tmpdir/segments
../../ami/s5/local/ami_sdm_scoring_data_prep.sh
     sed -ir "s!$p1!$p2!" $dir/segments
../../ami/s5b/local/ami_ihm_scoring_data_prep.sh
     sed -ir "s:$p1:$p2:" $tmpdir/segments
../../ami/s5b/local/ami_mdm_scoring_data_prep.sh
     sed -ir "s:$p1:$p2:" $tmpdir/segments
../../ami/s5b/local/ami_sdm_scoring_data_prep.sh
  sed -i "s/_non_normalized//g" $file
../../aspire/s5/local/multi_condition/prepare_impulses_noises.sh
sed -i'' 's/#1$//g' $lout/lexicon.txt
sed -i'' 's/#1$//g' $lout/lexiconp.txt
../../babel/s5c/local/syllab/generate_syllable_lang.sh
sed -i'' 's/#1$//g' $lout/lexicon.txt
sed -i'' 's/#1$//g' $lout/lexiconp.txt
../../babel/s5d/local/syllab/generate_phone_lang.sh
sed -i'' 's/#1$//g' $lout/lexicon.txt
sed -i'' 's/#1$//g' $lout/lexiconp.txt
../../babel/s5d/local/syllab/generate_syllable_lang.sh
sed -i "/\[$w\]/d" $tmpdir/lexicon.3
../../callhome_egyptian/s5/local/callhome_prepare_dict.sh
#sed -i "s:\s.*_fsp-([AB]): \1:g" data/dev/stm
#ls exp/tri5a/decode_dev/score_*/dev.ctm | xargs -I {} sed -i -r 's:fsp\s1\s:fsp A :g' {}
#ls exp/tri5a/decode_dev/score_*/dev.ctm | xargs -I {} sed -i -r 's:fsp\s2\s:fsp B :g' {}
../../callhome_egyptian/s5/local/ctm.sh
#sed -i "s:\s.*_fsp-([AB]): \1:g" data/dev/stm
#ls exp/tri5a/decode_dev/score_*/dev.ctm | xargs -I {} sed -i -r 's:fsp\s1\s:fsp A :g' {}
#ls exp/tri5a/decode_dev/score_*/dev.ctm | xargs -I {} sed -i -r 's:fsp\s2\s:fsp B :g' {}
../../fisher_callhome_spanish/s5/local/ctm.sh
    sed -i "/\[$w\]/d" $tmpdir/lexicon.2
../../fisher_callhome_spanish/s5/local/fsp_prepare_dict.sh
sed -i '1i<UNK> SIL' $dir/lexicon.txt # insert word <UNK> with phone sil at the begining of the dictionary
../../gale_arabic/s5b/local/gale_prep_grapheme_dict.sh
 sed -i '1i<UNK> SIL' $dir/lexicon.txt
../../gale_arabic/s5/local/gale_prep_dict.sh

[langep]

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.

[danpovey]

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> .

[johnjosephmorgan]

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"

[danpovey]

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> .

[johnjosephmorgan]

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>.

[danpovey]

@desh2608 do you think you might have time to work on this issues? You can ask vimal for help.

[desh2608]

I understand that all uses of sed -i -r are to be replaced with perl -pi -e, as per @langep 's comment above. I'll start with making this edit.

[danpovey]

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).

[stale]

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.

[stale]

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.