Hi Colomban,
RE your discussion on IRC about $subject
To save you searching, some info about g_spawn not working (right) on windows, and that it won't be fixed:
http://article.gmane.org/gmane.editors.geany.devel/583
and it appears that it was always recognised that splitting on space wasn't an elegant way of generating argv for windows, and the problems that causes for filenames containing spaces, but nobody has been concerned enough to fix it :) see:
http://lists.uvena.de/geany/2007-November/002082.html
There is no discussion on why windows commands were used in the first place, but given the first reference above I guess it might still not be possible to avoid it.
Cheers Lex
Hey guys,
Le 05/02/2012 01:47, Lex Trotman a écrit :
Hi Colomban,
RE your discussion on IRC about $subject
To save you searching, some info about g_spawn not working (right) on windows, and that it won't be fixed:
http://article.gmane.org/gmane.editors.geany.devel/583
and it appears that it was always recognised that splitting on space wasn't an elegant way of generating argv for windows, and the problems that causes for filenames containing spaces, but nobody has been concerned enough to fix it :) see:
http://lists.uvena.de/geany/2007-November/002082.html
There is no discussion on why windows commands were used in the first place, but given the first reference above I guess it might still not be possible to avoid it.
Well, ok, thanks for the links. Actually my concern wasn't much the fact we don't use g_spawn* (though using them would've most likely addressed them) -- though of course it'd be good to have async spawn on Windows too. My concern was that the code we have (the one who calls CreateProcessW() & stuff) looked to me like it mishandled non-ASCII filenames -- and maybe even spaces.
So hum, huh... I haven't tested everything -- far from it -- but I still ended up with some changes, details ahead.
First, I've already pushed a few quite innocent things: [1] (code beauty), and [2] who helps to see WTF happened if creating run script failed.
Then, I have a few other changes, in the wip/windows-spawn-fixes of my GitHub fork: https://github.com/b4n/geany/commits/wip/windows-spawn-fixes The three last commits are relevant:
* da5480f68eba50308993820afb64ec1f8c4a175a - Fix win32_expand_environment_variables() and calling code encoding usage:
I think this one if fine, and actually fixes a few encoding misconversions in the spawn code. The only thing I know that may be wrong is that I removed the SearchPath() call (who at least should've been SearchPathW()) because AFAICT from the MSDN docs, CreateProcessW() already does the job for us (even better than we did). If we have something to do then it'd be NOT to search the path if we ain't got the G_SPAWN_SEARCH_PATH flag, though it's probably not much of an issue. The only problem I see is maybe the quoting of the arguments, but it's a mess already and shouldn't be worst with this change.
* 694087248edeb414e5ca12452e1a2bd40e0dc62f - Write geany_run_script.{sh,bat} in system locale encoding:
The commit message of this one almost says it all, but maybe it's not that good actually. I thing it is just fine, because I think the shell won't ever do magic for us -- and Windows's cmd.exe certainly doesn't -- so it's better to give it locale-encoded data. On most Linux boxes it wouldn't change anything since almost all use UTF8 nowadays, but maybe it'd even fix some problem on e.g. Linux with ISO-8859-* locale. But again, I'm not 100% sure it's right/the best fix.
* fec4c4730df3b9104f05616e7350368d4719f205 - On windows, set the cmd.exe codepage to the system locale:
Last but not least, something huh... let's say it works for me. The commit explains it, it adds a "chcp" call on top of geany_run_script.bat, passing it the system locale. The goal is to make cmd.exe understand the special characters in the script (of course it doesn't allow Unicode data @#!), so set it to use the local CP, hopefully the one we converted the script to (see previous commit).
This made my Windows XP VM box succeed at running scripts/programs named like "éç ßw", but not "éç ß↓" since the "↓" isn't available in Windows1252 [3].
So. The gain is that with these changes we can run programs with a limited set of special characters in their filename, which wasn't possible before, and the rest should work still the same. I haven't (yet?) tested spawning a build command with special chars in it, but it should probably work -- but maybe better do some tests.
I'm bored with that Windows stuff for now, so /pause. However, if anybody with (or without) Windows knowledge could test (Nick?), comment or whatever, it'd be awesome :)
Finally, take note I tested all this *ONLY* on Window XP Professional, I don't have any 2k/Vista/7/whatever box at hand, and I have no idea what works where.
OK, I'll stop for now, send the mail and take a rest about Windows stuff. Have fun ;)
Cheers, Colomban
[1] https://github.com/geany/geany/commit/ce21bdfb215f60c270817b641b52ba954303ce... [2] https://github.com/geany/geany/commit/0a22e8a624a095161dd3bdb958243be51510aa...
[3] though maybe it's a bad example if it isn't in the UTF-16 range, don't know
Cheers Lex _______________________________________________ Geany-devel mailing list Geany-devel@uvena.de https://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
On Mon, Feb 6, 2012 at 6:25 AM, Colomban Wendling lists.ban@herbesfolles.org wrote:
Hey guys,
Le 05/02/2012 01:47, Lex Trotman a écrit :
Hi Colomban,
RE your discussion on IRC about $subject
To save you searching, some info about g_spawn not working (right) on windows, and that it won't be fixed:
http://article.gmane.org/gmane.editors.geany.devel/583
and it appears that it was always recognised that splitting on space wasn't an elegant way of generating argv for windows, and the problems that causes for filenames containing spaces, but nobody has been concerned enough to fix it :) see:
http://lists.uvena.de/geany/2007-November/002082.html
There is no discussion on why windows commands were used in the first place, but given the first reference above I guess it might still not be possible to avoid it.
Well, ok, thanks for the links. Actually my concern wasn't much the fact we don't use g_spawn* (though using them would've most likely addressed them) -- though of course it'd be good to have async spawn on Windows too. My concern was that the code we have (the one who calls CreateProcessW() & stuff) looked to me like it mishandled non-ASCII filenames -- and maybe even spaces.
So hum, huh... I haven't tested everything -- far from it -- but I still ended up with some changes, details ahead.
First, I've already pushed a few quite innocent things: [1] (code beauty), and [2] who helps to see WTF happened if creating run script failed.
Then, I have a few other changes, in the wip/windows-spawn-fixes of my GitHub fork: https://github.com/b4n/geany/commits/wip/windows-spawn-fixes The three last commits are relevant:
Well, since my windows programming knowledge could be written on the head of a pin in crayon, with lots of space left over, I can't criticise or complement, but the ideas seem sensible.
- da5480f68eba50308993820afb64ec1f8c4a175a - Fix
win32_expand_environment_variables() and calling code encoding usage:
Environment variables are evil, but its a good idea to expand them when present.
I think this one if fine, and actually fixes a few encoding misconversions in the spawn code. The only thing I know that may be wrong is that I removed the SearchPath() call (who at least should've been SearchPathW()) because AFAICT from the MSDN docs, CreateProcessW() already does the job for us (even better than we did). If we have something to do then it'd be NOT to search the path if we ain't got the G_SPAWN_SEARCH_PATH flag, though it's probably not much of an issue. The only problem I see is maybe the quoting of the arguments, but it's a mess already and shouldn't be worst with this change.
- 694087248edeb414e5ca12452e1a2bd40e0dc62f - Write
geany_run_script.{sh,bat} in system locale encoding:
Yeah, its safer to convert the whole string even though only the filenames would actually be non-ascii, unless the "press return to continue" was localised. Perhaps now that might even be possible?
The commit message of this one almost says it all, but maybe it's not that good actually. I thing it is just fine, because I think the shell won't ever do magic for us -- and Windows's cmd.exe certainly doesn't -- so it's better to give it locale-encoded data. On most Linux boxes it wouldn't change anything since almost all use UTF8 nowadays, but maybe it'd even fix some problem on e.g. Linux with ISO-8859-* locale. But again, I'm not 100% sure it's right/the best fix.
- fec4c4730df3b9104f05616e7350368d4719f205 - On windows, set the cmd.exe
codepage to the system locale:
Last but not least, something huh... let's say it works for me. The commit explains it, it adds a "chcp" call on top of geany_run_script.bat, passing it the system locale. The goal is to make cmd.exe understand the special characters in the script (of course it doesn't allow Unicode data @#!), so set it to use the local CP, hopefully the one we converted the script to (see previous commit).
This made my Windows XP VM box succeed at running scripts/programs named like "éç ßw", but not "éç ß↓" since the "↓" isn't available in Windows1252 [3].
Does it work for two consecutive spaces?
So. The gain is that with these changes we can run programs with a limited set of special characters in their filename, which wasn't possible before, and the rest should work still the same. I haven't (yet?) tested spawning a build command with special chars in it, but it should probably work -- but maybe better do some tests.
Being a simple boring ascii guy I don't have any of them funny characters in *my* filenames to test with :)
I'm bored with that Windows stuff for now, so /pause. However, if anybody with (or without) Windows knowledge could test (Nick?), comment or whatever, it'd be awesome :)
Finally, take note I tested all this *ONLY* on Window XP Professional, I don't have any 2k/Vista/7/whatever box at hand, and I have no idea what works where.
OK, I'll stop for now, send the mail and take a rest about Windows stuff. Have fun ;)
Encodings and windows, you have *all* the fun.
Cheers Lex
Cheers, Colomban
[1] https://github.com/geany/geany/commit/ce21bdfb215f60c270817b641b52ba954303ce... [2] https://github.com/geany/geany/commit/0a22e8a624a095161dd3bdb958243be51510aa...
[3] though maybe it's a bad example if it isn't in the UTF-16 range, don't know
Cheers Lex _______________________________________________ Geany-devel mailing list Geany-devel@uvena.de https://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
Geany-devel mailing list Geany-devel@uvena.de https://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
Hi Colomban,
Pardon the serial post.
For windows, why do we split the command string in build_spawn_command pass it through two functions and glue it back together, thus opening it up to problems of quoting, multiple delimeters etc.
Since we only do the split in a #ifdef win32, why not just pass the whole comand as argv[0]? No changes would be needed (except removing the g_strsplit) so the functions in the API don't change, but probably utils_spawn_(a)sync should be documented to say that this will work on windows.
Cheers Lex