Bug 189058 - [WPE][GTK] webkit-flatpak intercepts --help for other commands
Summary: [WPE][GTK] webkit-flatpak intercepts --help for other commands
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Thibault Saunier
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-08-28 14:54 PDT by Michael Catanzaro
Modified: 2018-09-20 09:12 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.39 KB, patch)
2018-09-20 05:33 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff
Patch for landing (2.40 KB, patch)
2018-09-20 08:34 PDT, Thibault Saunier
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2018-08-28 14:54:06 PDT
$ run-webkit-tests --help
usage: webkit-flatpak [-h] [--verbose] [--debug] [--release]
                      [--platform PLATFORM] [--gtk] [--wpe] [-nf] [-u] [-b]
                      [-ba] [-q] [-t ...] [-c ...] [-y] [--avalaible]
                      [--gdb [GDB]] [-m COREDUMPCTL_MATCHES]
                      [--makeargs MAKEARGS] [--cmakeargs CMAKEARGS] [--clean]

optional arguments:
  -h, --help            show this help message and exit

General:
  --verbose             Show debug message
  --debug               Compile with Debug configuration, also installs Sdk
                        debug symboles.
  --release             Compile with Release configuration.
  --platform PLATFORM   Platform to use (e.g., "mac-lion")
  --gtk                 Alias for --platform=gtk
  --wpe                 Alias for --platform=wpe
  -nf, --no-flatpak-update
                        Do not update flaptak runtime/sdk
  -u, --update          Update the runtime/sdk/app and rebuild the development
                        environment if needed
  -b, --build-webkit    Force rebuilding the app.
  -ba, --build-all      Force rebuilding the app and its dependencies.
  -q, --quiet           Do not print anything
  -t ..., --tests ...   Run LayoutTests
  -c ..., --command ...
                        The command to run in the sandbox
  -y, --assumeyes       Automatically answer yes for all questions.
  --avalaible           Check if required dependencies are avalaible.
  --clean               Clean previous builds and restart from scratch

Debugging:
  --gdb [GDB]           Activate gdb, passing extra args to it if wanted.
  -m COREDUMPCTL_MATCHES, --coredumpctl-matches COREDUMPCTL_MATCHES
                        Arguments to pass to gdb.

Extra build arguments:
  --makeargs MAKEARGS   Optional Makefile flags
  --cmakeargs CMAKEARGS
                        One or more optional CMake flags (e.g.
                        --cmakeargs="-DFOO=bar
                        -DCMAKE_PREFIX_PATH=/usr/local")


Expected behavior: I see the help for run-webkit-tests :)
Actual behavior: I see the help for webkit-flatpak :(
Comment 1 Alicia Boya García 2018-09-12 11:19:55 PDT
This kind of bug usually occurs when someone passes user-provided arguments to a wrapper without a "--" prefix.
Comment 2 Alicia Boya García 2018-09-12 13:47:15 PDT
run-webkit-tests:
flatpakutils.run_in_sandbox_if_available(sys.argv)

|
v

flatpakutils.py:
def run_in_sandbox_if_available(args):
    [...]
    flatpak_runner = WebkitFlatpak.load_from_args(args)

|
v

flatpakutils.py:
@staticmethod
def load_from_args(args=None):
    [...] # creates a webkit-flatpak's ArgumentParser
    _, self.args = parser.parse_known_args(args=args, namespace=self)

What is `args` in run_in_sandbox_if_available() supposed to be? On the first invocation I would assume it is the command to be executed, but it's (also?) being parsed as the flags to webkit-flatpak. That's strange to say the least.

It seems like the code is trying to mix flatpak flags with the flags for the wrapped executable (we could call it the «flatpakee»?). This was probably intended to make it easy to set flags for both, but it's quite dangerous and as we are seeing now, can't cope with conflicts.

I think run-webkit-tests is doing too much, acting as both the wrapper and the wrapped, and that makes it confusing and creates these conflicts.
Comment 3 Thibault Saunier 2018-09-20 05:33:49 PDT
Created attachment 350189 [details]
Patch
Comment 4 Michael Catanzaro 2018-09-20 08:24:49 PDT
Comment on attachment 350189 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350189&action=review

> Tools/ChangeLog:11
> +        2. exists the app - The solution is to just make sure that `--help` is not used when using

exits
Comment 5 Thibault Saunier 2018-09-20 08:34:58 PDT
Created attachment 350204 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2018-09-20 09:12:38 PDT
Comment on attachment 350204 [details]
Patch for landing

Clearing flags on attachment: 350204

Committed r236261: <https://trac.webkit.org/changeset/236261>
Comment 7 WebKit Commit Bot 2018-09-20 09:12:39 PDT
All reviewed patches have been landed.  Closing bug.