Re: [Github-comments] [geany/geany] WIP: Windows (and Linux) spawn fixes (#1300)
Conflicts and still WIP, moved to 1.31 -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1300#issuecomment-281864018
Re: [Github-comments] [geany/geany] WIP: Windows (and Linux) spawn fixes (#1300)
@eht16 don't sorry for a second on how it'll be to get the changes here in, it's not important, and re-applying them manually will be easy. Just do what's needed to get #1095 in its best shape, and I'll deal with this later. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1300#issuecomment-260187057
Re: [Github-comments] [geany/geany] WIP: Windows (and Linux) spawn fixes (#1300)
@b4n I updated #1095 and picked the relevant commits from this PR and added 92294cb56679cd8943e454b246cbe96c76bcfcee on top of that which reverts (hopefully) all non-Windows changes. As the revert commit will most likely break merging this PR, I hope it will be easy to fix the conflicts as there are not too many changes. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1300#issuecomment-260159109
Re: [Github-comments] [geany/geany] WIP: Windows (and Linux) spawn fixes (#1300)
b4n commented on this pull request.
> @@ -826,14 +988,27 @@ static gchar *prepare_run_cmd(GeanyDocument *doc, gchar
> **working_dir, guint cmd
}
#endif
- run_cmd = build_create_shellscript(*working_dir, cmd_string, autoclose,
&error);
- if (!run_cmd)
- {
- ui_set_statusbar(TRUE, _("Failed to execute \"%s\"
(start-script could not be created: %s)"),
- !EMPTY(cmd_string_utf8) ? cmd_string_utf8 : NULL,
error->message);
- g_error_free(error);
- g_free(*working_dir);
- }
+#ifdef G_OS_WIN32
+ /* Expand environment variables like %blah%. */
+ SETPTR(cmd_string, win32_expand_environment_variables(cmd_string));
+#endif
+
+ gchar *helper =
g_build_filename(utils_resource_dir(RESOURCE_DIR_LIBEXEC), "geany-run-helper",
NULL);
Yes, but I think it should be added after (e.g. is slightly out of scope here)
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/1300
Re: [Github-comments] [geany/geany] WIP: Windows (and Linux) spawn fixes (#1300)
@eht16 I'll probably try and get a version that only touches Windows for the release. Or you can do it, as I probably won't have much time before noon tomorrow ;) -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1300#issuecomment-260151846
Re: [Github-comments] [geany/geany] WIP: Windows (and Linux) spawn fixes (#1300)
Looks good conceptually. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1300#issuecomment-260145847
Re: [Github-comments] [geany/geany] WIP: Windows (and Linux) spawn fixes (#1300)
codebrainz commented on this pull request.
> @@ -826,14 +988,27 @@ static gchar *prepare_run_cmd(GeanyDocument *doc, gchar
> **working_dir, guint cmd
}
#endif
- run_cmd = build_create_shellscript(*working_dir, cmd_string, autoclose,
&error);
- if (!run_cmd)
- {
- ui_set_statusbar(TRUE, _("Failed to execute \"%s\"
(start-script could not be created: %s)"),
- !EMPTY(cmd_string_utf8) ? cmd_string_utf8 : NULL,
error->message);
- g_error_free(error);
- g_free(*working_dir);
- }
+#ifdef G_OS_WIN32
+ /* Expand environment variables like %blah%. */
+ SETPTR(cmd_string, win32_expand_environment_variables(cmd_string));
+#endif
+
+ gchar *helper =
g_build_filename(utils_resource_dir(RESOURCE_DIR_LIBEXEC), "geany-run-helper",
NULL);
To facilitate customization, it might be nice to put a test to see if the file
exists in the config dir and use the system one only if a customized one
doesn't exist (as with most stuff in Geany).
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/1300#pullrequestreview-8303883
Re: [Github-comments] [geany/geany] WIP: Windows (and Linux) spawn fixes (#1300)
codebrainz commented on this pull request.
> +} Placeholder;
+
+
+static const gchar *placeholder_replacement(Placeholder *placeholders, size_t
n_placeholders, gchar placeholder)
+{
+ for (size_t i = 0; i < n_placeholders; i++)
+ {
+ if (placeholders[i].placeholder == placeholder)
+ return placeholders[i].replacement;
+ }
+
+ return NULL;
+}
+
+
+/* Replaces %s-style placeholders in a string.
Oh it's for the build commands? My question still stands, but it's completely
out of scope for this PR.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/1300
Re: [Github-comments] [geany/geany] WIP: Windows (and Linux) spawn fixes (#1300)
codebrainz commented on this pull request.
> +} Placeholder;
+
+
+static const gchar *placeholder_replacement(Placeholder *placeholders, size_t
n_placeholders, gchar placeholder)
+{
+ for (size_t i = 0; i < n_placeholders; i++)
+ {
+ if (placeholders[i].placeholder == placeholder)
+ return placeholders[i].replacement;
+ }
+
+ return NULL;
+}
+
+
+/* Replaces %s-style placeholders in a string.
Just out of curiosity, why not using template-style placeholders and re-using
the template replacement code?
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/1300#pullrequestreview-8303608
Re: [Github-comments] [geany/geany] WIP: Windows (and Linux) spawn fixes (#1300)
@b4n pushed 1 commit. 59158ed Improve a comment -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/geany/geany/pull/1300/files/07f8d29d5fe0e42cc9882474ed1469e6b7b01f0e..59158ed0c8c2e3cad95d7734c5acc814ddf837ff
Re: [Github-comments] [geany/geany] WIP: Windows (and Linux) spawn fixes (#1300)
@b4n pushed 1 commit. 07f8d29 Remove spurious executable bit on some source files -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/geany/geany/pull/1300/files/900072cf157606c2abddc75ea8be610a237130a5..07f8d29d5fe0e42cc9882474ed1469e6b7b01f0e
Re: [Github-comments] [geany/geany] WIP: Windows (and Linux) spawn fixes (#1300)
@b4n pushed 1 commit. 900072c Restore OSX compatibility to the run helper -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/geany/geany/pull/1300/files/087c9d0777c20ccdb6478796e67bdaa631f69c7b..900072cf157606c2abddc75ea8be610a237130a5
Re: [Github-comments] [geany/geany] WIP: Windows (and Linux) spawn fixes (#1300)
Okay the `cd` part is for OSX: 98ae34f1dcc671bc691c40fa9896ab5624818e7f I'll add it back then. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1300#issuecomment-260140456
Re: [Github-comments] [geany/geany] WIP: Windows (and Linux) spawn fixes (#1300)
@b4n pushed 1 commit. 087c9d0 Fix distribution of geany-run-helper scripts (oops) -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/geany/geany/pull/1300/files/870c1379a0f633195c74e73a839d83dc3e50abfa..087c9d0777c20ccdb6478796e67bdaa631f69c7b
Re: [Github-comments] [geany/geany] WIP: Windows (and Linux) spawn fixes (#1300)
@b4n pushed 2 commits. 22017d6 Remove now unused Windows utils 870c137 spawn: Don't depend on utils.h, and fix locale compat on Windows -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/geany/geany/pull/1300/files/0a8d07d6062efa4da799f954ae475b927d0aa87d..870c1379a0f633195c74e73a839d83dc3e50abfa
[Github-comments] [geany/geany] WIP: Windows (and Linux) spawn fixes (#1300)
Based off #1095 and comments, plus some code I wrote ages ago (and have been using since then) Main things * Use `CreateProcessW()` on Windows * Use an installed run script rather than a dynamically created one * Fix replacing build command placeholders on non-Windows so that everything is always properly quoted. This is based on some quoting code I wrote ages ago to fix a "security" issue we have that a very special filename can execute arbitrary commands. This has 2 main advantages * placeholders replacements are always properly quoted (minus bugs in the code, but I'm using it for a while now in my own machine: initial version is from 2012 and I had one single relevant fix since then, more than one year ago) * placeholders in a placeholder replacement isn't replaced again. Currently in master, each pattern is replaced, and then the next, lading to possibly finding placeholders inside a previous replacement. With my patch, it's replaced in one go, and only from the source string. Also something like this is required to be able to replace "complex" placeholders, like full commands to pass to `sh -c` like we do in the run command. Without something clever, it's virtually impossible to get correct quoting for this. A better way to fix it *could* be to replace placeholders after having split the arguments; however it requires lot of care and possibly conditional code too, in order to support all of *NIX, Window, and VTE. And there would have to be an argv-to-commandline anyway, as we need to have a way to pass an argument to `sh -c`. Good Windows support is missing, and currently no extra quoting is done here, so no changes on the placeholders there apart from replacing all at once. On Windows, it should fix all the spawning issues related to non-ASCII filenames. On Linux, it should not change much apart from properly replacing placeholders, and possibly some uncommon things like if the run script could not be created for some reason. **Warning:** currently the new run script doesn't `cd` to the working directory configured in the build command, and this relies on us setting the proper working directory when spawning the command itself. If this breaks anything (like possibly a *.profile* that `cd`s somewhere?), it could be added back. You can view, comment on, or merge this pull request online at: https://github.com/geany/geany/pull/1300 -- Commit Summary -- * Windows: Convert working directory into locale encoding before spawning commands * Windows: Change codepage for directory change in created batch run script * Windows: Convert command into locale encoding before executing * Windows: Convert generated batch script into system codepage * spawn: Fix Windows environment encoding and other related fixes * spawn: Fix Windows environment building (oops) * spawn: convert working_directory only if non-NULL * Drop useless macro and use G_N_ELEMENTS() instead * Prevent undefined working directory and command * Use GetConsoleCP() to get the input console codepage * Merge remote-tracking branch 'eht16/issue1076_win32_build_working_dir_locale' into eth16winspawnenc * spawn: Use Wide API on Windows * Merge branch 'issue1075_win32_build_working_dir_locale' into eth16winspawnenc * Use a program run helper rather than a script * Fix quoting of cmd.exe command * Implement autoclose feature in the run helper * Implement the run helper as a script * Fix autoclose in non-Windows run script (oops) * Windows: fix running a command when the helper has a space in its path * Fix invalid memory access (oops) * Windows: expand environment variables using UNICODE API * Windows: Expand environmen variables in build commands again * Don't try and unlink the run command, it's not a file anymore (oops) * Remove unused stuff * Escape replacements for build command placeholders * Fix quoting of the run helper path * Fix run command on Linux -- File Changes -- M src/Makefile.am (9) M src/build.c (380) A src/geany-run-helper (25) A src/geany-run-helper.bat (27) M src/prefix.h (1) M src/spawn.c (124) M src/utils.c (3) M src/utils.h (1) M src/win32.c (45) M src/win32.h (4) -- Patch Links -- https://github.com/geany/geany/pull/1300.patch https://github.com/geany/geany/pull/1300.diff -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/1300
