Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo

Close

#943 quotes and build command

v1.24
closed-fixed
v1.23
6
5 days ago
2013-03-12
gok6tm
No

Hi there !

I have a problem with custom build command since i upgrade to 1.23 on Wind~.
I define my build command as :
command : "C:\Documents and Settings\xxx\Application Data\geany\tools\jsl-0.3.0\jsl.exe" -process "%d\%f" -conf jsl.default.conf
work path : "C:\Documents and Settings\xxx\Application Data\geany\tools\jsl-0.3.0\"

And now with 1.23 it produce an error (in french) :
"C:\Documents and Settings\xxx\Application Data\geany\tools\jsl-0.3.0\jsl.exe" -process "D:\pathToFile\file.js" -conf jsl.default.conf (dans le dossier : "C:\Documents and Settings\xxx\Application Data\geany\tools\jsl-0.3.0\")
'C:\Documents' n'est pas reconnu en tant que commande interne
ou externe, un programme ex cutable ou un fichier de commandes.

If i use single quote instead of double quote i have this message (in french) :
Syntaxe du nom de fichier, de r pertoire ou de volume incorrecte.

I seen in the mailing list there was some changes near the management of quotes in the build command, maybe there is an edge effect for a Wind~ environment.

There is also an error with accentuated character ("ex cutable" / "r pertoire"). maybe it was present on previous version i don't remember

Thanks for your reading and consideration

Duplicates of this bug: [#969], [#980], [#996]

Related

Bugs: #969
Bugs: #980
Bugs: #996

Discussion

1 2 > >> (Page 1 of 2)
  • Lex Trotman
    Lex Trotman
    2013-03-13

    There has been a change to the way build commands are run on windows because the old way would fail if a lot of output was created. Now system() is used with temporary files instead of CreateChildProcess() with pipes.

    It appears this also requires a change in the quoting of commands when paths and filenames have spaces in them.

    Experimentation has shown (thanks CeruleanSky) that enclosing the whole command in two sets of double quotes and not using any other quotes will work even if paths and filenames contain multiple spaces. eg ""your command""

     
  • Lex Trotman
    Lex Trotman
    2013-03-13

    • status: open --> pending
     
  • gok6tm
    gok6tm
    2013-03-13

    Thanks for your comment.

    So i try with two sets of double quotes like :
    ""C:\Documents and Settings\xxx\Application Data\geany\tools\jsl-0.3.0\jsl.exe -process %d\%f -conf jsl.default.conf""
    No best result for me :
    ""C:\Documents and Settings\xxx\Application Data\geany\tools\jsl-0.3.0\jsl.exe -process D:\zzz\rightItemView.js -conf jsl.default.conf"" (dans le dossier : D:\zzz)
    Syntaxe du nom de fichier, de r pertoire ou de volume incorrecte.

     
    • Found in: --> v1.23
    • Fixed in: v1.23 --> None
     
  • Matthew Brush
    Matthew Brush
    2013-09-16

    • labels: Windows --> Windows, build command quote quoting split arguments compiler
    • Description has changed:

    Diff:

    --- old
    +++ new
    @@ -18,3 +18,5 @@
     There is also an error with accentuated character \("ex cutable" / "r pertoire"\). maybe it was present on previous version i don't remember
    
     Thanks for your reading and consideration
    +
    +Duplicates of this bug: [#969], [#980], [#996]
    
    • Priority: 5 --> 6
     
  • Matthew Brush
    Matthew Brush
    2013-09-16

    • labels: Windows, build command quote quoting split arguments compiler --> windows build command quote quoting split arguments compiler
     
  • Matthew Brush
    Matthew Brush
    2013-09-16

    • labels: windows build command quote quoting split arguments compiler --> windows build command quote quoting split arguments compiler, windows, build, command, quote, quoting, split, arguments, compiler
     
  • Matthew Brush
    Matthew Brush
    2013-09-16

    • labels: windows build command quote quoting split arguments compiler, windows, build, command, quote, quoting, split, arguments, compiler --> windows, build, command, quote, quoting, split, arguments, compiler
     
  • Matthew Brush
    Matthew Brush
    2013-09-17

     
  • Matthew Brush
    Matthew Brush
    2013-09-17

    The regression might have been introduced in a3664fae9ece396952d732cc937e63192d8c6f76. I just had a quick scan of the history, but it looks possible.

    @ntrel If it is this commit, do you mind reverting it until we can come up with a proper fix for the original problem? I wonder if it could just use GLib async spawning like the other platforms. Later releases of GLib might have the kinks worked out by now.

     
  • Lex Trotman
    Lex Trotman
    2013-09-17

    @ntrel, or could you document (or point to MS documents) how to avoid the problems of these bug reports.

     
  • Nick Treleaven
    Nick Treleaven
    2013-10-16

    Fix:
    https://github.com/geany/geany/pull/180

    Matt, Lex: just for the record, using @ntrel doesn't notify me in any way, I don't think sf.net supports that. Just email me next time ;-)

     
  • Matthew Brush
    Matthew Brush
    2013-10-17

    I did email you, twice even :) First by bug mail and then directly. The @ntrel notation is just habit from Github for me at least.

     
    • Nick Treleaven
      Nick Treleaven
      2013-10-17

      Yes, thanks Matt. Also it might have been more noticeable if the bug title mentioned windows.

       
  • Dimitar Zhekov
    Dimitar Zhekov
    2013-10-23

    Here is what I believe to be The fix for Windows pipe blocking, known as "broken async spawning", as discussed in ML "Ping on Bug #943 - windows build command". Please test, and if it works, I'll remove build.c SYNC_SPAWN and upload a proper git patch.

     
    Attachments
  • Nick Treleaven
    Nick Treleaven
    2013-10-25

    Dimitar: I think custom async spawning needs longer testing before a release. I suggest we use my fix to sync spawning for the next release, and commit your async spawning to git for testing. Haven't tried it yet.

     
    • Dimitar Zhekov
      Dimitar Zhekov
      2013-11-02

      Nick, it seems that half measures won't work, unless I create a super ugly fix (and nblock.diff already isn't very pretty). Clearing the whole spawning mess in all the modules will take time, so if your fix is not included, please apply it.

       
  • Matthew Brush
    Matthew Brush
    2013-10-25

    Nick, the system() stuff obviously hadn't gotten much testing before merging. Also it fixes a problem only one person reported (actually you even didn't open a bug report).

    Even if the quoting stuff is "fixed", it still has the issues of completely locking up Geany, opening a separate command-line prompt window, using temp files, using temp files insecurely, potentially leaking temp files, changing Geany's working directory without changing it back, using brittle hand-rolled command-line quoting, renaming a function broken_() without removing it (if it's broken, remove it), and still using the broken_() function when the environment vector is NULL without any explanation why it's not broken when an environment vector is passed.

    I haven't reviewed or tested Dimitar's changes in detail, but they simply must be better than the system() "fix". I'd personally feel more comfortable using code that tries to "do it right" than the current workaround. That being said, I infrequently use Geany on Windows, so I'm not affected too much either way, I'm speaking strictly from a code quality POV. Unless we're going to release in the very immediate future, I think it'd be great to get Dimitar's changes in a pull request for review and integrating into Geany for testing.

    My two cents.

     
    • Nick Treleaven
      Nick Treleaven
      2013-10-25

      Nick, the system() stuff obviously hadn't gotten much testing before merging. Also it fixes a problem only one person reported (actually you even didn't open a bug report).

      Matt, the tone of the above makes it unnecessarily difficult for me to respond. IMO the quoted part was completely unnecessary and is uncalled for. You can make your point without personal attacks. I had a very good reason to make that commit.

      I'm happy for Dimitar's code to go into git master.

       
  • Dimitar Zhekov
    Dimitar Zhekov
    2013-10-25

    I expected at least whoever filled/pinged #943 to test the fix. Maybe we should signal him, and the fillers of #969, #980 and #996?

    BTW, "Format -> Send Selection to" may be tested in *nix as well. The changes there are actually larger than the win~1 piping/polling.

     
  • Nick Treleaven
    Nick Treleaven
    2013-10-30

    Dimitar: Thanks for your work. I've just tried the patch, and unfortunately I get the same issue as with git master without SYNC_SPAWN defined. I.e. when I run Make Object, the errors are not shown in the message window. Also async spawn on Windows has the same issue as this bug report (quoted spaces are not preserved as one argument), so we will need some kind of fix to build.c similar to my PR even when using GLib async spawning.

     
  • Dimitar Zhekov
    Dimitar Zhekov
    2013-10-30

    The quoting problem is because Geany does g_strsplit(cmd_string, " ", 0) under win32, irrespective of whether SYNC_SPAWN is used. It's not a spawn problem, all spawn/exec family use argv[] in the first place. A possible solution would be replacing both g_strsplit() [win32] and "/bin/sh" "-c" ... assignments [*nix] in build_spawn_cmd() with g_shell_parse_argv(), as in tools.c:tools_execute_custom_command().

    I'll try Make Object tomorrow. Both FiF and tools returned errors in my tests, so it's interesting when/why build does not. When finished, I'll post an updated patch with g_shell_parse_argv() and Make Object fix if one is needed/possible/whatever.

     
  • Dimitar Zhekov
    Dimitar Zhekov
    2013-10-31

    OK, I tested build under win~1, and the results are not rosy.

    g_shell_parse_argv() works, but feels a bit awkward: ' also quotes, and \ is an escape character, so you need \\ or / for directory separator. The advantage is that we can pass double quotes to the command, which is impossible with the regular win~1 syntax AFAIK (grep text-with-" under cmd is a PITA). Of course, I can write a small utils_parse_argv() function which calls g_shell_parse_argv() under *nix and implements the win~1 parsing, no big deal. Comments?

    As of the missing Make Object error messages:

    make object ::= cc -o build.o build.c

    [blue] cc -o build.o build.c (in directory: C:\tmp\scp\geany-2013-06-11\src)
    [blue] Compilation failed.
    [light red] In file included from build.c:31:0:
    [light red] geany.h:28:21: fatal error: gtk/gtk.h: No such file or directory
    [dark red] compilation terminated.

    make object ::= make build.o

    [blue] make build.o (in directory: C:\tmp\scp\geany-2013-06-11\src)
    [black] cc -c -o build.o build.c
    [blue] Compilation failed.
    [dark red] make: *** [build.o] Error 1

    make build.o from cmd.exe CL:

    cc -c -o build.o build.c
    In file included from build.c:31:0:
    geany.h:28:21: fatal error: gtk/gtk.h: No such file or directory
    compilation terminated.
    make: *** [build.o] Error 1

    What we see here is that Geany properly captures the error messages from make, which appear after the gcc error messages. So it's not a matter of buggy client reading from stderr. The reason we don't receive the subprocess messages may be (a) the glib execution helper program, or (b) GNU make trying to be too smart, and running the commands with cmd/COMSPEC if needed (it even recognizes 4NT), or without cmd/COMSPEC if not needed, or with sh/SHELL if one exists. It's horribly broken and misses more often than hits. Or (c) we may need to specify G_SPAWN_LEAVE_DESCRIPTORS_OPEN - if that does'nt work, there will be no easy solution...

     
  • Dimitar Zhekov
    Dimitar Zhekov
    2013-11-01

    As expected, G_SPAWN_LEAVE_DESCRIPTORS_OPEN did nothing.
    The loss of stderr is does not depend on the program: when Geany start X, and it starts Y, the output of Y is lost, but cmd /c X >& output.txt captures all stdout/err from X and Y.

    Why glib uses a helper program to start programs under win~1 is described at http://lists-archives.com/gtk/02371-regarding-helper-process-in-glib-2-8-4.html

    The funny thins is that CreateProcess() supports working dir... Considering that some of our spawns need command line (tools, build), others argument vector (utils_spawn_[a]sync), and yet others both (grep), I plan to write something like:

    gboolean utils_start_async_with_pipes(
        const gchar *command_text,
        const gchar *working_directory,
        gchar **argv,
        gchar **envp,
        GPid *child_pid,
        gint *standard_input,
        gint *standard_output,
        gint *standard_error,
        GError **error
    );
    

    where either command_text or argv may be NULL, and both may be non-null. The *nix implementation will call g_shell_parse_argv(command_text) and g_spawn_async_with_pipes(), under win~1 it'll create a full command line and call CreateProcess() directly (our win32.c contains some of the required code). If that works, I can write a proper win32_spawn with async support, and end the spawning saga for good.

     
    Last edit: Matthew Brush 2013-11-02
    • Lex Trotman
      Lex Trotman
      2013-11-02

      Do we need the argv option? IIRC all Geany uses start with a command string, so it would be better to have an operating system dependent breakup as part of the utils function if its needed, rather than encouraging Geany and plugin code to do incorrect hacks to break the command string up.

      PS I assume the gchars are pointers but that SF formatting ate the stars

       
1 2 > >> (Page 1 of 2)