[geany] Alter spawn to return the error message only in error->message (#701)

List overview All Threads

newer

older

[geany] Moving the Compiler tab to...

[geany] Update...

zhekov

19 Oct 2015 19 Oct '15

1:06 p.m.

The goal of this PR is to improve the spawning error message, in both the spawn itself and most of it's callers, as described in issue #541. You can view, comment on, or merge this pull request online at:

https://github.com/geany/geany/pull/701

-- Commit Summary --

* Alter spawn to return the error message only in error->message

-- File Changes --

M src/spawn.c (54)

-- Patch Links --

https://github.com/geany/geany/pull/701.patch https://github.com/geany/geany/pull/701.diff

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701

Attachments:

attachment.htm (text/html — 1.7 KB)

Show replies by date

zhekov

19 Oct 19 Oct

1:22 p.m.

This commit is incomplete: needs to fix g_spawn citing the original program.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701#issuecomment-149287727

zhekov

1:47 p.m.

Pending commits: build, callbacks, printing, search, templates and tools.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701#issuecomment-149294816

elextr

7:45 p.m.

Yes, this is a better way, and it removes the %s substitution in messages. Such substitutions might not make sense if the messages are translated.

Thats why I showed in #541 that if the the caller is going to add to the returned message it simply separates the two messages with `:` not combines them into a sentence. Then the order is fixed and language independent.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701#issuecomment-149378355

zhekov

20 Oct 20 Oct

12:39 p.m.

I plan one final change to the spawning module messages. Since both g_spawn/*nix and the Windows spawn are complex calls, where several functions may fail, the plain error message may not be insufficient to understand what's going on.

To avoid replacing all g_spawn/*nix messages, and handle this situation, the final messages will be as follows:

exec() or CreateProcess() failed, which are almost 100% of the cases => the plain errno/winerror message is returned: "No such file or directory", "Invalid exec format" and so on.

Some other function failed (exceedingly rare) -> Descriptive text (errno/winerror message), for example: "Failed to read data from child process (Bad file descriptor)" or "Unexpected error in select() reading data from a child process (I/O error)". These examples are a bit random, I have never seen g_spawn failing like that.

And there will be no different error messages depending on whether spawn is compiled for testing. The above text should be a reasonable compromise - too bad it haven't occurred to me earlier.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701#issuecomment-149626060

zhekov

1:50 p.m.

That's all. As noted in the issue, the build command (start and kill) messages are different, so I'm leaving them as is, unless we have clarity on their texts.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701#issuecomment-149646073

Colomban Wendling

21 Oct 21 Oct

10:42 a.m.

...

} else { command = g_strdup(tool_prefs.context_action_cmd);
format = _("Cannot execute context action command \"%s\": %s. "
	"Check the path setting in Filetype configuration.");

this and above ones seem to be swapped

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701/files#r42632225

Colomban Wendling

10:44 a.m.

...

	ui_set_statusbar(TRUE, "Context action command failed: %s", error->message);

	ui_set_statusbar(TRUE, format, command, error->message);

I'm not fond of non-literal arguments to printf()-style formats as GCC can warn for literals but isn't smart enough for this usage (yet). But if it makes things much simpler, I guess it's ok as it's not out first one.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701/files#r42632495

Colomban Wendling

10:45 a.m.

...

	ui_set_statusbar(TRUE, "Context action command failed: %s", error->message);

	ui_set_statusbar(TRUE, format, command, error->message);

do we really want the literal `%s` rather than the replaced (and actually executed) version?

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701/files#r42632648

Colomban Wendling

10:53 a.m.

...

@@ -577,7 +604,83 @@ static gboolean spawn_async_with_pipes(const gchar *working_directory, const gch

spawned = g_spawn_async_with_pipes(working_directory, full_argv, envp, G_SPAWN_SEARCH_PATH | (child_pid ? G_SPAWN_DO_NOT_REAP_CHILD : 0), NULL, NULL,
child_pid, stdin_fd, stdout_fd, stderr_fd, error);
child_pid, stdin_fd, stdout_fd, stderr_fd, &gerror);
if (!spawned)

{
gint en = 0;
const gchar *message = gerror->message;
/* try to cut glib citing of the program name or working directory: they may be long,
   and only the caller knows whether they're UTF-8. We lose the exact chdir error. */
switch (gerror->code)

hum… I'm not sure I'm fan of such convoluted logic just for shrinking an uncommon string. Isn't that a bit overkill?

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701/files#r42633777

zhekov

1:09 p.m.

...

} else { command = g_strdup(tool_prefs.context_action_cmd);
format = _("Cannot execute context action command \"%s\": %s. "
	"Check the path setting in Filetype configuration.");

Definitely.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701/files#r42652263

zhekov

1:18 p.m.

...

	ui_set_statusbar(TRUE, "Context action command failed: %s", error->message);

	ui_set_statusbar(TRUE, format, command, error->message);

As you can see, the two formats only differ by "the path setting in <Preferences | Filetype configuration>", so I could have used a common _("Cannot execute context action command "%s": %s. " "Check the path setting in %s.") but wasn't sure it'll be good for the translations. If you say it's OK, I'll simplify the whole thing.

As of whether "we really want the literal %s", I'm not sure what you are talking about... We want the original command, without the selected text, which may be very long, and is an argument anyway.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701/files#r42653396

zhekov

1:33 p.m.

...

@@ -577,7 +604,83 @@ static gboolean spawn_async_with_pipes(const gchar *working_directory, const gch

spawned = g_spawn_async_with_pipes(working_directory, full_argv, envp, G_SPAWN_SEARCH_PATH | (child_pid ? G_SPAWN_DO_NOT_REAP_CHILD : 0), NULL, NULL,
child_pid, stdin_fd, stdout_fd, stderr_fd, error);
child_pid, stdin_fd, stdout_fd, stderr_fd, &gerror);
if (!spawned)

{
gint en = 0;
const gchar *message = gerror->message;
/* try to cut glib citing of the program name or working directory: they may be long,
   and only the caller knows whether they're UTF-8. We lose the exact chdir error. */
switch (gerror->code)

Unfortunately not. All "en" errors are returned by exec(), so they are the most common ones. And it's not about shrinking only.

The first reason is that g_spawn cites possibly non-UTF-8 text in a GError. Put something like this in geany_debug(), and Help -> Debug Messages will abort Geany with an assertion. And if that can happen, how can I be sure about the status bar, or the Status tab? The next gtk+ version may cause Geany to abort.

The second reason is to unify the format. g_spawn() cites the program name for some errors, but not for others, whereas we want standard `Cannot execute <what program> "<program>": <error text>. Check the path setting in <...>` messages everywhere. At least that's the current consensus.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701/files#r42655396

zhekov

1:38 p.m.

...

@@ -577,7 +604,83 @@ static gboolean spawn_async_with_pipes(const gchar *working_directory, const gch

spawned = g_spawn_async_with_pipes(working_directory, full_argv, envp, G_SPAWN_SEARCH_PATH | (child_pid ? G_SPAWN_DO_NOT_REAP_CHILD : 0), NULL, NULL,
child_pid, stdin_fd, stdout_fd, stderr_fd, error);
child_pid, stdin_fd, stdout_fd, stderr_fd, &gerror);
if (!spawned)

{
gint en = 0;
const gchar *message = gerror->message;
/* try to cut glib citing of the program name or working directory: they may be long,
   and only the caller knows whether they're UTF-8. We lose the exact chdir error. */
switch (gerror->code)

About convoluted logic, well - after replacing the *entire* spawning under Windows, reverting a G_SPAWN error code back to to errno is only a small touch. Why should I regard g_spawn under *nix as a sacred cow?.. :)

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701/files#r42656108

Colomban Wendling

29 Oct 29 Oct

2:33 p.m.

...

	ui_set_statusbar(TRUE, "Context action command failed: %s", error->message);

	ui_set_statusbar(TRUE, format, command, error->message);

`_("Cannot execute context action command "%s": %s. %s")` with a `check_msg=_("Check the path setting in Preferences.")` in each branch is probably fine for translations and works around the format issue. @frlan what do you think, is this OK for translators?

...

As of whether we really want the literal (replacement) %s - we want the original command, without the selected text, which may be very long, and is an argument anyway.

OK, if we assume it either can't be a Geany issue or that we don't care if Geany replaced `%s` in a way that broke spawning.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701/files#r43426440

Colomban Wendling

2:48 p.m.

...

@@ -617,7 +617,8 @@ static gchar *run_command(const gchar *command, const gchar *file_name, } else {
g_warning("templates_replace_command: %s", error->message);
g_warning(_("Cannot execute template replacement command \"%s\": %s. "
	"Check the path setting in template."), command, error->message);

something like *Cannot execute command "%s" from the template: %s. Check the path in the template.* would probably be more correct and easier to understand (as esp. it's not a command that processes templates, but a command replaced in the template).

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701/files#r43428286

Colomban Wendling

2:50 p.m.

...

@@ -239,8 +239,9 @@ void tools_execute_custom_command(GeanyDocument *doc, const gchar *command) } else {

geany_debug("spawn_sync() failed: %s", error->message);

ui_set_statusbar(TRUE, _("Custom command failed: %s"), error->message);

ui_set_statusbar(TRUE, _("Cannot execute custom command \"%s\": %s. "

	"Check the path setting in Preferences."), command, error->message);

Custom commands are not configured in the Preferences (although they probably should, but that's another topic) but in their own dialog (*Edit → Format → Send Selection to → Set Custom Commands*).

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701/files#r43428515

Colomban Wendling

2:51 p.m.

...

@@ -577,7 +604,83 @@ static gboolean spawn_async_with_pipes(const gchar *working_directory, const gch

spawned = g_spawn_async_with_pipes(working_directory, full_argv, envp, G_SPAWN_SEARCH_PATH | (child_pid ? G_SPAWN_DO_NOT_REAP_CHILD : 0), NULL, NULL,
child_pid, stdin_fd, stdout_fd, stderr_fd, error);
child_pid, stdin_fd, stdout_fd, stderr_fd, &gerror);
if (!spawned)

{
gint en = 0;
const gchar *message = gerror->message;
/* try to cut glib citing of the program name or working directory: they may be long,
   and only the caller knows whether they're UTF-8. We lose the exact chdir error. */
switch (gerror->code)

OK, non-UTF-8 that can lead to a crash is a good enough reason. Though we probably should fix that in the debug dialog too, if we can.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701/files#r43428645

Colomban Wendling

2:55 p.m.

@elextr please check the strings for goude Angrish :)

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701#issuecomment-152286327

Colomban Wendling

3 p.m.

Reminder for me: let's not forget to close #541 when merging this

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701#issuecomment-152287734

zhekov

30 Oct 30 Oct

1:28 p.m.

...

	ui_set_statusbar(TRUE, "Context action command failed: %s", error->message);

	ui_set_statusbar(TRUE, format, command, error->message);

Actually, it occurred to me a few days ago that not caring about %s is wrong, but I was too busy setting up my new computer...

A %s with a selection containing any quotes may break the command parsing under *unix. Thus we'd better display the short version on G_SPAWN_ERROR, and the long version on G_SHELL_ERROR (== parse error). Of course, it may break the spawning too, if %s is at the start of the command, but that's a bit extreme.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701/files#r43529283

zhekov

1:29 p.m.

...

@@ -617,7 +617,8 @@ static gchar *run_command(const gchar *command, const gchar *file_name, } else {
g_warning("templates_replace_command: %s", error->message);
g_warning(_("Cannot execute template replacement command \"%s\": %s. "
	"Check the path setting in template."), command, error->message);

Sure, why not (I'm not using these).

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701/files#r43529413

zhekov

1:31 p.m.

...

@@ -239,8 +239,9 @@ void tools_execute_custom_command(GeanyDocument *doc, const gchar *command) } else {

geany_debug("spawn_sync() failed: %s", error->message);

ui_set_statusbar(TRUE, _("Custom command failed: %s"), error->message);

ui_set_statusbar(TRUE, _("Cannot execute custom command \"%s\": %s. "

	"Check the path setting in Preferences."), command, error->message);

Yes... I haven't defined new ones from a very long time, and forgot about this.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701/files#r43529583

zhekov

1:34 p.m.

...

@@ -577,7 +604,83 @@ static gboolean spawn_async_with_pipes(const gchar *working_directory, const gch

spawned = g_spawn_async_with_pipes(working_directory, full_argv, envp, G_SPAWN_SEARCH_PATH | (child_pid ? G_SPAWN_DO_NOT_REAP_CHILD : 0), NULL, NULL,
child_pid, stdin_fd, stdout_fd, stderr_fd, error);
child_pid, stdin_fd, stdout_fd, stderr_fd, &gerror);
if (!spawned)

{
gint en = 0;
const gchar *message = gerror->message;
/* try to cut glib citing of the program name or working directory: they may be long,
   and only the caller knows whether they're UTF-8. We lose the exact chdir error. */
switch (gerror->code)

It's a gtk+ or glib assertion, not Geany, so we can only find some workaround.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701/files#r43529951

zhekov

1 Nov 1 Nov

8:38 a.m.

...

	ui_set_statusbar(TRUE, "Context action command failed: %s", error->message);

	ui_set_statusbar(TRUE, format, command, error->message);

Fixed.

(Small note: %s with a selection containing double quotes may cause parsing error under Windows too.)

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701/files#r43582667

zhekov

9:08 a.m.

...

@@ -617,7 +617,8 @@ static gchar *run_command(const gchar *command, const gchar *file_name, } else {
g_warning("templates_replace_command: %s", error->message);
g_warning(_("Cannot execute template replacement command \"%s\": %s. "
	"Check the path setting in template."), command, error->message);

Fixed.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701/files#r43582951

zhekov

9:23 a.m.

...

@@ -239,8 +239,9 @@ void tools_execute_custom_command(GeanyDocument *doc, const gchar *command) } else {

geany_debug("spawn_sync() failed: %s", error->message);

ui_set_statusbar(TRUE, _("Custom command failed: %s"), error->message);

ui_set_statusbar(TRUE, _("Cannot execute custom command \"%s\": %s. "

	"Check the path setting in Preferences."), command, error->message);

Fixed.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701/files#r43583065

zhekov

12:50 p.m.

Oops, we missed the string freeze...

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701#issuecomment-152848544

Colomban Wendling

12:54 p.m.

LGTM

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701#issuecomment-152848780

Colomban Wendling

12:57 p.m.

Oopsie indeed for the string freeze, but @frlan just gave me a special pass :)

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701#issuecomment-152848872

Colomban Wendling

1:02 p.m.

Merged #701.

--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/701#event-451400123

3118

Age (days ago)

3131

Last active (days ago)

github-comments@lists.geany.org

30 comments

3 participants

tags (0)

participants (3)

Colomban Wendling
elextr
zhekov