Hi All,
I just noticed that the new spawn code exposes almost every single bit of API possible. Do we really want to do that, or should we limit it only to what is currently needed by any plugins? A quick survey of Geany-Plugins shows no usage of any of this yet.
IMO, we shouldn't expose anything which is not needed by plugins, especially if it's not related to the plugin API.
Cheers, Matthew Brush
On 2015-06-20 08:12 PM, Matthew Brush wrote:
Hi All,
I just noticed that the new spawn code exposes almost every single bit of API possible. Do we really want to do that, or should we limit it only to what is currently needed by any plugins? A quick survey of Geany-Plugins shows no usage of any of this yet.
IMO, we shouldn't expose anything which is not needed by plugins, especially if it's not related to the plugin API.
See: https://github.com/geany/geany/pull/523
Cheers, Matthew Brush
On 21.6.2015 г. 06:12, Matthew Brush wrote:
I just noticed that the new spawn code exposes almost every single bit of API possible. Do we really want to do that, or should we limit it only to what is currently needed by any plugins? A quick survey of Geany-Plugins shows no usage of any of this yet.
IMO, we shouldn't expose anything which is not needed by plugins, especially if it's not related to the plugin API.
The API is designed not only to ease/fix the spawn pipe I/O, but as a possible replacement for all glib spawn functions - and these may be invoked by any plugin. (There is no replacement for g_spawn_close_pid, which works fine, and for g_spawn_check_exit_status, because it's 2.34+ only - instead, the WIF* macros are defined system-independently.)
Spawn neither requires not uses anything from Geany (except i18n), and does not change anything in Geany state, so it's functions are not strictly "Geany" - it could be a separate library just as well.
In the next plugins (after this release), Scope will require at least WIF*, spawn_kill_process, SpawnFlags, SpawnReadFunc, spawn_with_callbacks, SpawnWriteData and spawn_write_data. It'll be good if 'debugger' uses them as well...
-- E-gards: Jimmy
On 2015-06-21 04:25 AM, Dimitar Zhekov wrote:
On 21.6.2015 г. 06:12, Matthew Brush wrote:
I just noticed that the new spawn code exposes almost every single bit of API possible. Do we really want to do that, or should we limit it only to what is currently needed by any plugins? A quick survey of Geany-Plugins shows no usage of any of this yet.
IMO, we shouldn't expose anything which is not needed by plugins, especially if it's not related to the plugin API.
The API is designed not only to ease/fix the spawn pipe I/O, but as a possible replacement for all glib spawn functions - and these may be invoked by any plugin. (There is no replacement for g_spawn_close_pid, which works fine, and for g_spawn_check_exit_status, because it's 2.34+ only - instead, the WIF* macros are defined system-independently.)
Spawn neither requires not uses anything from Geany (except i18n), and does not change anything in Geany state, so it's functions are not strictly "Geany" - it could be a separate library just as well.
Yeah, it almost should be a "separate" library (at least like TM or MIO). Personally I don't like the tendency of Geany's plugin API to be getting bloated with stuff that has nothing to do with Geany or plugins, but rather general purpose stuff making up for shortcomings in GLib, or general-purpose convenience functions. But I think maybe this is a conversation for another day.
In the next plugins (after this release), Scope will require at least WIF*, spawn_kill_process, SpawnFlags, SpawnReadFunc, spawn_with_callbacks, SpawnWriteData and spawn_write_data. It'll be good if 'debugger' uses them as well...
I figured there would be some use in at least Scope. Do you have any problem if we make all the API private for now and then as you need it in Scope (or any other plugins requesting it) we can export precisely what is needed, at that time?
Cheers, Matthew Brush
On 21.6.2015 г. 20:39, Matthew Brush wrote:
On 2015-06-21 04:25 AM, Dimitar Zhekov wrote:
On 21.6.2015 г. 06:12, Matthew Brush wrote:
[...]
Spawn neither requires not uses anything from Geany (except i18n), and does not change anything in Geany state, so it's functions are not
Yeah, it almost should be a "separate" library (at least like TM or MIO). Personally I don't like the tendency of Geany's plugin API to be getting bloated with stuff that has nothing to do with Geany or plugins, but rather general purpose stuff making up for shortcomings in GLib, or general-purpose convenience functions. But I think maybe this is a conversation for another day.
+1 on that. Any generic purpose functions, not really dependent on Geany, should be separated in a library, and used by Geany and the plugins on equal basic. Perhaps in a next version...
Some reasons for the plugins to use the spawn API, in no particular order:
- supports native CL under Win~1 (not -is-escape *nix parsing) - does not spawn helper processes (creating a process is costly under Win~1, though that's only notable for > several) - handles locale arguments better (the former FiF locale error) - may provide better security, depending on how glib starts processes
But I will not try to force the API on anybody, and do not want anyone else to do so. If the reasons are not compelling, so be it.
In the next plugins (after this release), Scope will require at least WIF*, spawn_kill_process, SpawnFlags, SpawnReadFunc, spawn_with_callbacks, SpawnWriteData and spawn_write_data. It'll be good if 'debugger' uses them as well...
I figured there would be some use in at least Scope. Do you have any problem if we make all the API private for now and then as you need it in Scope (or any other plugins requesting it) we can export precisely what is needed, at that time?
I want the next Scope to depend on Geany-1.25 stable-release. If the functions are hidden now, it'll have to depend on Geany-git-date, which I'd rather avoid...
[A note to myself: the README for this release should state that Scope depends on glib-2.18 now, not only Geany-1.22.]
-- E-gards: Jimmy
On 2015-06-22 10:18 AM, Dimitar Zhekov wrote:
On 21.6.2015 г. 20:39, Matthew Brush wrote:
On 2015-06-21 04:25 AM, Dimitar Zhekov wrote:
On 21.6.2015 г. 06:12, Matthew Brush wrote:
[...]
Spawn neither requires not uses anything from Geany (except i18n), and does not change anything in Geany state, so it's functions are not
Yeah, it almost should be a "separate" library (at least like TM or MIO). Personally I don't like the tendency of Geany's plugin API to be getting bloated with stuff that has nothing to do with Geany or plugins, but rather general purpose stuff making up for shortcomings in GLib, or general-purpose convenience functions. But I think maybe this is a conversation for another day.
+1 on that. Any generic purpose functions, not really dependent on Geany, should be separated in a library, and used by Geany and the plugins on equal basic. Perhaps in a next version...
Some reasons for the plugins to use the spawn API, in no particular order:
- supports native CL under Win~1 (not -is-escape *nix parsing)
- does not spawn helper processes (creating a process is costly under
Win~1, though that's only notable for > several)
- handles locale arguments better (the former FiF locale error)
- may provide better security, depending on how glib starts processes
But I will not try to force the API on anybody, and do not want anyone else to do so. If the reasons are not compelling, so be it.
I might try it with the code-format plugin eventually as it's currently incredibly slow to do real-time formatting on Windows, because it spawns clang-format _a lot_.
In the next plugins (after this release), Scope will require at least WIF*, spawn_kill_process, SpawnFlags, SpawnReadFunc, spawn_with_callbacks, SpawnWriteData and spawn_write_data. It'll be good if 'debugger' uses them as well...
I figured there would be some use in at least Scope. Do you have any problem if we make all the API private for now and then as you need it in Scope (or any other plugins requesting it) we can export precisely what is needed, at that time?
I want the next Scope to depend on Geany-1.25 stable-release. If the functions are hidden now, it'll have to depend on Geany-git-date, which I'd rather avoid...
We should make Colomban decide :) I get what you're saying but I also feel uneasy about blanket exporting of APIs with no current users of it, so we don't know exactly what really needs to be exported.
Cheers, Matthew Brush
On 23.6.2015 г. 02:25, Matthew Brush wrote:
[...]
One thing I forgot: the plugin API currently exports utils_spawn_[a]sync, and a few plugins use utils_spawn_sync. These functions were (partially correct) wrappers around the old glib/win32 spawn, and are now wrappers around spawn_[a]sync. Someday, in the distant future, they should be obsoleted...
I get what you're saying but I also feel uneasy about blanket exporting of APIs with no current users of it, so we don't know exactly what really needs to be exported.
Well, with nothing from spawn exported, we can be pretty sure that all plugins _will_ be using utils_spawn and g_spawn instead. :)
We should make Colomban decide :)
The leading developers should decide - including you.
-- E-gards: Jimmy
On 24 June 2015 at 02:04, Dimitar Zhekov dimitar.zhekov@gmail.com wrote:
On 23.6.2015 г. 02:25, Matthew Brush wrote:
[...]
One thing I forgot: the plugin API currently exports utils_spawn_[a]sync, and a few plugins use utils_spawn_sync. These functions were (partially correct) wrappers around the old glib/win32 spawn, and are now wrappers around spawn_[a]sync. Someday, in the distant future, they should be obsoleted...
I get what you're saying but I also feel uneasy about blanket exporting of APIs with no current users of it, so we don't know exactly what really needs to be exported.
In a previous post Dimitar has listed the API he requests for his plugin, so that (at least) should be made available in 1.25. It is spurious to make him and therefore any potential windows users wait for another release cycle.
Well, with nothing from spawn exported, we can be pretty sure that all plugins _will_ be using utils_spawn and g_spawn instead. :)
Yes, the argument that nothing uses it is also spurious given the API only just came into existence.
Cheers Lex
We should make Colomban decide :)
The leading developers should decide - including you.
-- E-gards: Jimmy _______________________________________________ Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
On 2015-06-23 01:40 PM, Lex Trotman wrote:
On 24 June 2015 at 02:04, Dimitar Zhekov dimitar.zhekov@gmail.com wrote:
On 23.6.2015 г. 02:25, Matthew Brush wrote:
[...]
One thing I forgot: the plugin API currently exports utils_spawn_[a]sync, and a few plugins use utils_spawn_sync. These functions were (partially correct) wrappers around the old glib/win32 spawn, and are now wrappers around spawn_[a]sync. Someday, in the distant future, they should be obsoleted...
I get what you're saying but I also feel uneasy about blanket exporting of APIs with no current users of it, so we don't know exactly what really needs to be exported.
In a previous post Dimitar has listed the API he requests for his plugin, so that (at least) should be made available in 1.25. It is spurious to make him and therefore any potential windows users wait for another release cycle.
He listed some of the API that is exposed. There's nothing spurious about being cautious about mass-exporting API that nobody even asks for, IMO.
Well, with nothing from spawn exported, we can be pretty sure that all plugins _will_ be using utils_spawn and g_spawn instead. :)
Yes, the argument that nothing uses it is also spurious given the API only just came into existence.
Nobody made any argument that nobody uses, I just mentioned that nobody is using it YET (since they could've, as it's all exported for plugins ATM in Git master), so we could therefore unexpose it without causing anybody grief. There's nothing spurious going on here.
Cheers, Matthew Brush
On 2015-06-23 09:04 AM, Dimitar Zhekov wrote:
On 23.6.2015 г. 02:25, Matthew Brush wrote:
[...]
One thing I forgot: the plugin API currently exports utils_spawn_[a]sync, and a few plugins use utils_spawn_sync. These functions were (partially correct) wrappers around the old glib/win32 spawn, and are now wrappers around spawn_[a]sync. Someday, in the distant future, they should be obsoleted...
+1
I get what you're saying but I also feel uneasy about blanket exporting of APIs with no current users of it, so we don't know exactly what really needs to be exported.
Well, with nothing from spawn exported, we can be pretty sure that all plugins _will_ be using utils_spawn and g_spawn instead. :)
I think the general policy is to export stuff on demand as plugins need it. Seeing as you wrote the API in question, I'm assuming you know best the stuff you will need, so I don't personally see much problem preemptively exposing that stuff[0].
From you're previous email you mentioned the stuff checked with [x] below:
[ ] spawn_get_program_name() [ ] spawn_check_command() [x] spawn_kill_process() [ ] spawn_async_with_pipes() [ ] spawn_async() [x] spawn_with_callbacks() [x] spawn_write_data() [ ] spawn_get_exit_status() [ ] spawn_sync()
[x] SpawnFlags [x] SpawnReadFunc [x] SpawnWriteData
Is that sufficient, or is there other stuff? I will remove the /** from anything that is not expected to be needed, if nobody objects.
About the WIF* macros, those (at present) are not "officially" part of the plugin API as they have no Doxygen comments, though they will still be visible to plugins since they're macros and bypass the linker trick we use to hide functions. I think it would be best to add documentation to those (and possible define them into the "GEANY_" 'namespace'?), at the very least to ensure nobody accidentally moves or hides them. Just the other day I tried to move them into spawn.c thinking they were there on accident, but then I left them because another .c file uses them so they need to be in a (not necessarily public) header.
We should make Colomban decide :)
The leading developers should decide - including you.
Well you know my opinion :) I think everyone pretty much agrees we shouldn't expose stuff unrelated to the plugin API, and I think we all also agree it's stupid for plugins to have to copy&paste code that exists already or else use an inferior version. I also think everyone agrees that a separate utility library would be a good idea. The problem I have is that once this API makes it into the next release, it's "forever" stuck inside Geany and we'll have to keep it indefinitely, regardless if something possibly better[1] like GProcess comes available to us, and we don't even use it internally anymore.
Cheers, Matthew Brush
[0]: At least until we establish a policy around what belongs in the plugin API or not. [1]: I have no idea if it's better, just an example.
Le 24/06/2015 00:18, Matthew Brush a écrit :
[…]
I think the general policy is to export stuff on demand as plugins need it. Seeing as you wrote the API in question, I'm assuming you know best the stuff you will need, so I don't personally see much problem preemptively exposing that stuff[0].
From you're previous email you mentioned the stuff checked with [x] below:
[ ] spawn_get_program_name() [ ] spawn_check_command() [x] spawn_kill_process() [ ] spawn_async_with_pipes() [ ] spawn_async() [x] spawn_with_callbacks() [x] spawn_write_data() [ ] spawn_get_exit_status() [ ] spawn_sync()
[x] SpawnFlags [x] SpawnReadFunc [x] SpawnWriteData
Seems to make sense, only spawn_write_data() doesn't strike me as totally required, but I understand. TLDR version below.
Is that sufficient, or is there other stuff? I will remove the /** from anything that is not expected to be needed, if nobody objects.
IMO, spawn_with_callbacks() is *the* key API from spawn, what makes it great.
spawn_async_with_pipes() can be nice, but most people probably shouldn't use them as they have same IO trickery most IO layers have, where you have to take care of not filling up any pipes (write) or forget to empty them (read). The only real difference wit g_spawn_async*() is that it uses native API on Windows (which indeed solves some issues so is useful, but that's not my point). Inside Geany, we can rely on the GLib main loop to be running so anyone can use the handier spawn_with_callbacks().
BTW, I just noticed that on Windows spawn_async_with_pipes() requires the GLib main loop to be running if child_pid=NULL (well, it registers a watch func, not sure what happens if it doesn't execute -- zombie?). We probably don't care much though.
spawn_async() doesn't seem so much useful to me, as it doesn't (seem) to have so much advantages over g_spawn_async(), which works okay (as there is no pipes involved). Also, it's a thin wrapper around spawn_async_with_pipes(). I don't know about speed or anything though.
spawn_sync() is nice because it allows to easily pipe through a process (send some data to stdin and and read the output). How often is this useful for everyone I don't know -- but any plugin wanting to call a filter command might benefit from it. Also, it's not that hard to reproduce using spawn_with_callbacks() as that one has SPAWN_SYNC, so only the communication callbacks have to be implemented.
spawn_get_exit_status_cb() seem useless to the API IMO. It's a trivial function currently only used by spawn itself. It might even be sensible to make it completely internal.
I would have said that spawn_write_data() wasn't really required because it's relatively simple and not used by Geany outside of spawn, but it's indeed probably handy in combination with spawn_with_callbacks() to anyone wanting to feed a simple buffer to stdin.
spawn_get_program_name() is only used in spawn itself and doesn't seem to be a strict requirement.
spawn_check_command() seem nice to validate a user command before actually running it, so it might be useful to anyone wanting to run user-supplied commands through the spawn API, to e.g. check for issues right when the user defines the command (like how we do it in the custom commands dialog).
We should make Colomban decide :)
The leading developers should decide - including you.
Well you know my opinion :) I think everyone pretty much agrees we shouldn't expose stuff unrelated to the plugin API, and I think we all also agree it's stupid for plugins to have to copy&paste code that exists already or else use an inferior version. I also think everyone agrees that a separate utility library would be a good idea.
I'm everyone and all :)
Regards, Colomban
Colomban,
Correct me if I'm wrong, but despite my loudly voiced misgivings :) doesn't the spawn_* series do command quoting and g_spawn* not?
If that the case they should not be mixed on the same platform otherwise the user has to know which commands are run with which function to know if they need to do the quoting themselves.
Cheers Lex
Le 24/06/2015 01:57, Lex Trotman a écrit :
Colomban,
Correct me if I'm wrong, but despite my loudly voiced misgivings :) doesn't the spawn_* series do command quoting and g_spawn* not?
If that the case they should not be mixed on the same platform otherwise the user has to know which commands are run with which function to know if they need to do the quoting themselves.
Well, those support an additional "command" parameter that indeed is read in a platform-dependent manner. (this was a discussion point and ended like this).
They however also support argvs just fine, so you can use those too and they would work the same. With this, you can also imagine using g_shell_parse_argv() on all platforms if you like, just like you would do with g_spawn().
So well, yes, the user has to know which syntax is needed, but that's basically true already.
Cheers, Colomban
On 24 June 2015 at 10:06, Colomban Wendling lists.ban@herbesfolles.org wrote:
Le 24/06/2015 01:57, Lex Trotman a écrit :
Colomban,
Correct me if I'm wrong, but despite my loudly voiced misgivings :) doesn't the spawn_* series do command quoting and g_spawn* not?
If that the case they should not be mixed on the same platform otherwise the user has to know which commands are run with which function to know if they need to do the quoting themselves.
Well, those support an additional "command" parameter that indeed is read in a platform-dependent manner. (this was a discussion point and ended like this).
They however also support argvs just fine, so you can use those too and they would work the same. With this, you can also imagine using g_shell_parse_argv() on all platforms if you like, just like you would do with g_spawn().
Then Geany should be changed to do that too, for all commands (IIRC build commands already do it).
Note by "user" I mean the end user, not the plugin programmer. Having the end user need to know if they should quote commands or not is bad (tm). If that is already the case then it should be fixed.
Cheers Lex
So well, yes, the user has to know which syntax is needed, but that's basically true already.
Cheers, Colomban _______________________________________________ Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
On 24.6.2015 г. 03:12, Lex Trotman wrote:
On 24 June 2015 at 10:06, Colomban Wendlinglists.ban@herbesfolles.org wrote:
Le 24/06/2015 01:57, Lex Trotman a écrit :
Colomban,
Correct me if I'm wrong, but despite my loudly voiced misgivings :) doesn't the spawn_* series do command quoting and g_spawn* not?
No. It uses quoting only internally, (a) when creating a Windows command line from argv, for obvious reasons, and (b) if your Windows CL program name contains spaces, to make sure that you won't run "C:\Program.exe" or "C:\Program Files\Foo.exe" if your "C:\Program Files\Foo\Bar.exe" does not exist. Yes, CreateProcess() will really try them, if you miss the quotes by mistake.
[g_]spawn*() via argv produce identical results.
Well, those support an additional "command" parameter that indeed is read in a platform-dependent manner. (this was a discussion point and ended like this).
They however also support argvs just fine, so you can use those too and they would work the same. With this, you can also imagine using g_shell_parse_argv() on all platforms if you like, just like you would do with g_spawn().
Then Geany should be changed to do that too, for all commands (IIRC build commands already do it).
Unless one is using \ as escape character or ' for quoting under Windows, the commands will work. The default filetypes always contain "%x" and never '%x', so they will work.
Note by "user" I mean the end user, not the plugin programmer. Having the end user need to know if they should quote commands or not is bad (tm). If that is already the case then it should be fixed.
They only need to quote a %x which may contain spaces, as usual, using double quotes, as in the default filetypes. The documentation examples also show double quotes, and unless I'm not missing something, we don't guarantee that \ and ' will work under Windows like in Unix.
The new spawn is more Windows friendly, as it'll never treat the Windows directory separator \ as an escape character, and I think that will benefit the end users. The forward slashes will work as well (they work since MS-DOS 5.0).
-- E-gards: Jimmy
Hi Dimitar,
I'm now totally confused, so let me ask the important question directly:
A user is entering a command into Geany, if the command is going to be run by spawn_* will they enter it the same way as they would if the command is going to be run by g_spawn*? In all versions, sync/async/pipes or not.
Colomban suggested that plugins can use g_spawn* for some commands because they "work" for both Windows and Linux, and spawn_* for others, but this is a bad idea if commands need to be entered differently, since an end user then needs to know which commands to enter in which way.
So If the answer is they are different then we need to make Geany (and any spawning plugins with user entered commands) use all the same commands on the platform, all spawn_* on Windows and all g_spawn* on Linux.
Cheers Lex
On 26 June 2015 at 03:13, Dimitar Zhekov dimitar.zhekov@gmail.com wrote:
On 24.6.2015 г. 03:12, Lex Trotman wrote:
On 24 June 2015 at 10:06, Colomban Wendlinglists.ban@herbesfolles.org wrote:
Le 24/06/2015 01:57, Lex Trotman a écrit :
Colomban,
Correct me if I'm wrong, but despite my loudly voiced misgivings :) doesn't the spawn_* series do command quoting and g_spawn* not?
No. It uses quoting only internally, (a) when creating a Windows command line from argv, for obvious reasons, and (b) if your Windows CL program name contains spaces, to make sure that you won't run "C:\Program.exe" or "C:\Program Files\Foo.exe" if your "C:\Program Files\Foo\Bar.exe" does not exist. Yes, CreateProcess() will really try them, if you miss the quotes by mistake.
[g_]spawn*() via argv produce identical results.
Well, those support an additional "command" parameter that indeed is read in a platform-dependent manner. (this was a discussion point and ended like this).
They however also support argvs just fine, so you can use those too and they would work the same. With this, you can also imagine using g_shell_parse_argv() on all platforms if you like, just like you would do with g_spawn().
Then Geany should be changed to do that too, for all commands (IIRC build commands already do it).
Unless one is using \ as escape character or ' for quoting under Windows, the commands will work. The default filetypes always contain "%x" and never '%x', so they will work.
Note by "user" I mean the end user, not the plugin programmer. Having the end user need to know if they should quote commands or not is bad (tm). If that is already the case then it should be fixed.
They only need to quote a %x which may contain spaces, as usual, using double quotes, as in the default filetypes. The documentation examples also show double quotes, and unless I'm not missing something, we don't guarantee that \ and ' will work under Windows like in Unix.
The new spawn is more Windows friendly, as it'll never treat the Windows directory separator \ as an escape character, and I think that will benefit the end users. The forward slashes will work as well (they work since MS-DOS 5.0).
-- E-gards: Jimmy
Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
On 26.6.2015 г. 01:47, Lex Trotman wrote:
Hi Dimitar,
Hi, Lex.
On 26 June 2015 at 03:13, Dimitar Zhekovdimitar.zhekov@gmail.com wrote:
It uses quoting internally [...bla-bla-bla unneeded tech details...]
I'm now totally confused, so let me ask the important question directly:
I should have been more terse, sorry. :)
A user is entering a command into Geany, if the command is going to be run by spawn_* will they enter it the same way as they would if the command is going to be run by g_spawn*? In all versions, sync/async/pipes or not.
Short answer: yes.
(Long answer: spawning under Unix and Windows was never exactly the same. When splitting a command line into an argument vector with g_shell_parse_argv(), and then running it with g_spawn(), or using g_spawn('/bin/sh', command), which are the standard practices under Unix, \ will work as escape character, and ' as argument quoter, as expected under *nix. That doesn't work under Windows, didn't work in 1.23/1.24 either, and I haven't checked any earlier versions.)
-- E-gards: Jimmy
On 27 June 2015 at 03:13, Dimitar Zhekov dimitar.zhekov@gmail.com wrote:
On 26.6.2015 г. 01:47, Lex Trotman wrote:
Hi Dimitar,
Hi, Lex.
On 26 June 2015 at 03:13, Dimitar Zhekovdimitar.zhekov@gmail.com wrote:
It uses quoting internally [...bla-bla-bla unneeded tech details...]
I'm now totally confused, so let me ask the important question directly:
I should have been more terse, sorry. :)
Thats ok, maybe I havn't been totally clear about where I see the "problem" and its impact on the exposed API.
A user is entering a command into Geany, if the command is going to be run by spawn_* will they enter it the same way as they would if the command is going to be run by g_spawn*? In all versions, sync/async/pipes or not.
Short answer: yes.
(Long answer: spawning under Unix and Windows was never exactly the same. When splitting a command line into an argument vector with g_shell_parse_argv(), and then running it with g_spawn(), or using g_spawn('/bin/sh', command), which are the standard practices under Unix, \ will work as escape character, and ' as argument quoter, as expected under *nix. That doesn't work under Windows, didn't work in 1.23/1.24 either, and I haven't checked any earlier versions.)
Your long answer contradicts your short answer by illustrating some places where they differ. :)
My concern is that, for a given platform, all commands a user enters into Geany or plugins should use the same meta chars and quoting. My concern is that a user should not be faced with the problem that some commands have to be entered with windows rules and some with unix rules.
Many plugins currently do their own command parsing to get an argv and others use g_spawn_command (which is hard wired to unix rules, it uses g_shell_parse_argv). On windows these plugins will now use different rules to the build commands because the build commands use spawn_* and get platform dependent rules.
These plugins should be encouraged to switch to the same rules as Geany, ie to use spawn_* calls with a cmd not an argv. That means the appropriate spawn_* API should be available.
The API we need to expose for this is the API that allows an unparsed command to be used, not versions that only allow argv.
So the spawn API that is a drop in replacement for g_spawn is only needed where g_spawn doesn't work, and then it should only be used as a temporary workaround until the plugins move to using the cmd version.
So to me that means:
/* platform dependent checking */
spawn_get_program_name spawn_check_command
/* platform dependent running */
spawn_with_callbacks spawn_async spawn_sync
/* associated support functions */
spawn_kill_process spawn_write_data spawn_get_exit_status_callback
Cheers Lex
-- E-gards: Jimmy _______________________________________________ Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
On 27.6.2015 г. 04:40, Lex Trotman wrote:
My concern is that, for a given platform, all commands a user enters into Geany or plugins should use the same meta chars and quoting. [...]
Well, after we could not agree on universal syntax, the next best thing would be to at least have consistent syntax on each platform.
Many plugins currently do their own command parsing to get an argv and others use g_spawn_command (which is hard wired to unix rules, it uses g_shell_parse_argv). On windows these plugins will now use different rules to the build commands because the build commands use spawn_* and get platform dependent rules.
They will, though not that different. Most important when specifying a windows command with the *nix rules is you need to use either \ or / as directory separator. Both will work, Win~1 supports that.
These plugins should be encouraged to switch to the same rules as Geany, ie to use spawn_* calls with a cmd not an argv. That means the appropriate spawn_* API should be available.
As of me, they should be encouraged to use the native Windows rules for the benefit of the users. Unless we design Geany for *nix users which run Win~1 from time to time. :)
(I assume be "cmd" you mean "command line", not cmd.exe).
The API we need to expose for this is the API that allows an unparsed command to be used, not versions that only allow argv.
All spawn calls support both a CL and an argv. If both are passed, argv is appended to CL.
So the spawn API that is a drop in replacement for g_spawn is only needed where g_spawn doesn't work, and then it should only be used as a temporary workaround __until the plugins move to using the cmd version__.
Sometimes it's native to use argv (scope: <gdb-executable>, "--quiet", "--interpreter=mi2"). Having only CL would mean that one needs to create it from argv, and we currently have no platform independent mechanism for that (it's not hard to write one, but still).
So to me that means:
/* platform dependent checking */
spawn_get_program_name
Do we actually have a use case for this one?..
spawn_check_command
/* platform dependent running */
spawn_with_callbacks spawn_async spawn_sync
/* associated support functions */
spawn_kill_process
This one is "platform dependent killing".
spawn_write_data
This one looks "platform independent", but it's actually very easy to create a stdin-write function that blocks under Windows...
spawn_get_exit_status_callback
If you mean spawn_get_exit_status_cb, that only copies int status into gint *exit_status. You can check the status using the WIF* macros. With spawn.h included, the basic ones are defined on both *nix and Win~1.
--
Well, if spawn_sync and spawn_async remain public, for easier plugin migration, and the functions that Scope needs remain public, that means only spawn_get_program_name, spawn_async_with_pipes and spawn_get_exit_status_cb will be hidden.
Personally I'm for making spawn_async_with_pipes static until somebody requests it. It's powerful, but very hard to use properly.
And spawn_get_exit_status_cb should be static as well. That's just a one-liner, and somebody may confuse it with a function that processes the child exit status in some way.
--
An updated list of the API-s asked to remain public:
me WIF* lex spawn_get_program_name lex spawn_check_command me/lex spawn_kill_process spawn_async_with_pipes lex spawn_async me/lex spawn_with_callbacks me spawn_write_data lex? spawn_get_exit_status_cb lex spawn_sync
-- E-gards: Jimmy
On 28 June 2015 at 05:46, Dimitar Zhekov dimitar.zhekov@gmail.com wrote:
On 27.6.2015 г. 04:40, Lex Trotman wrote:
My concern is that, for a given platform, all commands a user enters into Geany or plugins should use the same meta chars and quoting. [...]
Well, after we could not agree on universal syntax, the next best thing would be to at least have consistent syntax on each platform.
Yes.
Many plugins currently do their own command parsing to get an argv and others use g_spawn_command (which is hard wired to unix rules, it uses g_shell_parse_argv). On windows these plugins will now use different rules to the build commands because the build commands use spawn_* and get platform dependent rules.
They will, though not that different. Most important when specifying a windows command with the *nix rules is you need to use either \ or / as directory separator. Both will work, Win~1 supports that.
I thought (based on memory of our previous discussions) that there were differences in quoting as well. Also though "Windows" might accept / in paths, will all programs, or will they confuse it with options?
These plugins should be encouraged to switch to the same rules as Geany, ie to use spawn_* calls with a cmd not an argv. That means the appropriate spawn_* API should be available.
As of me, they should be encouraged to use the native Windows rules for the benefit of the users. Unless we design Geany for *nix users which run Win~1 from time to time. :)
It seems that way sometimes :)
(I assume be "cmd" you mean "command line", not cmd.exe).
Yes, sorry, ambiguous choice of abbreviation.
The API we need to expose for this is the API that allows an unparsed command to be used, not versions that only allow argv.
All spawn calls support both a CL and an argv. If both are passed, argv is appended to CL.
So the spawn API that is a drop in replacement for g_spawn is only needed where g_spawn doesn't work, and then it should only be used as a temporary workaround __until the plugins move to using the cmd version__.
Sometimes it's native to use argv (scope: <gdb-executable>, "--quiet", "--interpreter=mi2"). Having only CL would mean that one needs to create it from argv, and we currently have no platform independent mechanism for that (it's not hard to write one, but still).
Does the user enter the individual argv elements? Its fine to use argv for code generated commands, in fact I would recommend it, let spawn do whatever quoting it needs, don't have everybody try to code it wrong themselves. I am not in any way suggesting that the spawn_* functions not have argv available, just that it not be used for *user entered* commands.
So to me that means:
/* platform dependent checking */
spawn_get_program_name
Do we actually have a use case for this one?..
Hmmm, no, scratch it then.
spawn_check_command
/* platform dependent running */
spawn_with_callbacks spawn_async spawn_sync
/* associated support functions */
spawn_kill_process
This one is "platform dependent killing".
Yes, thats why I included it.
spawn_write_data
This one looks "platform independent", but it's actually very easy to create a stdin-write function that blocks under Windows...
Better document it then :)
spawn_get_exit_status_callback
If you mean spawn_get_exit_status_cb, that only copies int status into gint *exit_status. You can check the status using the WIF* macros. With spawn.h included, the basic ones are defined on both *nix and Win~1.
ok.
--
Well, if spawn_sync and spawn_async remain public, for easier plugin migration, and the functions that Scope needs remain public, that means only spawn_get_program_name, spawn_async_with_pipes and spawn_get_exit_status_cb will be hidden.
Personally I'm for making spawn_async_with_pipes static until somebody requests it. It's powerful, but very hard to use properly.
Ok, most plugins seem to do fairly simple things.
And spawn_get_exit_status_cb should be static as well. That's just a one-liner, and somebody may confuse it with a function that processes the child exit status in some way.
I understood it as a convenient default callback, hardly worth not exporting it if everybody is going to define the same one-liner themselves. And I'm sure you will document it so nobody is confused :)
--
An updated list of the API-s asked to remain public:
me WIF* lex spawn_get_program_name
scratch the above
lex spawn_check_command me/lex spawn_kill_process spawn_async_with_pipes lex spawn_async me/lex spawn_with_callbacks me spawn_write_data lex? spawn_get_exit_status_cb lex spawn_sync
Cheers Lex
-- E-gards: Jimmy _______________________________________________ Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
On 27.6.2015 г. 23:50, Lex Trotman wrote:
On 28 June 2015 at 05:46, Dimitar Zhekovdimitar.zhekov@gmail.com wrote:
On 27.6.2015 г. 04:40, Lex Trotman wrote:
spawn_write_data
This one looks "platform independent", but it's actually very easy to create a stdin-write function that blocks under Windows...
Better document it then :)
from spawn.c:853: "The @a stdin_cb may write to @c channel only once per invocation, only if @c G_IO_OUT is set, and only a non-zero number of characters."
Though I haven't tested (or don't remember) whether writing more than 4K under Win~1 may block... If that's the case, your stdin_cb will better be identical to spawn_write_data.
spawn_get_exit_status_callback
I understood it as a convenient default callback, hardly worth not exporting it if everybody is going to define the same one-liner themselves. And I'm sure you will document it so nobody is confused :)
and from spawn.c:853: "Convinience @c GChildWatchFunc callback that copies the child exit status into a gint pointed by @a exit_status."
It took me a considerable time to solve a seemingly simple task, so it would have been a shame not to document everything. :)
-- E-gards: Jimmy
On 2015-06-27 12:46 PM, Dimitar Zhekov wrote:
[...]
An updated list of the API-s asked to remain public:
me WIF* lex spawn_get_program_name lex spawn_check_command me/lex spawn_kill_process spawn_async_with_pipes lex spawn_async me/lex spawn_with_callbacks me spawn_write_data lex? spawn_get_exit_status_cb lex spawn_sync
We should only export what you have concrete plans to use during the next cycle. If Lex wants update plugins during the next cycle to use other parts of the API, we can expose them at the time that the PR is made. No point to speculatively expose API that nobody has immediate plans to use.
Cheers, Matthew Brush
On 28 June 2015 at 12:40, Matthew Brush mbrush@codebrainz.ca wrote:
On 2015-06-27 12:46 PM, Dimitar Zhekov wrote:
[...]
An updated list of the API-s asked to remain public:
me WIF* lex spawn_get_program_name lex spawn_check_command me/lex spawn_kill_process spawn_async_with_pipes lex spawn_async me/lex spawn_with_callbacks me spawn_write_data lex? spawn_get_exit_status_cb lex spawn_sync
We should only export what you have concrete plans to use during the next cycle. If Lex wants update plugins during the next cycle to use other parts of the API, we can expose them at the time that the PR is made. No point to speculatively expose API that nobody has immediate plans to use.
If no API is available then nobody will have any plans so no API is needed so nobody will have any plans for the egg so no chicken is needed so ....
Cheers, Matthew Brush _______________________________________________ Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
On 28 June 2015 at 12:54, Lex Trotman elextr@gmail.com wrote:
On 28 June 2015 at 12:40, Matthew Brush mbrush@codebrainz.ca wrote:
On 2015-06-27 12:46 PM, Dimitar Zhekov wrote:
[...]
An updated list of the API-s asked to remain public:
me WIF* lex spawn_get_program_name lex spawn_check_command me/lex spawn_kill_process spawn_async_with_pipes lex spawn_async me/lex spawn_with_callbacks me spawn_write_data lex? spawn_get_exit_status_cb lex spawn_sync
We should only export what you have concrete plans to use during the next cycle. If Lex wants update plugins during the next cycle to use other parts of the API, we can expose them at the time that the PR is made. No point to speculatively expose API that nobody has immediate plans to use.
If no API is available then nobody will have any plans so no API is needed so nobody will have any plans for the egg so no chicken is needed so ....
Or to look at it another way:
"Welcome to Geany 1.25, so that their command parsing is the same as Geanys, all plugins are recommended to upgrade to the spawn_* API - that we havn't released."
Cheers Lex
Cheers, Matthew Brush _______________________________________________ Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
On 2015-06-27 07:54 PM, Lex Trotman wrote:
On 28 June 2015 at 12:40, Matthew Brush mbrush@codebrainz.ca wrote:
On 2015-06-27 12:46 PM, Dimitar Zhekov wrote:
[...]
An updated list of the API-s asked to remain public:
me WIF* lex spawn_get_program_name lex spawn_check_command me/lex spawn_kill_process spawn_async_with_pipes lex spawn_async me/lex spawn_with_callbacks me spawn_write_data lex? spawn_get_exit_status_cb lex spawn_sync
We should only export what you have concrete plans to use during the next cycle. If Lex wants update plugins during the next cycle to use other parts of the API, we can expose them at the time that the PR is made. No point to speculatively expose API that nobody has immediate plans to use.
If no API is available then nobody will have any plans so no API is needed so nobody will have any plans for the egg so no chicken is needed so ....
It can easily be made available on-demand, as normal.
We should only be concerned about what Scope is requesting as it has concrete plans to use them during the next development cycle and doesn't want the plugin to depend on the next development version of Geany during that time[0]. There's no reason to speculatively rush other APIs in if no plugins have immediate plans to use them nor have the concern about depending on the development version of Geany.
Cheers, Matthew Brush
[0]: Which IMO is a kind of tenuous reason since most people will use the version of Scope that their distro provides or from other releases, which for the most part means the next released version of Geany/GP anyway. I guess maybe some bleeding edge distros might package it in-between releases or something, though.
Am 28.06.2015 um 05:14 schrieb Matthew Brush:
On 2015-06-27 07:54 PM, Lex Trotman wrote:
On 28 June 2015 at 12:40, Matthew Brush mbrush@codebrainz.ca wrote:
On 2015-06-27 12:46 PM, Dimitar Zhekov wrote:
[...]
An updated list of the API-s asked to remain public:
me WIF* lex spawn_get_program_name lex spawn_check_command me/lex spawn_kill_process spawn_async_with_pipes lex spawn_async me/lex spawn_with_callbacks me spawn_write_data lex? spawn_get_exit_status_cb lex spawn_sync
We should only export what you have concrete plans to use during the next cycle. If Lex wants update plugins during the next cycle to use other parts of the API, we can expose them at the time that the PR is made. No point to speculatively expose API that nobody has immediate plans to use.
If no API is available then nobody will have any plans so no API is needed so nobody will have any plans for the egg so no chicken is needed so ....
It can easily be made available on-demand, as normal.
We should only be concerned about what Scope is requesting as it has concrete plans to use them during the next development cycle and doesn't want the plugin to depend on the next development version of Geany during that time[0]. There's no reason to speculatively rush other APIs in if no plugins have immediate plans to use them nor have the concern about depending on the development version of Geany.
Depending on a development version of Geany can be a concern.
I don't want to judge on the specific spawn_* APIs. But why not just release with the APIs being available (linkable by plugins) without already setting them officially in stone. In other words, find a method to mark these APIs experimental so that we can more easily change or remove them. This experimental flag should be set for at least one release cycle (but perhaps more depending on the situation). Then only plugin developers that are prepared to adapt their plugins quickly can use them (likely the same plugin developers who pushed the API into Geany for their plugins in the first place). Arguably, this should be the default process for any new API (IMO).
Best regards
On 24.6.2015 г. 02:05, Colomban Wendling wrote:
IMO, spawn_with_callbacks() is *the* key API from spawn, what makes it great.
It is, indeed.
spawn_async_with_pipes() can be nice, but most people probably shouldn't use them as they have same IO trickery most IO layers have, where you have to take care of not filling up any pipes (write) or forget to empty them (read).
It's doccomment specially says "use spawn_with_callbacks() unless you need to setup specific event sources". Equivalent to glib g_spawn_async_with_pipes - shoot yourself in the foot. I'm not opposed to hiding it completely.
BTW, I just noticed that on Windows spawn_async_with_pipes() requires the GLib main loop to be running if child_pid=NULL (well, it registers a watch func, not sure what happens if it doesn't execute -- zombie?). We probably don't care much though.
I should check that, and either document it, or remove support for child_pid = NULL - especially if the function is hidden.
spawn_async() doesn't seem so much useful to me, [...]
spawn_sync() is nice because it allows to easily pipe through a process (send some data to stdin and and read the output). How often is this useful for everyone I don't know [...]
I wrote those for completeness, as equivalents to g_spawn_[a]_sync. Though the prototypes are different, and spawn_sync supports binary I/O. The reasons I mentioned before apply - no helper process, native command line, better locale support, and likely improved security.
spawn_get_exit_status_cb() seem useless to the API IMO. It's a trivial function currently only used by spawn itself. It might even be sensible to make it completely internal.
I'm not against it.
I would have said that spawn_write_data() wasn't really required because it's relatively simple and not used by Geany outside of spawn, but it's indeed probably handy in combination with spawn_with_callbacks() to anyone wanting to feed a simple buffer to stdin.
spawn_write_data() can be used either directly as a callback, or invoked by a more complex callback, which would probably be the case with Scope. It's indeed simple (if you know that the optimal read size under Windows is 2K), but do we need to repeat it, or write something similar, each time we want to write something to child's stdin?
spawn_get_program_name() is only used in spawn itself and doesn't seem to be a strict requirement.
May be handy to get the program name from a command line... but most people would probably need spawn_check_command().
spawn_check_command() seem nice to validate a user command before actually running it, so it might be useful to anyone wanting to run user-supplied commands through the spawn API, to e.g. check for issues right when the user defines the command (like how we do it in the custom commands dialog).
Any plugin that lets you define a command line may benefit from it. For now, I plan a per-program gdb executable setting in Scope, but without any gdb options, and thus don't need it...
-- E-gards: Jimmy
On 24.6.2015 г. 02:05, Colomban Wendling wrote:
BTW, I just noticed that on Windows spawn_async_with_pipes() requires the GLib main loop to be running if child_pid=NULL (well, it registers a watch func, not sure what happens if it doesn't execute -- zombie?). We probably don't care much though.
Fixed child_pid=NULL with PR 536.
Also defined GEANY_API_SYMBOL as empty when compiling the spawn testing program.
-- E-gards: Jimmy
On 24.6.2015 г. 01:18, Matthew Brush wrote:
On 2015-06-23 09:04 AM, Dimitar Zhekov wrote:
[...]
From you're previous email you mentioned the stuff checked with [x] below:
[x] spawn_kill_process() [x] spawn_with_callbacks() [x] spawn_write_data() [x] SpawnFlags [x] SpawnReadFunc [x] SpawnWriteData
Is that sufficient, or is there other stuff? I will remove the /** from anything that is not expected to be needed, if nobody objects.
Well, I can't be 100% sure before the implementation, but since everything in spawn is stateless, I can simply copy anything else, should need be, as a temporary measure.
About the WIF* macros, those (at present) are not "officially" part of the plugin API as they have no Doxygen comments
They are documented in POSIX wait [1]. Though, as being defined by Geany under Windows, a short Doxygen comment may be needed.
Well you know my opinion :) [...] I think we all also agree it's stupid for plugins to have to copy&paste code that exists already or else use an inferior version.
On that note, scope/src/plugme contains functions that I need exported from Geany since forever. Especially plugme_ui_setup_open_button_callback(), which is defined in several plugins under different names with varying quality. IIRC, the original Geany functions support Windows native file selection dialogs, but no plugin does. "Inferior version"-s.
regardless if something possibly better[1] like GProcess comes available to us, and we don't even use it internally anymore. [1]: I have no idea if it's better, just an example.
If it's based on the glib spawning functions, it'll be no better than them. And as long as the glib developers want to support G_SPAWN_LEAVE_DESCRIPTORS_OPEN under Windows and convert anything to UNICODE, the glib functions will not improve.
[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/wait.html
-- E-gards: Jimmy
On 2015-06-20 08:12 PM, Matthew Brush wrote:
Hi All,
I just noticed that the new spawn code exposes almost every single bit of API possible. Do we really want to do that, or should we limit it only to what is currently needed by any plugins? A quick survey of Geany-Plugins shows no usage of any of this yet.
IMO, we shouldn't expose anything which is not needed by plugins, especially if it's not related to the plugin API.
One further thought before we get locked into the exposed API.
Shouldn't all the spawn stuff be in the utils_* namespace? From the plugin developer perspective it's just some more utility functions like the ones it improves upon already in the utils_ namespace and that seems to be the place where we dump all the general purpose convenience functions and stuff that makes up for GLib short-comings, and that's effectively what this is.
If we wanted to keep the file-wise isolation of the spawn code, we could just tweak the Doxygen comments a bit to put the functions under that section of the API docs, rather than giving a handful of helper functions their own whole "module" (API-wise, ex. "namespace" and docs).
Any opinions?
Cheers, Matthew Brush
On 26 June 2015 at 11:11, Matthew Brush mbrush@codebrainz.ca wrote:
On 2015-06-20 08:12 PM, Matthew Brush wrote:
Hi All,
I just noticed that the new spawn code exposes almost every single bit of API possible. Do we really want to do that, or should we limit it only to what is currently needed by any plugins? A quick survey of Geany-Plugins shows no usage of any of this yet.
IMO, we shouldn't expose anything which is not needed by plugins, especially if it's not related to the plugin API.
On IRC Colomban had me look at what spawning G-P plugins did (I looked at C code only).
The result is that there are twelve g_spawn* calls which should be migrated to spawn_* equivalents. At least for those that use commands input by the user so the quoting is the same as Geany.
There are five that use utils_spawn (which uses spawn_* internally) and they also should be migrated if they do their own command decoding.
The above migrations will allow plugins that do their own decoding of commands to be simplified and become consistent.
There is also a custom spawn in geanyctags, I didn't look at what it did differently.
One further thought before we get locked into the exposed API.
Shouldn't all the spawn stuff be in the utils_* namespace? From the plugin developer perspective it's just some more utility functions like the ones it improves upon already in the utils_ namespace and that seems to be the place where we dump all the general purpose convenience functions and stuff that makes up for GLib short-comings, and that's effectively what this is.
It was proposed to make spawn a library, that would mean a separate namespace would be better.
If we wanted to keep the file-wise isolation of the spawn code, we could just tweak the Doxygen comments a bit to put the functions under that section of the API docs, rather than giving a handful of helper functions their own whole "module" (API-wise, ex. "namespace" and docs).
Is there a "misc" section? :)
But otherwise yeah, the utils section of the manual seems ok.
Any opinions?
Cheers, Matthew Brush
Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel
On Fri, Jun 26, 2015 at 3:31 AM, Lex Trotman elextr@gmail.com wrote:
There is also a custom spawn in geanyctags, I didn't look at what it did differently.
There isn't - I'm using utils_spawn_sync() inside the spawn_cmd() function.
Cheers,
Jiri
On 26 June 2015 at 17:45, Jiří Techet techet@gmail.com wrote:
On Fri, Jun 26, 2015 at 3:31 AM, Lex Trotman elextr@gmail.com wrote:
There is also a custom spawn in geanyctags, I didn't look at what it did differently.
There isn't - I'm using utils_spawn_sync() inside the spawn_cmd() function.
Explains why it had a custom spawn and two utils_spawn_sync() calls :)
Cheers,
Jiri
Devel mailing list Devel@lists.geany.org https://lists.geany.org/cgi-bin/mailman/listinfo/devel