| Summary: | ASTERISK-15293: [patch] Portability tweaks to contrib/scripts/safe_asterisk | ||
| Reporter: | Ben Klang (bklang) | Labels: | |
| Date Opened: | 2009-12-08 17:03:46.000-0600 | Date Closed: | 2010-03-25 11:20:36 |
| Priority: | Minor | Regression? | No |
| Status: | Closed/Complete | Components: | Core/Portability |
| Versions: | Frequency of Occurrence | ||
| Related Issues: | |||
| Environment: | Attachments: | ( 0) 20100106__issue16416__trunk.diff.txt ( 1) safe_asterisk-compat.patch ( 2) safe_asterisk-compat-1.patch | |
| Description: | The attached patch addresses several minor issues that have to do with improving the script's operation on non-Linux platforms: 1) Use mail instead of mailx (the latter supports specifying the subject with -s) 2) Change several test cases to work with old versions of test 3) Change the invocation of the date(1) command to work with non-GNU versions 4) Fix some minor bugs (such as the way id(1) is called and the order of shell output redirects) 5) Improve the error message to be more helpful when/if Asterisk crashes. There may be other portability issues, but this script has been tested on OSX, Linux and OpenSolaris and works on those platforms. | ||
| Comments: | By: Ben Klang (bklang) 2009-12-15 11:31:10.000-0600 Looks like I typoed the date format string. The day-of-month and minute are reversed. I have attached an updated patch. By: Tilghman Lesher (tilghman) 2010-01-06 17:11:10.000-0600 I have modified your patch quite a bit, because you've made some errors in terms of making the script more cross-platform-aware. 1) You changed defaults, which is not permitted, especially in the middle of a release cycle. 2) You changed from using the test utility to using the '[' shell builtin. This is less portable, not more portable. 3) Quotes around strings in test conditions allows values that contain special characters to avoid being interpreted by the shell, thus the quotes are needed for better portability. By: Ben Klang (bklang) 2010-01-06 17:45:33.000-0600 Thank you very much for making those additional portability changes. I only made the changes necessary to get the script to work on my system while (hopefully) not breaking existing systems. Regarding #1 in your comments, I assume you are referring to the change to KILLALLMPG123 switch? I can understand not wanting to commit that to old or current versions of Asterisk (especially to be respective of the release cycle), but is MPG123 even still in use for music on hold? I thought that system was deprecated around the time of Asterisk 1.0. To me the sane(er) default going forward is try not to kill any processes we did not start. I will test your patch on my systems as soon as possible and confirm the changes. By: Tilghman Lesher (tilghman) 2010-01-06 17:52:28.000-0600 People are still free to use the mpg123 utility for their MOH streams, whether we recommend its use or not. By: Tzafrir Cohen (tzafrir) 2010-01-07 10:48:04.000-0600 Could you please state the specific platform your fixes improve portability for? For instance, '[' is part of standard posix sh (well, may implemented by an external '[' binary, still part of posix). By: Tilghman Lesher (tilghman) 2010-01-11 16:16:03.000-0600 tzafrir: autoconf uses the 'test' binary in preference to '['. I assume there's a reason for that, even if it's non-POSIX shells that that's designed to work for. By: Digium Subversion (svnbot) 2010-01-11 21:18:37.000-0600 Repository: asterisk Revision: 239307 U branches/1.4/contrib/scripts/safe_asterisk ------------------------------------------------------------------------ r239307 | tilghman | 2010-01-11 21:18:37 -0600 (Mon, 11 Jan 2010) | 8 lines Portability and other fixes for the safe_asterisk script (closes issue ASTERISK-15293) Reported by: bklang Patches: safe_asterisk-compat-1.patch uploaded by bklang (license 919) 20100106__issue16416__trunk.diff.txt uploaded by tilghman (license 14) Tested by: bklang ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=239307 By: Digium Subversion (svnbot) 2010-01-11 21:21:41.000-0600 Repository: asterisk Revision: 239308 _U trunk/ U trunk/contrib/scripts/safe_asterisk ------------------------------------------------------------------------ r239308 | tilghman | 2010-01-11 21:21:41 -0600 (Mon, 11 Jan 2010) | 15 lines Merged revisions 239307 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r239307 | tilghman | 2010-01-11 21:18:36 -0600 (Mon, 11 Jan 2010) | 8 lines Portability and other fixes for the safe_asterisk script (closes issue ASTERISK-15293) Reported by: bklang Patches: safe_asterisk-compat-1.patch uploaded by bklang (license 919) 20100106__issue16416__trunk.diff.txt uploaded by tilghman (license 14) Tested by: bklang ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=239308 By: Digium Subversion (svnbot) 2010-01-11 21:24:06.000-0600 Repository: asterisk Revision: 239309 _U branches/1.6.0/ U branches/1.6.0/contrib/scripts/safe_asterisk ------------------------------------------------------------------------ r239309 | tilghman | 2010-01-11 21:24:06 -0600 (Mon, 11 Jan 2010) | 22 lines Merged revisions 239308 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ................ r239308 | tilghman | 2010-01-11 21:21:40 -0600 (Mon, 11 Jan 2010) | 15 lines Merged revisions 239307 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r239307 | tilghman | 2010-01-11 21:18:36 -0600 (Mon, 11 Jan 2010) | 8 lines Portability and other fixes for the safe_asterisk script (closes issue ASTERISK-15293) Reported by: bklang Patches: safe_asterisk-compat-1.patch uploaded by bklang (license 919) 20100106__issue16416__trunk.diff.txt uploaded by tilghman (license 14) Tested by: bklang ........ ................ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=239309 By: Digium Subversion (svnbot) 2010-01-11 21:25:13.000-0600 Repository: asterisk Revision: 239310 _U branches/1.6.1/ U branches/1.6.1/contrib/scripts/safe_asterisk ------------------------------------------------------------------------ r239310 | tilghman | 2010-01-11 21:25:12 -0600 (Mon, 11 Jan 2010) | 22 lines Merged revisions 239308 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ................ r239308 | tilghman | 2010-01-11 21:21:40 -0600 (Mon, 11 Jan 2010) | 15 lines Merged revisions 239307 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r239307 | tilghman | 2010-01-11 21:18:36 -0600 (Mon, 11 Jan 2010) | 8 lines Portability and other fixes for the safe_asterisk script (closes issue ASTERISK-15293) Reported by: bklang Patches: safe_asterisk-compat-1.patch uploaded by bklang (license 919) 20100106__issue16416__trunk.diff.txt uploaded by tilghman (license 14) Tested by: bklang ........ ................ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=239310 By: Digium Subversion (svnbot) 2010-01-11 21:25:20.000-0600 Repository: asterisk Revision: 239311 _U branches/1.6.2/ U branches/1.6.2/contrib/scripts/safe_asterisk ------------------------------------------------------------------------ r239311 | tilghman | 2010-01-11 21:25:20 -0600 (Mon, 11 Jan 2010) | 22 lines Merged revisions 239308 via svnmerge from https://origsvn.digium.com/svn/asterisk/trunk ................ r239308 | tilghman | 2010-01-11 21:21:40 -0600 (Mon, 11 Jan 2010) | 15 lines Merged revisions 239307 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r239307 | tilghman | 2010-01-11 21:18:36 -0600 (Mon, 11 Jan 2010) | 8 lines Portability and other fixes for the safe_asterisk script (closes issue ASTERISK-15293) Reported by: bklang Patches: safe_asterisk-compat-1.patch uploaded by bklang (license 919) 20100106__issue16416__trunk.diff.txt uploaded by tilghman (license 14) Tested by: bklang ........ ................ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=239311 | ||