GNOME Bugzilla – Bug 772033
add a --version flag
Last modified: 2016-11-15 23:09:57 UTC
Add a --version flag to gjs commandline which would print the version of gjs.
Created attachment 338055 [details] [review] console: Process arguments better We now process our command line arguments as follows: all arguments before a program name or a -c or --command argument are passed to the GJS interpreter itself. All arguments after the program are passed into the program's ARGV array. This solves some weirdness where "gjs -c '...' --help" was showing the GJS help instead of passing the --help option onto the program given with -c, as expected. This corresponds to the way Python processes its command line arguments, and also opens up the possibility of --version or other arguments that might want to be accepted both by GJS and a script.
Created attachment 338056 [details] [review] console: Add --version option Now that we can avoid passing --version on to scripts, it is simple to add a --version option that prints the current GJS version.
Review of attachment 338055 [details] [review]: Pretty nice!
Review of attachment 338056 [details] [review]: Feel free to commit with this fixed. ::: gjs/console.cpp @@ +35,3 @@ static char *coverage_output_path = NULL; static char *command = NULL; +static gboolean print_version = false; Should probably be bool?
(In reply to Cosimo Cecchi from comment #4) > Review of attachment 338056 [details] [review] [review]: > > Feel free to commit with this fixed. > > ::: gjs/console.cpp > @@ +35,3 @@ > static char *coverage_output_path = NULL; > static char *command = NULL; > +static gboolean print_version = false; > > Should probably be bool? I prefer not, because G_OPTION_ARG_NONE would cause g_option_context_parse() to treat the arg_data field of GOptionEntry as a gboolean*, so we'd be casting bool* -> void* -> gboolean*. Probably would work nonetheless, since I'm pretty sure bool and gboolean would be the same size everywhere, but better not to take a chance. I'll hold off committing to give you a chance to disagree :-) Also I realized this probably needs a NEWS entry since it's a slight incompatibility with how it worked before.
(In reply to Philip Chimento from comment #5) > (In reply to Cosimo Cecchi from comment #4) > > Review of attachment 338056 [details] [review] [review] [review]: > > > > Feel free to commit with this fixed. > > > > ::: gjs/console.cpp > > @@ +35,3 @@ > > static char *coverage_output_path = NULL; > > static char *command = NULL; > > +static gboolean print_version = false; > > > > Should probably be bool? > > I prefer not, because G_OPTION_ARG_NONE would cause g_option_context_parse() > to treat the arg_data field of GOptionEntry as a gboolean*, so we'd be > casting bool* -> void* -> gboolean*. Probably would work nonetheless, since > I'm pretty sure bool and gboolean would be the same size everywhere, but > better not to take a chance. > > I'll hold off committing to give you a chance to disagree :-) That's OK. I guess it makes sense since we're using the GLib API, to abide by its rules :-) > Also I realized this probably needs a NEWS entry since it's a slight > incompatibility with how it worked before. Sure, feel free to add one.
Created attachment 338133 [details] [review] console: Process arguments better We now process our command line arguments as follows: all arguments before a program name or a -c or --command argument are passed to the GJS interpreter itself. All arguments after the program are passed into the program's ARGV array. This solves some weirdness where "gjs -c '...' --help" was showing the GJS help instead of passing the --help option onto the program given with -c, as expected. This corresponds to the way Python processes its command line arguments, and also opens up the possibility of --version or other arguments that might want to be accepted both by GJS and a script.
Attachment 338056 [details] pushed as 0b9f702 - console: Add --version option Attachment 338133 [details] pushed as 7ea4903 - console: Process arguments better
I attached the version of the patch with the NEWS file, for reference.
Unfortunately, this change breaks executable JS scripts that specify GJS in their shebang line, because it's not possible to stick any command line arguments in between the GJS binary name and the script name. So, for example, script: #!/usr/bin/env gjs print(imports.mymodule.something); Whereas previously you could do ./script -I /path/to/mymodule --coverage-output=coverage.out Those flags now won't have the desired effect, and it's impossible to convey the options to script unless you run it as "gjs script -I ..." Looking at how Python does things I think it would be good to deprecate these command line arguments in favour of environment variables. We already have GJS_PATH, and we could add GJS_COVERAGE_PREFIXES and GJS_COVERAGE_OUTPUT. The question is how to do this while causing the least inconvenience to app authors who use the command line arguments. 1. We could revert the patch for now, and issue a warning when the command line options are used, suggesting to use the environment variables instead. Then in the next-after-next minor release we could remove the command line options. 2. We could keep the patch, and scan the command line arguments passed to the script to see if they contain -I, --include, --coverage-prefix, -C, or --coverage-output. Then we could issue a warning. Modules would break, but at least the module authors would know how to fix it. 3. We could keep the patch, scan the command line arguments passed to the script, but mimic the old behaviour too (that is, if the script gets -I, we still pass -I on to the script, but also add a path to the include paths.) Also in this case we would issue a warning. After a suitable amount of time the mimicking behaviour would be removed and we'd go back to #2 but hopefully after all users of the command line options had switched to environment variables instead.
For that matter, with #2 and #3 we could still continue to support the command line options if they were passed before the script, that's what Python does.
Created attachment 338864 [details] [review] console: Check script args for stray GJS args In order to ease the pain of migration, check to see if any --include-path, --coverage-prefix, or --coverage-output arguments are being passed to the JS script. If so, treat them as if they had been passed to GJS while issuing a warning that this behaviour will go away in the future.
Created attachment 338865 [details] [review] console: Env vars for coverage args Encourage use of environment variables for coverage command line arguments, GJS_COVERAGE_PREFIXES instead of --coverage-prefix, and GJS_COVERAGE_OUTPUT instead of --coverage-output. This is because we want to pass arguments occurring after the script on the command line as arguments to the script and not to GJS. Without environment variables, it would otherwise be impossible to turn on coverage, for example, for a script that started with #!/usr/bin/gjs.
Above are two patches that implement #3, which is the solution that I personally prefer.
Review of attachment 338864 [details] [review]: ::: gjs/console.cpp @@ +97,3 @@ + + if (coverage_prefixes != NULL && + int ix, argc_copy = argc + 1; I think this approach has a couple of issues: - in case the arguments are both before and after the script, those before will get ignored - furthermore, they will get ignored silently if they happen to be the same number of arguments - furthermore, the old include_path and coverage_prefixes will be leaked In general I would prefer to always use new variables for the compat parsing, always warn when the new variables are set, and concatenating the two sets of arguments.
Review of attachment 338865 [details] [review]: ::: gjs/console.cpp @@ +230,3 @@ + env_coverage_prefixes = g_getenv("GJS_COVERAGE_PREFIXES"); + if (env_coverage_prefixes != NULL) { + coverage_output_path = g_strdup(env_coverage_output_path); This can be moved inside the else block below. @@ +237,3 @@ + unsigned new_nprefixes = g_strv_length(env_prefixes); + char **new_coverage_prefixes = + if (env_coverage_prefixes != NULL) { I find it odd that GJS_COVERAGE_PREFIXES is combined with those passed on the command line... Usually environment variables completely override the behavior instead of adding to the default.
Created attachment 338917 [details] [review] console: Check script args for stray GJS args In order to ease the pain of migration, check to see if any --include-path, --coverage-prefix, or --coverage-output arguments are being passed to the JS script. If so, treat them as if they had been passed to GJS while issuing a warning that this behaviour will go away in the future.
Created attachment 338918 [details] [review] console: Env vars for coverage args Encourage use of environment variables for coverage command line arguments, GJS_COVERAGE_PREFIXES instead of --coverage-prefix, and GJS_COVERAGE_OUTPUT instead of --coverage-output. This is because we want to pass arguments occurring after the script on the command line as arguments to the script and not to GJS. Without environment variables, it would otherwise be impossible to turn on coverage, for example, for a script that started with #!/usr/bin/gjs.
(In reply to Cosimo Cecchi from comment #15) > Review of attachment 338864 [details] [review] [review]: > > ::: gjs/console.cpp > @@ +97,3 @@ > + > + if (coverage_prefixes != NULL && > + int ix, argc_copy = argc + 1; > > I think this approach has a couple of issues: > - in case the arguments are both before and after the script, those before > will get ignored For --coverage-output I think that's OK, since you can only have one coverage output, it seems logical to have the rightmost one on the command line take precedence. > - furthermore, they will get ignored silently if they happen to be the same > number of arguments > - furthermore, the old include_path and coverage_prefixes will be leaked > > In general I would prefer to always use new variables for the compat > parsing, always warn when the new variables are set, and concatenating the > two sets of arguments. I assumed that storing pointers to non-NULL strvs in GOptionEntry would reallocate and append to them instead of overwriting and leaking them, but if that is not the case, then your way is better.
Review of attachment 338917 [details] [review]: Thanks, looks good now.
Review of attachment 338918 [details] [review]: Looks good.
Attachment 338917 [details] pushed as 27feb41 - console: Check script args for stray GJS args Attachment 338918 [details] pushed as a8887e3 - console: Env vars for coverage args
Created attachment 339894 [details] [review] console: Fix wrong cast in coverage prefixes I'm not sure what I was thinking here, but env_coverage_prefixes is a string, not a strv, and needs to be split up.
Reopening again, I found a bug in my code
Review of attachment 339894 [details] [review]: ::: gjs/console.cpp @@ +259,2 @@ if (coverage_prefixes == NULL) { + coverage_prefixes = g_strdupv(env_prefixes); Is env_prefixes not leaked in this case?
Created attachment 339966 [details] [review] console: Fix wrong cast in coverage prefixes I'm not sure what I was thinking here, but env_coverage_prefixes is a string, not a strv, and needs to be split up.
(In reply to Cosimo Cecchi from comment #25) > Review of attachment 339894 [details] [review] [review]: > > ::: gjs/console.cpp > @@ +259,2 @@ > if (coverage_prefixes == NULL) { > + coverage_prefixes = g_strdupv(env_prefixes); > > Is env_prefixes not leaked in this case? Yes, I realized it could be made even simpler as well
Review of attachment 339966 [details] [review]: Looks good now.
Attachment 339966 [details] pushed as 735d816 - console: Fix wrong cast in coverage prefixes