* Script broken after updating bash to 4.3.46-7? @ 2016-08-27 10:28 Gene Pavlovsky 2016-08-27 11:25 ` Gene Pavlovsky 2016-08-30 3:21 ` Eric Blake 0 siblings, 2 replies; 14+ messages in thread From: Gene Pavlovsky @ 2016-08-27 10:28 UTC (permalink / raw) To: cygwin After I updated Cygwin yesterday, a daily database backup bash script (`automysqlbackup`) broke. My previous bash was 4.3.42-4 (installed when I updated Cygwin on 2016/07/23), current is 4.3.46-7. Here's the code snippet: ```bash local i;i=0; while read -r; do alldbnames[i++]="$REPLY"; done < <(mysql --user="${CONFIG_mysql_dump_username}" --password="${CONFIG_mysql_dump_password}" --host="${CONFIG_mysql_dump_host}" "${mysql_opt[@]}" --batch --skip-column-names -e "show databases") ``` This is supposed to get the list of all databases. Before it worked. Now every item on the list ends with the CR character ($'\r'), causing a bunch of issues with further script opreation. I'm using official MariaDB Windows x64 binaries. Frankly if mysql does output CRLF line endings, I don't know why that script worked before, considering that this command in an interactive shell produces a similar result: ```bash # echo $'information_schema\r' | { read -r var; echo "{$var}"; } }information_schema ``` But it did work somehow... Question is - what made it stop working now, and what would be best way to fix it? Regards, Gene. -- Problem reports: http://cygwin.com/problems.html FAQ: http://cygwin.com/faq/ Documentation: http://cygwin.com/docs.html Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Script broken after updating bash to 4.3.46-7? 2016-08-27 10:28 Script broken after updating bash to 4.3.46-7? Gene Pavlovsky @ 2016-08-27 11:25 ` Gene Pavlovsky 2016-08-27 17:24 ` Andrey Repin 2016-08-30 3:21 ` Eric Blake 1 sibling, 1 reply; 14+ messages in thread From: Gene Pavlovsky @ 2016-08-27 11:25 UTC (permalink / raw) To: cygwin Looks like it's related to a recent change in bash, which is `read` now honors Cygwin-specific `igncr` shell option (`set -o igncr`), which I didn't enable. Adding `set -o igncr` to the top of the script does the job, however I'd like to know how many more scripts are potentially malfunctioning now? It's lucky that one sent e-mails on errors, some others might just break silently. Is it advisable to add `set -o igncr` to /etc/profile or SHELLOPTS? I didn't use that feature before and am worried about some other negative side effects. --Gene On 27 August 2016 at 02:49, Gene Pavlovsky <gene.pavlovsky@gmail.com> wrote: > After I updated Cygwin yesterday, a daily database backup bash script > (`automysqlbackup`) broke. > My previous bash was 4.3.42-4 (installed when I updated Cygwin on > 2016/07/23), current is 4.3.46-7. > Here's the code snippet: > ```bash > local i;i=0; > while read -r; do alldbnames[i++]="$REPLY"; done < <(mysql > --user="${CONFIG_mysql_dump_username}" > --password="${CONFIG_mysql_dump_password}" > --host="${CONFIG_mysql_dump_host}" "${mysql_opt[@]}" --batch > --skip-column-names -e "show databases") > ``` > This is supposed to get the list of all databases. Before it worked. > Now every item on the list ends with the CR character ($'\r'), causing > a bunch of issues with further script opreation. I'm using official > MariaDB Windows x64 binaries. > Frankly if mysql does output CRLF line endings, I don't know why that > script worked before, considering that this command in an interactive > shell produces a similar result: > ```bash > # echo $'information_schema\r' | { read -r var; echo "{$var}"; } > }information_schema > ``` > But it did work somehow... Question is - what made it stop working > now, and what would be best way to fix it? > > Regards, > Gene. -- Problem reports: http://cygwin.com/problems.html FAQ: http://cygwin.com/faq/ Documentation: http://cygwin.com/docs.html Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Script broken after updating bash to 4.3.46-7? 2016-08-27 11:25 ` Gene Pavlovsky @ 2016-08-27 17:24 ` Andrey Repin 2016-08-28 10:19 ` Gene Pavlovsky 2016-08-29 14:05 ` cyg Simple 0 siblings, 2 replies; 14+ messages in thread From: Andrey Repin @ 2016-08-27 17:24 UTC (permalink / raw) To: Gene Pavlovsky, cygwin Greetings, Gene Pavlovsky! > Looks like it's related to a recent change in bash, which is `read` > now honors Cygwin-specific `igncr` shell option (`set -o igncr`), > which I didn't enable. > Adding `set -o igncr` to the top of the script does the job, however > I'd like to know how many more scripts are potentially malfunctioning > now? > It's lucky that one sent e-mails on errors, some others might just > break silently. > Is it advisable to add `set -o igncr` to /etc/profile or SHELLOPTS? I > didn't use that feature before and am worried about some other > negative side effects. It is advisable to not have CR's in your scripts to begin with. -- With best regards, Andrey Repin Saturday, August 27, 2016 19:14:10 Sorry for my terrible english... -- Problem reports: http://cygwin.com/problems.html FAQ: http://cygwin.com/faq/ Documentation: http://cygwin.com/docs.html Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Script broken after updating bash to 4.3.46-7? 2016-08-27 17:24 ` Andrey Repin @ 2016-08-28 10:19 ` Gene Pavlovsky 2016-08-29 13:28 ` Nellis, Kenneth 2016-08-29 14:05 ` cyg Simple 1 sibling, 1 reply; 14+ messages in thread From: Gene Pavlovsky @ 2016-08-28 10:19 UTC (permalink / raw) To: cygwin Andrey, That's what I personally think, none of the scripts I use have CRs, and this is why I'd prefer not using the `igncr` option. However the recent change to how `read` works makes it necessary to modify existing scripts which interoperate with Windows console programs (in my case, Windows build of mysql (MariaDB)) that produce CRLF line endings. --Gene On 27 August 2016 at 19:15, Andrey Repin <anrdaemon@yandex.ru> wrote: > Greetings, Gene Pavlovsky! > >> Looks like it's related to a recent change in bash, which is `read` >> now honors Cygwin-specific `igncr` shell option (`set -o igncr`), >> which I didn't enable. >> Adding `set -o igncr` to the top of the script does the job, however >> I'd like to know how many more scripts are potentially malfunctioning >> now? >> It's lucky that one sent e-mails on errors, some others might just >> break silently. >> Is it advisable to add `set -o igncr` to /etc/profile or SHELLOPTS? I >> didn't use that feature before and am worried about some other >> negative side effects. > > It is advisable to not have CR's in your scripts to begin with. > > > -- > With best regards, > Andrey Repin > Saturday, August 27, 2016 19:14:10 > > Sorry for my terrible english... > -- Problem reports: http://cygwin.com/problems.html FAQ: http://cygwin.com/faq/ Documentation: http://cygwin.com/docs.html Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: Script broken after updating bash to 4.3.46-7? 2016-08-28 10:19 ` Gene Pavlovsky @ 2016-08-29 13:28 ` Nellis, Kenneth 0 siblings, 0 replies; 14+ messages in thread From: Nellis, Kenneth @ 2016-08-29 13:28 UTC (permalink / raw) To: cygwin From: Gene Pavlovsky > Andrey, > That's what I personally think, none of the scripts I use have CRs, > and this is why I'd prefer not using the `igncr` option. > However the recent change to how `read` works makes it necessary to > modify existing scripts which interoperate with Windows console > programs (in my case, Windows build of mysql (MariaDB)) that produce > CRLF line endings. > --Gene I sympathize, but it could be argued that your script was invalid; that it properly should have piped the output of mysql through d2u. --Ken Nellis ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Script broken after updating bash to 4.3.46-7? 2016-08-27 17:24 ` Andrey Repin 2016-08-28 10:19 ` Gene Pavlovsky @ 2016-08-29 14:05 ` cyg Simple 1 sibling, 0 replies; 14+ messages in thread From: cyg Simple @ 2016-08-29 14:05 UTC (permalink / raw) To: cygwin On 8/27/2016 12:15 PM, Andrey Repin wrote: > Greetings, Gene Pavlovsky! > >> Looks like it's related to a recent change in bash, which is `read` >> now honors Cygwin-specific `igncr` shell option (`set -o igncr`), >> which I didn't enable. >> Adding `set -o igncr` to the top of the script does the job, however >> I'd like to know how many more scripts are potentially malfunctioning >> now? >> It's lucky that one sent e-mails on errors, some others might just >> break silently. >> Is it advisable to add `set -o igncr` to /etc/profile or SHELLOPTS? I >> didn't use that feature before and am worried about some other >> negative side effects. > > It is advisable to not have CR's in your scripts to begin with. > I thought this was about the data the script was reading and not the script itself. Data can have \r regardless of where in the data it exists. That said relying on defaults for a process has been taboo since the dawn of computing. Often defaults change on a whim, they are bicycle shed colors, a maintainer may prefer blue instead of red in any given release. If your script requires a specific mode for the shell to process data properly then be sure to set it before reading the data. -- cyg Simple -- Problem reports: http://cygwin.com/problems.html FAQ: http://cygwin.com/faq/ Documentation: http://cygwin.com/docs.html Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Script broken after updating bash to 4.3.46-7? 2016-08-27 10:28 Script broken after updating bash to 4.3.46-7? Gene Pavlovsky 2016-08-27 11:25 ` Gene Pavlovsky @ 2016-08-30 3:21 ` Eric Blake 2016-08-30 18:21 ` cyg Simple 1 sibling, 1 reply; 14+ messages in thread From: Eric Blake @ 2016-08-30 3:21 UTC (permalink / raw) To: cygwin [-- Attachment #1.1: Type: text/plain, Size: 1397 bytes --] On 08/26/2016 06:49 PM, Gene Pavlovsky wrote: > After I updated Cygwin yesterday, a daily database backup bash script > (`automysqlbackup`) broke. > My previous bash was 4.3.42-4 (installed when I updated Cygwin on > 2016/07/23), current is 4.3.46-7. > Here's the code snippet: > ```bash > local i;i=0; > while read -r; do alldbnames[i++]="$REPLY"; done < <(mysql > --user="${CONFIG_mysql_dump_username}" > --password="${CONFIG_mysql_dump_password}" > --host="${CONFIG_mysql_dump_host}" "${mysql_opt[@]}" --batch > --skip-column-names -e "show databases") > ``` > This is supposed to get the list of all databases. Before it worked. By pure luck. The 'read' builtin was inconsistent on cygwin, in that it FORCED people to use text mode (and ate \r before \n), even when they wanted pure binary input, which was different than all other aspects of bash; and worse had a bug where \r before something else would cause the something else to be incorrectly eaten. > Now every item on the list ends with the CR character ($'\r'), causing > a bunch of issues with further script opreation. I'm using official > MariaDB Windows x64 binaries. Simplest fix: read ... < <(mysql ... | dos2unix) There. Now you aren't feeding \r to read in the first place. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Script broken after updating bash to 4.3.46-7? 2016-08-30 3:21 ` Eric Blake @ 2016-08-30 18:21 ` cyg Simple 2016-08-30 18:32 ` Eric Blake 0 siblings, 1 reply; 14+ messages in thread From: cyg Simple @ 2016-08-30 18:21 UTC (permalink / raw) To: cygwin On 8/29/2016 2:30 PM, Eric Blake wrote: > > Simplest fix: > > read ... < <(mysql ... | dos2unix) > This will break when the data returned by mysql is supposed to contain \r. > There. Now you aren't feeding \r to read in the first place. > But you might want to feed \r to read. It isn't a fix, it is a potential work around dependent on the data set results. If a read that is supposed to be reading binary data doesn't pass all of the data to the routine then it is broken. -- cyg Simple -- Problem reports: http://cygwin.com/problems.html FAQ: http://cygwin.com/faq/ Documentation: http://cygwin.com/docs.html Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Script broken after updating bash to 4.3.46-7? 2016-08-30 18:21 ` cyg Simple @ 2016-08-30 18:32 ` Eric Blake 2016-08-31 10:16 ` cyg Simple 0 siblings, 1 reply; 14+ messages in thread From: Eric Blake @ 2016-08-30 18:32 UTC (permalink / raw) To: cygwin [-- Attachment #1.1: Type: text/plain, Size: 2246 bytes --] On 08/30/2016 12:04 PM, cyg Simple wrote: > On 8/29/2016 2:30 PM, Eric Blake wrote: >> >> Simplest fix: >> >> read ... < <(mysql ... | dos2unix) >> > > This will break when the data returned by mysql is supposed to contain \r. > >> There. Now you aren't feeding \r to read in the first place. >> > > But you might want to feed \r to read. It isn't a fix, it is a > potential work around dependent on the data set results. If a read that > is supposed to be reading binary data doesn't pass all of the data to > the routine then it is broken. Now we're talking past each other. That's what the recent bash fixed. 'read' in bash 3.2.42-4 was broken - it corrupted binary data, with no recourse, by eating \r (and worse, by sometimes eating the byte after \r). 'read' in bash 3.2.46-7 is fixed - by default it is strictly binary (all bytes are read as-is, including \r), but can also be switched to text mode (using 'igncr', all \r are ignored). If you want to preserve mid-line \r but treat line endings of \r\n as a single byte, then leave binary mode on and strip the line endings via a separate tool like d2u (note, however, that it is very rare to have data where mid-line \r is important but line-ending \r\n should be treated as plain \n). I strongly think that using igncr is a crutch, and you normally shouldn't use it; particularly not if you want to be portable to other platforms. Instead, massaging your data through d2u is a great way to be portable. But sometimes the ease of ignoring ALL \r is easier than worrying about portability, so I keep the 'igncr' code in Cygwin. And it is only because the OP tried using 'igncr' in the first place (whether or not it was actually needed) that we have now flushed out the existence of a latent bug in the 'igncr' implementation that interacts weirdly with $()\n in PS1. On that front, I'm still hoping to find time to debug and/or for someone to post a patch. But whether PS1 behaves weirdly under 'igncr' is orthogonal to my suggestion above - using 'mysql|d2u' is a great way to avoid the need to worry about 'igncr'. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Script broken after updating bash to 4.3.46-7? 2016-08-30 18:32 ` Eric Blake @ 2016-08-31 10:16 ` cyg Simple 2016-09-04 9:38 ` Gene Pavlovsky 0 siblings, 1 reply; 14+ messages in thread From: cyg Simple @ 2016-08-31 10:16 UTC (permalink / raw) To: cygwin On 8/30/2016 1:38 PM, Eric Blake wrote: > On 08/30/2016 12:04 PM, cyg Simple wrote: >> On 8/29/2016 2:30 PM, Eric Blake wrote: >>> >>> Simplest fix: >>> >>> read ... < <(mysql ... | dos2unix) >>> >> >> This will break when the data returned by mysql is supposed to contain \r. >> >>> There. Now you aren't feeding \r to read in the first place. >>> >> >> But you might want to feed \r to read. It isn't a fix, it is a >> potential work around dependent on the data set results. If a read that >> is supposed to be reading binary data doesn't pass all of the data to >> the routine then it is broken. > > Now we're talking past each other. > > That's what the recent bash fixed. 'read' in bash 3.2.42-4 was broken - > it corrupted binary data, with no recourse, by eating \r (and worse, by > sometimes eating the byte after \r). 'read' in bash 3.2.46-7 is fixed - > by default it is strictly binary (all bytes are read as-is, including > \r), but can also be switched to text mode (using 'igncr', all \r are > ignored). If you want to preserve mid-line \r but treat line endings of > \r\n as a single byte, then leave binary mode on and strip the line > endings via a separate tool like d2u (note, however, that it is very > rare to have data where mid-line \r is important but line-ending \r\n > should be treated as plain \n). > > I strongly think that using igncr is a crutch, and you normally > shouldn't use it; particularly not if you want to be portable to other > platforms. Instead, massaging your data through d2u is a great way to > be portable. But sometimes the ease of ignoring ALL \r is easier than > worrying about portability, so I keep the 'igncr' code in Cygwin. > > And it is only because the OP tried using 'igncr' in the first place > (whether or not it was actually needed) that we have now flushed out the > existence of a latent bug in the 'igncr' implementation that interacts > weirdly with $()\n in PS1. On that front, I'm still hoping to find time > to debug and/or for someone to post a patch. But whether PS1 behaves > weirdly under 'igncr' is orthogonal to my suggestion above - using > 'mysql|d2u' is a great way to avoid the need to worry about 'igncr'. > Thank you for the retort Eric. Happy to know that it is fixed which in the back of my mind I knew already. I can imagine data such as full message email or a small document data containing \r\n as valid data in the database field and if you use a line ending conversion utility you might loose that data. -- cyg Simple -- Problem reports: http://cygwin.com/problems.html FAQ: http://cygwin.com/faq/ Documentation: http://cygwin.com/docs.html Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Script broken after updating bash to 4.3.46-7? 2016-08-31 10:16 ` cyg Simple @ 2016-09-04 9:38 ` Gene Pavlovsky 2016-09-04 17:37 ` Lee 2016-09-06 13:15 ` Eric Blake 0 siblings, 2 replies; 14+ messages in thread From: Gene Pavlovsky @ 2016-09-04 9:38 UTC (permalink / raw) To: cygwin This issue never affected me personally, but it sounds like a serious bug and I'm as glad as anybody that it's finally fixed. However, having existing scripts suddenly breaking is not great. I'd like to remind that I'm not the author of the script in question, [AutoMySQLBackup](http://sourceforge.net/projects/automysqlbackup/). If I put "| d2u" there I'll have to remember it and do it every time automysqlbackup is updated - or create and maintain a Cygwin package for this script. And who knows how many other scripts might be broken, I just didn't find it yet? Having a `read`-specific shell option telling read to treat `\r\n` (and only `\r\n`, not \r followed by something else) same as `\n` would be bad things to have? For me, this kind of option would be more useful than the `igncr` crutch Let me say it another way - in OOP programming, one of good practices is Single Responsibility Principle - a class should be responsible for only one feature/function, and that feature/function should be totally encapsulated in that class. Similar to that, an option should be responsible for one behavior. With this change to `read`, the `igncr` shell option is starting to look like a kitchen sink... split it into separate options, please! I think making UNIX scripts work on Cygwin with no or minimum modifications (or bug-hunting) should be one of high priorities, no? If some scripts erroneously have CRLF line endings, it's easy to find and `d2u` them, rather than using the `igncr` crutch, but with the recent change to `read`, countless scripts might be broken in a non-obvious way. Fixing them would require finding out they're broken, in the first place. Imagine if I didn't set up my cron to e-mail me the cron jobs output? My backup script would just stop working silently, and some time later when I needed a recent backup, I would find out there aren't any. There might be something else lurking that I haven't found yet. Once a script, broken by this change to `read`, is found, it must be checked thoroughly to find out where exactly is the problem, where to put '| d2u', or maybe 'set -o igncr'. These fixes must also be applied anytime a 3rd party script is updated. Quite a lot of work! Hope you will consider my point. Regards, Gene. On 30 August 2016 at 23:57, cyg Simple <cygsimple@gmail.com> wrote: > On 8/30/2016 1:38 PM, Eric Blake wrote: >> On 08/30/2016 12:04 PM, cyg Simple wrote: >>> On 8/29/2016 2:30 PM, Eric Blake wrote: >>>> >>>> Simplest fix: >>>> >>>> read ... < <(mysql ... | dos2unix) >>>> >>> >>> This will break when the data returned by mysql is supposed to contain \r. >>> >>>> There. Now you aren't feeding \r to read in the first place. >>>> >>> >>> But you might want to feed \r to read. It isn't a fix, it is a >>> potential work around dependent on the data set results. If a read that >>> is supposed to be reading binary data doesn't pass all of the data to >>> the routine then it is broken. >> >> Now we're talking past each other. >> >> That's what the recent bash fixed. 'read' in bash 3.2.42-4 was broken - >> it corrupted binary data, with no recourse, by eating \r (and worse, by >> sometimes eating the byte after \r). 'read' in bash 3.2.46-7 is fixed - >> by default it is strictly binary (all bytes are read as-is, including >> \r), but can also be switched to text mode (using 'igncr', all \r are >> ignored). If you want to preserve mid-line \r but treat line endings of >> \r\n as a single byte, then leave binary mode on and strip the line >> endings via a separate tool like d2u (note, however, that it is very >> rare to have data where mid-line \r is important but line-ending \r\n >> should be treated as plain \n). >> >> I strongly think that using igncr is a crutch, and you normally >> shouldn't use it; particularly not if you want to be portable to other >> platforms. Instead, massaging your data through d2u is a great way to >> be portable. But sometimes the ease of ignoring ALL \r is easier than >> worrying about portability, so I keep the 'igncr' code in Cygwin. >> >> And it is only because the OP tried using 'igncr' in the first place >> (whether or not it was actually needed) that we have now flushed out the >> existence of a latent bug in the 'igncr' implementation that interacts >> weirdly with $()\n in PS1. On that front, I'm still hoping to find time >> to debug and/or for someone to post a patch. But whether PS1 behaves >> weirdly under 'igncr' is orthogonal to my suggestion above - using >> 'mysql|d2u' is a great way to avoid the need to worry about 'igncr'. >> > > Thank you for the retort Eric. Happy to know that it is fixed which in > the back of my mind I knew already. I can imagine data such as full > message email or a small document data containing \r\n as valid data in > the database field and if you use a line ending conversion utility you > might loose that data. > > -- > cyg Simple > > -- > Problem reports: http://cygwin.com/problems.html > FAQ: http://cygwin.com/faq/ > Documentation: http://cygwin.com/docs.html > Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple > -- Problem reports: http://cygwin.com/problems.html FAQ: http://cygwin.com/faq/ Documentation: http://cygwin.com/docs.html Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Script broken after updating bash to 4.3.46-7? 2016-09-04 9:38 ` Gene Pavlovsky @ 2016-09-04 17:37 ` Lee 2016-09-05 23:37 ` Gene Pavlovsky 2016-09-06 13:15 ` Eric Blake 1 sibling, 1 reply; 14+ messages in thread From: Lee @ 2016-09-04 17:37 UTC (permalink / raw) To: cygwin On 9/4/16, Gene Pavlovsky wrote: > This issue never affected me personally, but it sounds like a serious > bug and I'm as glad as anybody that it's finally fixed. > However, having existing scripts suddenly breaking is not great. I'd > like to remind that I'm not the author of the script in question, > [AutoMySQLBackup](http://sourceforge.net/projects/automysqlbackup/). > If I put "| d2u" there I'll have to remember it and do it every time > automysqlbackup is updated ... Right - updating something like automysqlbackup to add "| d2u" after every dos program call is a non-starter. But would putting the dos program inside a script that converted dos -> unix line endings work? eg - have a mysql script that comes before the windows version of mysql in your path that does something like /path/to/windows/version/of/mysql $@ | dos2unix > - or create and maintain a Cygwin package for this script. It seems to me that it isn't the script that's broken - it's the whole idea of having a dos program transparently integrate into an environment that expects unix line endings that's broken. So until there's a bash option that automatically translates '\r\n' line endings into '\n' line endings you're stuck doing some kind of work-around or using a cygwin version of mysql. Lee > And who knows how many other scripts might be broken, > I just didn't find it yet? > > Having a `read`-specific shell option telling read to treat `\r\n` > (and only `\r\n`, not \r followed by something else) same as `\n` > would be bad things to have? For me, this kind of option would be more > useful than the `igncr` crutch > Let me say it another way - in OOP programming, one of good practices > is Single Responsibility Principle - a class should be responsible for > only one feature/function, and that feature/function should be totally > encapsulated in that class. Similar to that, an option should be > responsible for one behavior. With this change to `read`, the `igncr` > shell option is starting to look like a kitchen sink... split it into > separate options, please! > I think making UNIX scripts work on Cygwin with no or minimum > modifications (or bug-hunting) should be one of high priorities, no? > If some scripts erroneously have CRLF line endings, it's easy to find > and `d2u` them, rather than using the `igncr` crutch, but with the > recent change to `read`, countless scripts might be broken in a > non-obvious way. Fixing them would require finding out they're broken, > in the first place. Imagine if I didn't set up my cron to e-mail me > the cron jobs output? My backup script would just stop working > silently, and some time later when I needed a recent backup, I would > find out there aren't any. There might be something else lurking that > I haven't found yet. Once a script, broken by this change to `read`, > is found, it must be checked thoroughly to find out where exactly is > the problem, where to put '| d2u', or maybe 'set -o igncr'. These > fixes must also be applied anytime a 3rd party script is updated. > Quite a lot of work! > Hope you will consider my point. > Regards, > Gene. > > On 30 August 2016 at 23:57, cyg Simple <cygsimple@gmail.com> wrote: >> On 8/30/2016 1:38 PM, Eric Blake wrote: >>> On 08/30/2016 12:04 PM, cyg Simple wrote: >>>> On 8/29/2016 2:30 PM, Eric Blake wrote: >>>>> >>>>> Simplest fix: >>>>> >>>>> read ... < <(mysql ... | dos2unix) >>>>> >>>> >>>> This will break when the data returned by mysql is supposed to contain >>>> \r. >>>> >>>>> There. Now you aren't feeding \r to read in the first place. >>>>> >>>> >>>> But you might want to feed \r to read. It isn't a fix, it is a >>>> potential work around dependent on the data set results. If a read >>>> that >>>> is supposed to be reading binary data doesn't pass all of the data to >>>> the routine then it is broken. >>> >>> Now we're talking past each other. >>> >>> That's what the recent bash fixed. 'read' in bash 3.2.42-4 was broken - >>> it corrupted binary data, with no recourse, by eating \r (and worse, by >>> sometimes eating the byte after \r). 'read' in bash 3.2.46-7 is fixed - >>> by default it is strictly binary (all bytes are read as-is, including >>> \r), but can also be switched to text mode (using 'igncr', all \r are >>> ignored). If you want to preserve mid-line \r but treat line endings of >>> \r\n as a single byte, then leave binary mode on and strip the line >>> endings via a separate tool like d2u (note, however, that it is very >>> rare to have data where mid-line \r is important but line-ending \r\n >>> should be treated as plain \n). >>> >>> I strongly think that using igncr is a crutch, and you normally >>> shouldn't use it; particularly not if you want to be portable to other >>> platforms. Instead, massaging your data through d2u is a great way to >>> be portable. But sometimes the ease of ignoring ALL \r is easier than >>> worrying about portability, so I keep the 'igncr' code in Cygwin. >>> >>> And it is only because the OP tried using 'igncr' in the first place >>> (whether or not it was actually needed) that we have now flushed out the >>> existence of a latent bug in the 'igncr' implementation that interacts >>> weirdly with $()\n in PS1. On that front, I'm still hoping to find time >>> to debug and/or for someone to post a patch. But whether PS1 behaves >>> weirdly under 'igncr' is orthogonal to my suggestion above - using >>> 'mysql|d2u' is a great way to avoid the need to worry about 'igncr'. >>> >> >> Thank you for the retort Eric. Happy to know that it is fixed which in >> the back of my mind I knew already. I can imagine data such as full >> message email or a small document data containing \r\n as valid data in >> the database field and if you use a line ending conversion utility you >> might loose that data. >> >> -- >> cyg Simple >> >> -- >> Problem reports: http://cygwin.com/problems.html >> FAQ: http://cygwin.com/faq/ >> Documentation: http://cygwin.com/docs.html >> Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple >> > > -- > Problem reports: http://cygwin.com/problems.html > FAQ: http://cygwin.com/faq/ > Documentation: http://cygwin.com/docs.html > Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple > > -- Problem reports: http://cygwin.com/problems.html FAQ: http://cygwin.com/faq/ Documentation: http://cygwin.com/docs.html Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Script broken after updating bash to 4.3.46-7? 2016-09-04 17:37 ` Lee @ 2016-09-05 23:37 ` Gene Pavlovsky 0 siblings, 0 replies; 14+ messages in thread From: Gene Pavlovsky @ 2016-09-05 23:37 UTC (permalink / raw) To: cygwin Lee, you have a point. Adding mysql.sh that runs `mysql.exe | d2u`, or using a cygwin version of mysql looks like the most viable solution at the moment. Or staying with the older version of bash, in which `read` still handled `\r\n` line endings... My point is OK I'll fix that script, who knows what other scripts might be broken now? And each one might need an investigation before I find out which program's windows version needs a wrapper script etc. --Gene On 4 September 2016 at 20:37, Lee <ler762@gmail.com> wrote: > On 9/4/16, Gene Pavlovsky wrote: >> This issue never affected me personally, but it sounds like a serious >> bug and I'm as glad as anybody that it's finally fixed. >> However, having existing scripts suddenly breaking is not great. I'd >> like to remind that I'm not the author of the script in question, >> [AutoMySQLBackup](http://sourceforge.net/projects/automysqlbackup/). >> If I put "| d2u" there I'll have to remember it and do it every time >> automysqlbackup is updated ... > > Right - updating something like automysqlbackup to add "| d2u" after > every dos program call is a non-starter. But would putting the dos > program inside a script that converted dos -> unix line endings work? > eg - have a mysql script that comes before the windows version of > mysql in your path that does something like > /path/to/windows/version/of/mysql $@ | dos2unix > >> - or create and maintain a Cygwin package for this script. > > It seems to me that it isn't the script that's broken - it's the whole > idea of having a dos program transparently integrate into an > environment that expects unix line endings that's broken. So until > there's a bash option that automatically translates '\r\n' line > endings into '\n' line endings you're stuck doing some kind of > work-around or using a cygwin version of mysql. > > Lee > > >> And who knows how many other scripts might be broken, >> I just didn't find it yet? >> >> Having a `read`-specific shell option telling read to treat `\r\n` >> (and only `\r\n`, not \r followed by something else) same as `\n` >> would be bad things to have? For me, this kind of option would be more >> useful than the `igncr` crutch >> Let me say it another way - in OOP programming, one of good practices >> is Single Responsibility Principle - a class should be responsible for >> only one feature/function, and that feature/function should be totally >> encapsulated in that class. Similar to that, an option should be >> responsible for one behavior. With this change to `read`, the `igncr` >> shell option is starting to look like a kitchen sink... split it into >> separate options, please! >> I think making UNIX scripts work on Cygwin with no or minimum >> modifications (or bug-hunting) should be one of high priorities, no? >> If some scripts erroneously have CRLF line endings, it's easy to find >> and `d2u` them, rather than using the `igncr` crutch, but with the >> recent change to `read`, countless scripts might be broken in a >> non-obvious way. Fixing them would require finding out they're broken, >> in the first place. Imagine if I didn't set up my cron to e-mail me >> the cron jobs output? My backup script would just stop working >> silently, and some time later when I needed a recent backup, I would >> find out there aren't any. There might be something else lurking that >> I haven't found yet. Once a script, broken by this change to `read`, >> is found, it must be checked thoroughly to find out where exactly is >> the problem, where to put '| d2u', or maybe 'set -o igncr'. These >> fixes must also be applied anytime a 3rd party script is updated. >> Quite a lot of work! >> Hope you will consider my point. >> Regards, >> Gene. >> >> On 30 August 2016 at 23:57, cyg Simple <cygsimple@gmail.com> wrote: >>> On 8/30/2016 1:38 PM, Eric Blake wrote: >>>> On 08/30/2016 12:04 PM, cyg Simple wrote: >>>>> On 8/29/2016 2:30 PM, Eric Blake wrote: >>>>>> >>>>>> Simplest fix: >>>>>> >>>>>> read ... < <(mysql ... | dos2unix) >>>>>> >>>>> >>>>> This will break when the data returned by mysql is supposed to contain >>>>> \r. >>>>> >>>>>> There. Now you aren't feeding \r to read in the first place. >>>>>> >>>>> >>>>> But you might want to feed \r to read. It isn't a fix, it is a >>>>> potential work around dependent on the data set results. If a read >>>>> that >>>>> is supposed to be reading binary data doesn't pass all of the data to >>>>> the routine then it is broken. >>>> >>>> Now we're talking past each other. >>>> >>>> That's what the recent bash fixed. 'read' in bash 3.2.42-4 was broken - >>>> it corrupted binary data, with no recourse, by eating \r (and worse, by >>>> sometimes eating the byte after \r). 'read' in bash 3.2.46-7 is fixed - >>>> by default it is strictly binary (all bytes are read as-is, including >>>> \r), but can also be switched to text mode (using 'igncr', all \r are >>>> ignored). If you want to preserve mid-line \r but treat line endings of >>>> \r\n as a single byte, then leave binary mode on and strip the line >>>> endings via a separate tool like d2u (note, however, that it is very >>>> rare to have data where mid-line \r is important but line-ending \r\n >>>> should be treated as plain \n). >>>> >>>> I strongly think that using igncr is a crutch, and you normally >>>> shouldn't use it; particularly not if you want to be portable to other >>>> platforms. Instead, massaging your data through d2u is a great way to >>>> be portable. But sometimes the ease of ignoring ALL \r is easier than >>>> worrying about portability, so I keep the 'igncr' code in Cygwin. >>>> >>>> And it is only because the OP tried using 'igncr' in the first place >>>> (whether or not it was actually needed) that we have now flushed out the >>>> existence of a latent bug in the 'igncr' implementation that interacts >>>> weirdly with $()\n in PS1. On that front, I'm still hoping to find time >>>> to debug and/or for someone to post a patch. But whether PS1 behaves >>>> weirdly under 'igncr' is orthogonal to my suggestion above - using >>>> 'mysql|d2u' is a great way to avoid the need to worry about 'igncr'. >>>> >>> >>> Thank you for the retort Eric. Happy to know that it is fixed which in >>> the back of my mind I knew already. I can imagine data such as full >>> message email or a small document data containing \r\n as valid data in >>> the database field and if you use a line ending conversion utility you >>> might loose that data. >>> >>> -- >>> cyg Simple >>> >>> -- >>> Problem reports: http://cygwin.com/problems.html >>> FAQ: http://cygwin.com/faq/ >>> Documentation: http://cygwin.com/docs.html >>> Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple >>> >> >> -- >> Problem reports: http://cygwin.com/problems.html >> FAQ: http://cygwin.com/faq/ >> Documentation: http://cygwin.com/docs.html >> Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple >> >> > > -- > Problem reports: http://cygwin.com/problems.html > FAQ: http://cygwin.com/faq/ > Documentation: http://cygwin.com/docs.html > Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple > -- Problem reports: http://cygwin.com/problems.html FAQ: http://cygwin.com/faq/ Documentation: http://cygwin.com/docs.html Unsubscribe info: http://cygwin.com/ml/#unsubscribe-simple ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Script broken after updating bash to 4.3.46-7? 2016-09-04 9:38 ` Gene Pavlovsky 2016-09-04 17:37 ` Lee @ 2016-09-06 13:15 ` Eric Blake 1 sibling, 0 replies; 14+ messages in thread From: Eric Blake @ 2016-09-06 13:15 UTC (permalink / raw) To: cygwin [-- Attachment #1.1: Type: text/plain, Size: 636 bytes --] On 09/04/2016 04:38 AM, Gene Pavlovsky wrote: > This issue never affected me personally, but it sounds like a serious > bug and I'm as glad as anybody that it's finally fixed. > However, having existing scripts suddenly breaking is not great. I'd > like to remind that I'm not the author of the script in question, > [AutoMySQLBackup](http://sourceforge.net/projects/automysqlbackup/). Then submit a bug report to the author of that script to tell them to fix their buggy script to portably strip carriage returns. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 604 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2016-09-06 13:15 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-08-27 10:28 Script broken after updating bash to 4.3.46-7? Gene Pavlovsky 2016-08-27 11:25 ` Gene Pavlovsky 2016-08-27 17:24 ` Andrey Repin 2016-08-28 10:19 ` Gene Pavlovsky 2016-08-29 13:28 ` Nellis, Kenneth 2016-08-29 14:05 ` cyg Simple 2016-08-30 3:21 ` Eric Blake 2016-08-30 18:21 ` cyg Simple 2016-08-30 18:32 ` Eric Blake 2016-08-31 10:16 ` cyg Simple 2016-09-04 9:38 ` Gene Pavlovsky 2016-09-04 17:37 ` Lee 2016-09-05 23:37 ` Gene Pavlovsky 2016-09-06 13:15 ` Eric Blake
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).