Make the --release/--dev more consistent and less surprising (#30091)

There were some issues with the way that the `--release` and `--dev`
arguments were handled in mach commands.

 - Not all commands accepted them in the same way. For instance `./mach
   test-wpt` didn't really accept them at all.
 - If you did not pass either of them, mach would try to guess which
   build you meant. This guess was often quite surprising as it wasn't
   printed and it depended on the state of the your target directory,
   which is difficult to remember.
 - The `dev` profile is colloquially called a "debug" profile and some
   commands accepted `-d` or `--debug...` like arguments, but `--debug`
   with `./mach run` meant run in a debugger. It was easy to mix this
   up.

This change:

 - Centralizes where build type argument processing happens. Now it the
   same shared decorator in CommandBase.
 - Uses a `BuildType` enum instead of passing around two different
   booleans. This reduces the error checking for situations where both
   are true.
 - Be much less clever about guessing what build to use. Now if you
   don't specify a build type, `--dev` is chosen. I think this behavior
   matches cargo.
 - Makes it so that `./mach test-wpt` accepts the exact same arguments
   and has the same behavior as other commands. In addition, the suite
   correct for `test-wpt` is removed. There are only two suites now and
   it's quite unlikely that people will confuse WPT tests for rust unit
   tests.

.github/workflows/linux.yml

@@ -80,7 +80,7 @@ jobs:
       - name: Release build
         run: python3 ./mach build --release
       - name: Smoketest
-        run: xvfb-run python3 ./mach smoketest
+        run: xvfb-run python3 ./mach smoketest --release
       - name: Script tests
         run: ./mach test-scripts
       - name: Unit tests

.github/workflows/mac-wpt.yml

@@ -42,7 +42,7 @@ jobs:
           python3 -m pip install --upgrade pip virtualenv
           python3 ./mach bootstrap
       - name: Smoketest
-        run: python3 ./mach smoketest
+        run: python3 ./mach smoketest --release
       - name: Run tests
         run: |
           python3 ./mach test-wpt --with-${{ inputs.layout }} \

.github/workflows/mac.yml

@@ -69,7 +69,7 @@ jobs:
         run: |
           python3 ./mach build --release
       - name: Smoketest
-        run: python3 ./mach smoketest
+        run: python3 ./mach smoketest --release
       - name: Script tests
         run: ./mach test-scripts
       - name: Unit tests

.github/workflows/nightly-rust.yml

@@ -30,6 +30,6 @@ jobs:
       - name: Release build
         run: python3 ./mach build --release
       - name: Smoketest
-        run: xvfb-run python3 ./mach smoketest
+        run: xvfb-run python3 ./mach smoketest --release
       - name: Unit tests
         run: python3 ./mach test-unit --release

.github/workflows/windows.yml

@@ -67,7 +67,7 @@ jobs:
       - name: Copy resources
         run: cp D:\a\servo\servo\resources C:\a\servo\servo -Recurse
       - name: Smoketest
-        run: python mach smoketest --angle
+        run: python mach smoketest --angle --release
       - name: Unit tests
         if: ${{ inputs.unit-tests || github.ref_name == 'try-windows' }}
         run: python mach test-unit --release

python/servo/build_commands.py

@@ -35,21 +35,13 @@ from mach.registrar import Registrar
 import servo.platform
 import servo.util
 
-from servo.command_base import CommandBase, call, check_call
+from servo.command_base import BuildType, CommandBase, call, check_call
 from servo.gstreamer import windows_dlls, windows_plugins, macos_plugins
 
 
 @CommandProvider
 class MachCommands(CommandBase):
-    @Command('build',
-             description='Build Servo',
-             category='build')
-    @CommandArgument('--release', '-r',
-                     action='store_true',
-                     help='Build in release mode')
-    @CommandArgument('--dev', '-d',
-                     action='store_true',
-                     help='Build in development mode')
+    @Command('build', description='Build Servo', category='build')
     @CommandArgument('--jobs', '-j',
                      default=None,
                      help='Number of jobs to run in parallel')
@@ -64,41 +56,14 @@ class MachCommands(CommandBase):
                      help='Print very verbose output')
     @CommandArgument('params', nargs='...',
                      help="Command-line arguments to be passed through to Cargo")
-    @CommandBase.build_like_command_arguments
-    def build(self, release=False, dev=False, jobs=None, params=None, no_package=False,
+    @CommandBase.common_command_arguments(build_configuration=True, build_type=True)
+    def build(self, build_type: BuildType, jobs=None, params=None, no_package=False,
               verbose=False, very_verbose=False, libsimpleservo=False, **kwargs):
         opts = params or []
         has_media_stack = "media-gstreamer" in self.features
 
-        release_path = path.join(self.target_path, "release", "servo")
-        dev_path = path.join(self.target_path, "debug", "servo")
-
-        release_exists = path.exists(release_path)
-        dev_exists = path.exists(dev_path)
-
-        if not (release or dev):
-            if self.config["build"]["mode"] == "dev":
-                dev = True
-            elif self.config["build"]["mode"] == "release":
-                release = True
-            elif release_exists and not dev_exists:
-                release = True
-            elif dev_exists and not release_exists:
-                dev = True
-            else:
-                print("Please specify either --dev (-d) for a development")
-                print("  build, or --release (-r) for an optimized build.")
-                sys.exit(1)
-
-        if release and dev:
-            print("Please specify either --dev or --release.")
-            sys.exit(1)
-
-        if release:
+        if build_type == BuildType.RELEASE:
             opts += ["--release"]
-            servo_path = release_path
-        else:
-            servo_path = dev_path
 
         if jobs is not None:
             opts += ["-j", jobs]
@@ -176,15 +141,14 @@ class MachCommands(CommandBase):
                     flavor = "googlevr"
                 elif "oculusvr" in self.features:
                     flavor = "oculusvr"
-                rv = Registrar.dispatch("package", context=self.context,
-                                        release=release, dev=dev, target=self.cross_compile_target,
-                                        flavor=flavor)
+                rv = Registrar.dispatch("package", context=self.context, build_type=build_type,
+                                        target=self.cross_compile_target, flavor=flavor)
                 if rv:
                     return rv
 
             if sys.platform == "win32":
                 servo_exe_dir = os.path.dirname(
-                    self.get_binary_path(release, dev, target=self.cross_compile_target, simpleservo=libsimpleservo)
+                    self.get_binary_path(build_type, target=self.cross_compile_target, simpleservo=libsimpleservo)
                 )
                 assert os.path.exists(servo_exe_dir)
 
@@ -224,7 +188,7 @@ class MachCommands(CommandBase):
 
             elif sys.platform == "darwin":
                 servo_path = self.get_binary_path(
-                    release, dev, target=self.cross_compile_target, simpleservo=libsimpleservo)
+                    build_type, target=self.cross_compile_target, simpleservo=libsimpleservo)
                 servo_bin_dir = os.path.dirname(servo_path)
                 assert os.path.exists(servo_bin_dir)
 

python/servo/command_base.py

@@ -10,6 +10,7 @@
 from __future__ import print_function
 
 import contextlib
+from enum import Enum
 from typing import Dict, List, Optional
 import functools
 import gzip
@@ -42,10 +43,15 @@ import servo.platform
 import servo.util as util
 from servo.util import download_file, get_default_cache_dir
 
-BIN_SUFFIX = ".exe" if sys.platform == "win32" else ""
 NIGHTLY_REPOSITORY_URL = "https://servo-builds2.s3.amazonaws.com/"
 
 
+class BuildType(Enum):
+    """ The build type of this Servo build. Either `DEV` or `RELEASE`."""
+    DEV = 1
+    RELEASE = 2
+
+
 @contextlib.contextmanager
 def cd(new_path):
     """Context manager for changing the current working directory"""
@@ -281,26 +287,22 @@ class CommandBase(object):
     def get_top_dir(self):
         return self.context.topdir
 
-    def get_apk_path(self, release):
+    def get_apk_path(self, build_type: BuildType):
         base_path = util.get_target_dir()
         base_path = path.join(base_path, "android", self.config["android"]["target"])
         apk_name = "servoapp.apk"
-        build_type = "release" if release else "debug"
-        return path.join(base_path, build_type, apk_name)
+        build_type_string = "release" if build_type == BuildType.RELEASE else "debug"
+        return path.join(base_path, build_type_string, apk_name)
 
-    def get_binary_path(self, release, dev, target=None, android=False, simpleservo=False):
-        # TODO(autrilla): this function could still use work - it shouldn't
-        # handle quitting, or printing. It should return the path, or an error.
+    def get_binary_path(self, build_type: BuildType, target=None, android=False, simpleservo=False):
         base_path = util.get_target_dir()
-
-        binary_name = "servo" + BIN_SUFFIX
-
         if android:
             base_path = path.join(base_path, "android", self.config["android"]["target"])
             simpleservo = True
         elif target:
             base_path = path.join(base_path, target)
 
+        binary_name = f"servo{servo.platform.get().executable_suffix()}"
         if simpleservo:
             if sys.platform == "win32":
                 binary_name = "simpleservo.dll"
@@ -309,41 +311,12 @@ class CommandBase(object):
             else:
                 binary_name = "libsimpleservo.so"
 
-        release_path = path.join(base_path, "release", binary_name)
-        dev_path = path.join(base_path, "debug", binary_name)
+        build_type_string = "release" if build_type == BuildType.RELEASE else "debug"
+        binary_path = path.join(base_path, build_type_string, binary_name)
 
-        # Prefer release if both given
-        if release and dev:
-            dev = False
-
-        release_exists = path.exists(release_path)
-        dev_exists = path.exists(dev_path)
-
-        if not release_exists and not dev_exists:
-            raise BuildNotFound('No Servo binary found.'
-                                ' Perhaps you forgot to run `./mach build`?')
-
-        if release and release_exists:
-            return release_path
-
-        if dev and dev_exists:
-            return dev_path
-
-        if not dev and not release and release_exists and dev_exists:
-            print("You have multiple profiles built. Please specify which "
-                  "one to run with '--release' or '--dev'.")
-            sys.exit()
-
-        if not dev and not release:
-            if release_exists:
-                return release_path
-            else:
-                return dev_path
-
-        print("The %s profile is not built. Please run './mach build%s' "
-              "and try again." % ("release" if release else "dev",
-                                  " --release" if release else ""))
-        sys.exit()
+        if not path.exists(binary_path):
+            raise BuildNotFound('No Servo binary found. Perhaps you forgot to run `./mach build`?')
+        return binary_path
 
     def detach_volume(self, mounted_volume):
         print("Detaching volume {}".format(mounted_volume))
@@ -739,72 +712,115 @@ class CommandBase(object):
         env['PKG_CONFIG_ALLOW_CROSS'] = "1"
 
     @staticmethod
-    def build_like_command_arguments(original_function):
-        decorators = [
-            CommandArgumentGroup('Cross Compilation'),
-            CommandArgument(
-                '--target', '-t',
-                group="Cross Compilation",
-                default=None,
-                help='Cross compile for given target platform',
-            ),
-            CommandArgument(
-                '--android', default=None, action='store_true',
-                help='Build for Android. If --target is not specified, this '
-                     'will choose a default target architecture.',
-            ),
-            CommandArgument('--win-arm64', action='store_true', help="Use arm64 Windows target"),
-            CommandArgumentGroup('Feature Selection'),
-            CommandArgument(
-                '--features', default=None, group="Feature Selection", nargs='+',
-                help='Space-separated list of features to also build',
-            ),
-            CommandArgument(
-                '--media-stack', default=None, group="Feature Selection",
-                choices=["gstreamer", "dummy"], help='Which media stack to use',
-            ),
-            CommandArgument(
-                '--libsimpleservo',
-                default=None,
-                group="Feature Selection",
-                action='store_true',
-                help='Build the libsimpleservo library instead of the servo executable',
-            ),
-            CommandArgument(
-                '--debug-mozjs',
-                default=False,
-                group="Feature Selection",
-                action='store_true',
-                help='Enable debug assertions in mozjs',
-            ),
-            CommandArgument(
-                '--with-debug-assertions',
-                default=False,
-                group="Feature Selection",
-                action='store_true',
-                help='Enable debug assertions in release',
-            ),
-            CommandArgument(
-                '--with-frame-pointer',
-                default=None, group="Feature Selection",
-                action='store_true',
-                help='Build with frame pointer enabled, used by the background hang monitor.',
-            ),
-            CommandArgument('--without-wgl', group="Feature Selection", default=None, action='store_true'),
-        ]
+    def common_command_arguments(build_configuration=False, build_type=False):
+        decorators = []
+        if build_type:
+            decorators += [
+                CommandArgumentGroup('Build Type'),
+                CommandArgument('--release', '-r', group="Build Type",
+                                action='store_true',
+                                help='Build in release mode'),
+                CommandArgument('--dev', '--debug', '-d', group="Build Type",
+                                action='store_true',
+                                help='Build in development mode'),
+            ]
 
-        def configuration_decorator(self, *args, **kwargs):
-            self.configure_cross_compilation(kwargs['target'], kwargs['android'], kwargs['win_arm64'])
+        if build_configuration:
+            decorators += [
+                CommandArgumentGroup('Cross Compilation'),
+                CommandArgument(
+                    '--target', '-t',
+                    group="Cross Compilation",
+                    default=None,
+                    help='Cross compile for given target platform',
+                ),
+                CommandArgument(
+                    '--android', default=None, action='store_true',
+                    help='Build for Android. If --target is not specified, this '
+                         'will choose a default target architecture.',
+                ),
+                CommandArgument('--win-arm64', action='store_true', help="Use arm64 Windows target"),
+                CommandArgumentGroup('Feature Selection'),
+                CommandArgument(
+                    '--features', default=None, group="Feature Selection", nargs='+',
+                    help='Space-separated list of features to also build',
+                ),
+                CommandArgument(
+                    '--media-stack', default=None, group="Feature Selection",
+                    choices=["gstreamer", "dummy"], help='Which media stack to use',
+                ),
+                CommandArgument(
+                    '--libsimpleservo',
+                    default=None,
+                    group="Feature Selection",
+                    action='store_true',
+                    help='Build the libsimpleservo library instead of the servo executable',
+                ),
+                CommandArgument(
+                    '--debug-mozjs',
+                    default=False,
+                    group="Feature Selection",
+                    action='store_true',
+                    help='Enable debug assertions in mozjs',
+                ),
+                CommandArgument(
+                    '--with-debug-assertions',
+                    default=False,
+                    group="Feature Selection",
+                    action='store_true',
+                    help='Enable debug assertions in release',
+                ),
+                CommandArgument(
+                    '--with-frame-pointer',
+                    default=None, group="Feature Selection",
+                    action='store_true',
+                    help='Build with frame pointer enabled, used by the background hang monitor.',
+                ),
+                CommandArgument('--without-wgl', group="Feature Selection", default=None, action='store_true'),
+            ]
 
-            self.features = kwargs.get("features", None) or []
-            self.configure_media_stack(kwargs['media_stack'])
-            return original_function(self, *args, **kwargs)
+        def decorator_function(original_function):
+            def configuration_decorator(self, *args, **kwargs):
+                if build_type:
+                    # If `build_type` already exists in kwargs we are doing a recursive dispatch.
+                    if 'build_type' not in kwargs:
+                        kwargs['build_type'] = self.configure_build_type(kwargs['release'], kwargs['dev'])
+                    kwargs.pop('release', None)
+                    kwargs.pop('dev', None)
 
-        decorators.reverse()
-        decorated_function = configuration_decorator
-        for decorator in decorators:
-            decorated_function = decorator(decorated_function)
-        return decorated_function
+                if build_configuration:
+                    self.configure_cross_compilation(kwargs['target'], kwargs['android'], kwargs['win_arm64'])
+                    self.features = kwargs.get("features", None) or []
+                    self.configure_media_stack(kwargs['media_stack'])
+
+                return original_function(self, *args, **kwargs)
+
+            decorators.reverse()
+            for decorator in decorators:
+                decorator(configuration_decorator)
+
+            return configuration_decorator
+
+        return decorator_function
+
+    def configure_build_type(self, release: bool, dev: bool) -> BuildType:
+        if release and dev:
+            print("Please specify either --dev (-d) for a development")
+            print("  build, or --release (-r) for an optimized build.")
+            sys.exit(1)
+
+        if not release and not dev:
+            if self.config["build"]["mode"] == "dev":
+                print("No build type specified, but .servobuild specified `--dev`.")
+                dev = True
+            elif self.config["build"]["mode"] == "release":
+                print("No build type specified, but .servobuild specified `--release`.")
+                release = True
+            else:
+                print("No build type specified so assuming `--dev`.")
+                dev = True
+
+        return BuildType.DEV if dev else BuildType.RELEASE
 
     def configure_cross_compilation(
             self,
@@ -972,7 +988,7 @@ class CommandBase(object):
     def ensure_rustup_version(self):
         try:
             version_line = subprocess.check_output(
-                ["rustup" + BIN_SUFFIX, "--version"],
+                ["rustup" + servo.platform.get().executable_suffix(), "--version"],
                 # Silence "info: This is the version for the rustup toolchain manager,
                 # not the rustc compiler."
                 stderr=open(os.devnull, "wb")

python/servo/devenv_commands.py

@@ -44,7 +44,7 @@ class MachCommands(CommandBase):
     @CommandArgument(
         'params', default=None, nargs='...',
         help="Command-line arguments to be passed through to cargo check")
-    @CommandBase.build_like_command_arguments
+    @CommandBase.common_command_arguments(build_configuration=True, build_type=False)
     def check(self, params, **kwargs):
         if not params:
             params = []
@@ -129,7 +129,7 @@ class MachCommands(CommandBase):
     @CommandArgument(
         'params', default=None, nargs='...',
         help="Command-line arguments to be passed through to cargo-fix")
-    @CommandBase.build_like_command_arguments
+    @CommandBase.common_command_arguments(build_configuration=True, build_type=False)
     def cargo_fix(self, params, **kwargs):
         if not params:
             params = []
@@ -144,7 +144,7 @@ class MachCommands(CommandBase):
     @CommandArgument(
         'params', default=None, nargs='...',
         help="Command-line arguments to be passed through to cargo-clippy")
-    @CommandBase.build_like_command_arguments
+    @CommandBase.common_command_arguments(build_configuration=True, build_type=False)
     def cargo_clippy(self, params, **kwargs):
         if not params:
             params = []

python/servo/package_commands.py

@@ -29,6 +29,7 @@ from mach.decorators import (
 from mach.registrar import Registrar
 
 from servo.command_base import (
+    BuildType,
     archive_deterministically,
     BuildNotFound,
     cd,
@@ -112,10 +113,6 @@ class PackageCommands(CommandBase):
     @Command('package',
              description='Package Servo',
              category='package')
-    @CommandArgument('--release', '-r', action='store_true',
-                     help='Package the release build')
-    @CommandArgument('--dev', '-d', action='store_true',
-                     help='Package the dev build')
     @CommandArgument('--android',
                      default=None,
                      action='store_true',
@@ -130,8 +127,8 @@ class PackageCommands(CommandBase):
                      default=None,
                      action='store_true',
                      help='Create a local Maven repository')
-    def package(self, release=False, dev=False, android=None, target=None,
-                flavor=None, maven=False):
+    @CommandBase.common_command_arguments(build_configuration=False, build_type=True)
+    def package(self, build_type: BuildType, android=None, target=None, flavor=None, maven=False):
         if android is None:
             android = self.config["build"]["android"]
         if target and android:
@@ -144,26 +141,21 @@ class PackageCommands(CommandBase):
 
         self.cross_compile_target = target
         env = self.build_env()
-        binary_path = self.get_binary_path(
-            release, dev, target=target, android=android,
-        )
+        binary_path = self.get_binary_path(build_type, target=target, android=android)
         dir_to_root = self.get_top_dir()
         target_dir = path.dirname(binary_path)
         if android:
             android_target = self.config["android"]["target"]
             if "aarch64" in android_target:
-                build_type = "Arm64"
+                arch_string = "Arm64"
             elif "armv7" in android_target:
-                build_type = "Armv7"
+                arch_string = "Armv7"
             elif "i686" in android_target:
-                build_type = "x86"
+                arch_string = "x86"
             else:
-                build_type = "Arm"
+                arch_string = "Arm"
 
-            if dev:
-                build_mode = "Debug"
-            else:
-                build_mode = "Release"
+            build_type_string = "Debug" if build_type == BuildType.DEV else "Release"
 
             flavor_name = "Main"
             if flavor is not None:
@@ -178,7 +170,7 @@ class PackageCommands(CommandBase):
             shutil.copytree(path.join(dir_to_root, 'resources'), dir_to_resources)
             change_prefs(dir_to_resources, "android", vr=vr)
 
-            variant = ":assemble" + flavor_name + build_type + build_mode
+            variant = ":assemble" + flavor_name + arch_string + build_type_string
             apk_task_name = ":servoapp" + variant
             aar_task_name = ":servoview" + variant
             maven_task_name = ":servoview:uploadArchive"
@@ -354,10 +346,6 @@ class PackageCommands(CommandBase):
     @Command('install',
              description='Install Servo (currently, Android and Windows only)',
              category='package')
-    @CommandArgument('--release', '-r', action='store_true',
-                     help='Install the release build')
-    @CommandArgument('--dev', '-d', action='store_true',
-                     help='Install the dev build')
     @CommandArgument('--android',
                      action='store_true',
                      help='Install on Android')
@@ -370,7 +358,8 @@ class PackageCommands(CommandBase):
     @CommandArgument('--target', '-t',
                      default=None,
                      help='Install the given target platform')
-    def install(self, release=False, dev=False, android=False, emulator=False, usb=False, target=None):
+    @CommandBase.common_command_arguments(build_configuration=False, build_type=True)
+    def install(self, build_type: BuildType, android=False, emulator=False, usb=False, target=None):
         if target and android:
             print("Please specify either --target or --android.")
             sys.exit(1)
@@ -380,21 +369,21 @@ class PackageCommands(CommandBase):
 
         env = self.build_env()
         try:
-            binary_path = self.get_binary_path(release, dev, android=android)
+            binary_path = self.get_binary_path(build_type, android=android)
         except BuildNotFound:
             print("Servo build not found. Building servo...")
             result = Registrar.dispatch(
-                "build", context=self.context, release=release, dev=dev, android=android,
+                "build", context=self.context, build_type=build_type, android=android,
             )
             if result:
                 return result
             try:
-                binary_path = self.get_binary_path(release, dev, android=android)
+                binary_path = self.get_binary_path(build_type, android=android)
             except BuildNotFound:
                 print("Rebuilding Servo did not solve the missing build problem.")
                 return 1
         if android:
-            pkg_path = self.get_apk_path(release)
+            pkg_path = self.get_apk_path(build_type)
             exec_command = [self.android_adb_path(env)]
             if emulator and usb:
                 print("Cannot install to both emulator and USB at the same time.")
@@ -411,7 +400,7 @@ class PackageCommands(CommandBase):
         if not path.exists(pkg_path):
             print("Servo package not found. Packaging servo...")
             result = Registrar.dispatch(
-                "package", context=self.context, release=release, dev=dev, android=android,
+                "package", context=self.context, build_type=build_type, android=android,
             )
             if result != 0:
                 return result

python/servo/post_build_commands.py

@@ -16,7 +16,7 @@ import subprocess
 from shutil import copytree, rmtree, copy2
 from typing import List
 
-import servo.util
+import mozdebug
 
 from mach.decorators import (
     CommandArgument,
@@ -24,9 +24,13 @@ from mach.decorators import (
     Command,
 )
 
+import servo.util
+import servo.platform
+
 from servo.command_base import (
+    BuildType,
     CommandBase,
-    check_call, check_output, BIN_SUFFIX,
+    check_call, check_output,
     is_linux,
 )
 
@@ -50,10 +54,6 @@ class PostBuildCommands(CommandBase):
     @Command('run',
              description='Run Servo',
              category='post-build')
-    @CommandArgument('--release', '-r', action='store_true',
-                     help='Run the release build')
-    @CommandArgument('--dev', '-d', action='store_true',
-                     help='Run the dev build')
     @CommandArgument('--android', action='store_true', default=None,
                      help='Run on an Android device through `adb shell`')
     @CommandArgument('--emulator',
@@ -62,12 +62,12 @@ class PostBuildCommands(CommandBase):
     @CommandArgument('--usb',
                      action='store_true',
                      help='For Android, run in the only USB device')
-    @CommandArgument('--debug', action='store_true',
+    @CommandArgument('--debugger', action='store_true',
                      help='Enable the debugger. Not specifying a '
-                          '--debugger option will result in the default '
+                          '--debugger-cmd option will result in the default '
                           'debugger being used. The following arguments '
                           'have no effect without this.')
-    @CommandArgument('--debugger', default=None, type=str,
+    @CommandArgument('--debugger-cmd', default=None, type=str,
                      help='Name of debugger to use.')
     @CommandArgument('--headless', '-z', action='store_true',
                      help='Launch in headless mode')
@@ -80,7 +80,8 @@ class PostBuildCommands(CommandBase):
     @CommandArgument(
         'params', nargs='...',
         help="Command-line arguments to be passed through to Servo")
-    def run(self, params, release=False, dev=False, android=None, debug=False, debugger=None,
+    @CommandBase.common_command_arguments(build_configuration=False, build_type=True)
+    def run(self, params, build_type: BuildType, android=None, debugger=False, debugger_cmd=None,
             headless=False, software=False, bin=None, emulator=False, usb=False, nightly=None):
         env = self.build_env()
         env["RUST_BACKTRACE"] = "1"
@@ -92,15 +93,15 @@ class PostBuildCommands(CommandBase):
             env['LIBGL_ALWAYS_SOFTWARE'] = "1"
         os.environ.update(env)
 
-        # Make --debugger imply --debug
-        if debugger:
-            debug = True
+        # Make --debugger-cmd imply --debugger
+        if debugger_cmd:
+            debugger = True
 
         if android is None:
             android = self.config["build"]["android"]
 
         if android:
-            if debug:
+            if debugger:
                 print("Android on-device debugging is not supported by mach yet. See")
                 print("https://github.com/servo/servo/wiki/Building-for-Android#debugging-on-device")
                 return
@@ -133,55 +134,54 @@ class PostBuildCommands(CommandBase):
             shell.communicate("\n".join(script) + "\n")
             return shell.wait()
 
-        args = [bin or self.get_nightly_binary_path(nightly) or self.get_binary_path(release, dev)]
+        args = [bin or self.get_nightly_binary_path(nightly) or self.get_binary_path(build_type)]
 
         if headless:
             args.append('-z')
 
         # Borrowed and modified from:
         # http://hg.mozilla.org/mozilla-central/file/c9cfa9b91dea/python/mozbuild/mozbuild/mach_commands.py#l883
-        if debug:
-            import mozdebug
-            if not debugger:
+        if debugger:
+            if not debugger_cmd:
                 # No debugger name was provided. Look for the default ones on
                 # current OS.
-                debugger = mozdebug.get_default_debugger_name(
+                debugger_cmd = mozdebug.get_default_debugger_name(
                     mozdebug.DebuggerSearch.KeepLooking)
 
-            self.debuggerInfo = mozdebug.get_debugger_info(debugger)
-            if not self.debuggerInfo:
+            debugger_info = mozdebug.get_debugger_info(debugger_cmd)
+            if not debugger_info:
                 print("Could not find a suitable debugger in your PATH.")
                 return 1
 
-            command = self.debuggerInfo.path
-            if debugger == 'gdb' or debugger == 'lldb':
-                rustCommand = 'rust-' + debugger
+            command = debugger_info.path
+            if debugger_cmd == 'gdb' or debugger_cmd == 'lldb':
+                rust_command = 'rust-' + debugger_cmd
                 try:
-                    subprocess.check_call([rustCommand, '--version'], env=env, stdout=open(os.devnull, 'w'))
+                    subprocess.check_call([rust_command, '--version'], env=env, stdout=open(os.devnull, 'w'))
                 except (OSError, subprocess.CalledProcessError):
                     pass
                 else:
-                    command = rustCommand
+                    command = rust_command
 
             # Prepend the debugger args.
-            args = ([command] + self.debuggerInfo.args + args + params)
+            args = ([command] + debugger_info.args + args + params)
         else:
             args = args + params
 
         try:
             check_call(args, env=env)
-        except subprocess.CalledProcessError as e:
-            if e.returncode < 0:
-                print(f"Servo was terminated by signal {-e.returncode}")
+        except subprocess.CalledProcessError as exception:
+            if exception.returncode < 0:
+                print(f"Servo was terminated by signal {-exception.returncode}")
             else:
-                print(f"Servo exited with non-zero status {e.returncode}")
-            return e.returncode
-        except OSError as e:
-            if e.errno == 2:
+                print(f"Servo exited with non-zero status {exception.returncode}")
+            return exception.returncode
+        except OSError as exception:
+            if exception.errno == 2:
                 print("Servo Binary can't be found! Run './mach build'"
                       " and try again!")
             else:
-                raise e
+                raise exception
 
     @Command('android-emulator',
              description='Run the Android emulator',
@@ -198,10 +198,6 @@ class PostBuildCommands(CommandBase):
     @Command('rr-record',
              description='Run Servo whilst recording execution with rr',
              category='post-build')
-    @CommandArgument('--release', '-r', action='store_true',
-                     help='Use release build')
-    @CommandArgument('--dev', '-d', action='store_true',
-                     help='Use dev build')
     @CommandArgument('--bin', default=None,
                      help='Launch with specific binary')
     @CommandArgument('--nightly', '-n', default=None,
@@ -209,12 +205,13 @@ class PostBuildCommands(CommandBase):
     @CommandArgument(
         'params', nargs='...',
         help="Command-line arguments to be passed through to Servo")
-    def rr_record(self, release=False, dev=False, bin=None, nightly=None, params=[]):
+    @CommandBase.common_command_arguments(build_configuration=False, build_type=True)
+    def rr_record(self, build_type: BuildType, bin=None, nightly=None, params=[]):
         env = self.build_env()
         env["RUST_BACKTRACE"] = "1"
 
         servo_cmd = [bin or self.get_nightly_binary_path(nightly)
-                     or self.get_binary_path(release, dev)] + params
+                     or self.get_binary_path(build_type)] + params
         rr_cmd = ['rr', '--fatal-errors', 'record']
         try:
             check_call(rr_cmd + servo_cmd)
@@ -242,11 +239,11 @@ class PostBuildCommands(CommandBase):
     @CommandArgument(
         'params', nargs='...',
         help="Command-line arguments to be passed through to cargo doc")
-    @CommandBase.build_like_command_arguments
+    @CommandBase.common_command_arguments(build_configuration=True, build_type=False)
     def doc(self, params: List[str], **kwargs):
         self.ensure_bootstrapped()
         rustc_path = check_output(
-            ["rustup" + BIN_SUFFIX, "which", "rustc"],
+            [f"rustup{servo.platform.get().executable_suffix()}", "which", "rustc"],
             cwd=self.context.topdir).decode("utf-8")
         assert path.basename(path.dirname(rustc_path)) == "bin"
         toolchain_path = path.dirname(path.dirname(rustc_path))

python/servo/testing_commands.py

@@ -33,10 +33,9 @@ from mach.decorators import (
     Command,
 )
 
-import servo.util
 import tidy
 
-from servo.command_base import CommandBase, call, check_call
+from servo.command_base import BuildType, CommandBase, call, check_call
 from servo.util import delete
 from distutils.dir_util import copy_tree
 
@@ -166,7 +165,7 @@ class MachCommands(CommandBase):
                      help="Run in bench mode")
     @CommandArgument('--nocapture', default=False, action="store_true",
                      help="Run tests with nocapture ( show test stdout )")
-    @CommandBase.build_like_command_arguments
+    @CommandBase.common_command_arguments(build_configuration=True, build_type=False)
     def test_unit(self, test_name=None, package=None, bench=False, nocapture=False, **kwargs):
         if test_name is None:
             test_name = []
@@ -242,13 +241,6 @@ class MachCommands(CommandBase):
         # We are setting is_build here to true, because running `cargo test` can trigger builds.
         env = self.build_env(is_build=True)
 
-        # on MSVC, we need some DLLs in the path. They were copied
-        # in to the servo.exe build dir, so just point PATH to that.
-        # TODO(mrobinson): This should be removed entirely.
-        if "msvc" in servo.platform.host_triple():
-            servo.util.prepend_paths_to_env(
-                env, "PATH", path.dirname(self.get_binary_path(False, False)))
-
         return self.run_cargo_build_like_command(
             "bench" if bench else "test",
             args,
@@ -332,57 +324,42 @@ class MachCommands(CommandBase):
              description='Run the tests harness that verifies that the test failures are reported correctly',
              category='testing',
              parser=wpt.create_parser)
-    def test_wpt_failure(self, **kwargs):
+    @CommandBase.common_command_arguments(build_configuration=False, build_type=True)
+    def test_wpt_failure(self, build_type: BuildType, **kwargs):
         kwargs["pause_after_test"] = False
         kwargs["include"] = ["infrastructure/failing-test.html"]
-        return not self._test_wpt(**kwargs)
+        return not self._test_wpt(build_type=build_type, **kwargs)
 
     @Command('test-wpt',
              description='Run the regular web platform test suite',
              category='testing',
              parser=wpt.create_parser)
-    def test_wpt(self, **kwargs):
-        ret = self.run_test_list_or_dispatch(kwargs["test_list"], "wpt", self._test_wpt, **kwargs)
-        if kwargs["always_succeed"]:
-            return 0
-        else:
-            return ret
+    @CommandBase.common_command_arguments(build_configuration=False, build_type=True)
+    def test_wpt(self, build_type: BuildType, **kwargs):
+        return self._test_wpt(build_type=build_type, **kwargs)
 
     @Command('test-wpt-android',
              description='Run the web platform test suite in an Android emulator',
              category='testing',
              parser=wpt.create_parser)
-    def test_wpt_android(self, release=False, dev=False, binary_args=None, **kwargs):
+    @CommandBase.common_command_arguments(build_configuration=False, build_type=True)
+    def test_wpt_android(self, build_type: BuildType, binary_args=None, **kwargs):
         kwargs.update(
-            release=release,
-            dev=dev,
             product="servodriver",
             processes=1,
-            binary_args=self.in_android_emulator(release, dev) + (binary_args or []),
+            binary_args=self.in_android_emulator(build_type) + (binary_args or []),
             binary=sys.executable,
         )
-        return self._test_wpt(android=True, **kwargs)
+        return self._test_wpt(build_type=build_type, android=True, **kwargs)
 
-    def _test_wpt(self, android=False, **kwargs):
+    def _test_wpt(self, build_type: BuildType, android=False, **kwargs):
         if not android:
             os.environ.update(self.build_env())
-        return wpt.run.run_tests(**kwargs)
 
-    # Helper to ensure all specified paths are handled, otherwise dispatch to appropriate test suite.
-    def run_test_list_or_dispatch(self, requested_paths, correct_suite, correct_function, **kwargs):
-        if not requested_paths:
-            return correct_function(**kwargs)
-        # Paths specified on command line. Ensure they can be handled, re-dispatch otherwise.
-        all_handled = True
-        for test_path in requested_paths:
-            suite = self.suite_for_path(test_path)
-            if suite is not None and correct_suite != suite:
-                all_handled = False
-                print("Warning: %s is not a %s test. Delegating to test-%s." % (test_path, correct_suite, suite))
-        if all_handled:
-            return correct_function(**kwargs)
-        # Dispatch each test to the correct suite via test()
-        Registrar.dispatch("test", context=self.context, params=requested_paths)
+        # TODO(mrobinson): Why do we pass the wrong binary path in when running WPT on Android?
+        binary_path = self.get_binary_path(build_type=build_type)
+        return_value = wpt.run.run_tests(binary_path, **kwargs)
+        return return_value if not kwargs["always_succeed"] else 0
 
     @Command('update-manifest',
              description='Run test-wpt --manifest-update SKIP_TESTS to regenerate MANIFEST.json',
@@ -411,18 +388,15 @@ class MachCommands(CommandBase):
     @Command('test-android-startup',
              description='Extremely minimal testing of Servo for Android',
              category='testing')
-    @CommandArgument('--release', '-r', action='store_true',
-                     help='Run the release build')
-    @CommandArgument('--dev', '-d', action='store_true',
-                     help='Run the dev build')
-    def test_android_startup(self, release, dev):
+    @CommandBase.common_command_arguments(build_configuration=False, build_type=True)
+    def test_android_startup(self, build_type: BuildType):
         html = """
             <script>
                 window.alert("JavaScript is running!")
             </script>
         """
         url = "data:text/html;base64," + html.encode("base64").replace("\n", "")
-        args = self.in_android_emulator(release, dev)
+        args = self.in_android_emulator(build_type)
         args = [sys.executable] + args + [url]
         process = subprocess.Popen(args, stdout=subprocess.PIPE)
         try:
@@ -437,11 +411,7 @@ class MachCommands(CommandBase):
         finally:
             process.terminate()
 
-    def in_android_emulator(self, release, dev):
-        if (release and dev) or not (release or dev):
-            print("Please specify one of --dev or --release.")
-            sys.exit(1)
-
+    def in_android_emulator(self, build_type: BuildType):
         avd = "servo-x86"
         target = "i686-linux-android"
         print("Assuming --target " + target)
@@ -450,42 +420,28 @@ class MachCommands(CommandBase):
         env = self.build_env()
         os.environ["PATH"] = env["PATH"]
         assert self.setup_configuration_for_android_target(target)
-        apk = self.get_apk_path(release)
+        apk = self.get_apk_path(build_type)
 
         py = path.join(self.context.topdir, "etc", "run_in_headless_android_emulator.py")
         return [py, avd, apk]
 
-    @Command('test-jquery',
-             description='Run the jQuery test suite',
-             category='testing')
-    @CommandArgument('--release', '-r', action='store_true',
-                     help='Run the release build')
-    @CommandArgument('--dev', '-d', action='store_true',
-                     help='Run the dev build')
-    def test_jquery(self, release, dev):
-        return self.jquery_test_runner("test", release, dev)
+    @Command('test-jquery', description='Run the jQuery test suite', category='testing')
+    @CommandBase.common_command_arguments(build_configuration=False, build_type=True)
+    def test_jquery(self, build_type: BuildType):
+        return self.jquery_test_runner("test", build_type)
 
-    @Command('test-dromaeo',
-             description='Run the Dromaeo test suite',
-             category='testing')
-    @CommandArgument('tests', default=["recommended"], nargs="...",
-                     help="Specific tests to run")
-    @CommandArgument('--release', '-r', action='store_true',
-                     help='Run the release build')
-    @CommandArgument('--dev', '-d', action='store_true',
-                     help='Run the dev build')
-    def test_dromaeo(self, tests, release, dev):
-        return self.dromaeo_test_runner(tests, release, dev)
+    @Command('test-dromaeo', description='Run the Dromaeo test suite', category='testing')
+    @CommandArgument('tests', default=["recommended"], nargs="...", help="Specific tests to run")
+    @CommandBase.common_command_arguments(build_configuration=False, build_type=True)
+    def test_dromaeo(self, tests, build_type: BuildType):
+        return self.dromaeo_test_runner(tests, build_type)
 
     @Command('update-jquery',
              description='Update the jQuery test suite expected results',
              category='testing')
-    @CommandArgument('--release', '-r', action='store_true',
-                     help='Run the release build')
-    @CommandArgument('--dev', '-d', action='store_true',
-                     help='Run the dev build')
-    def update_jquery(self, release, dev):
-        return self.jquery_test_runner("update", release, dev)
+    @CommandBase.common_command_arguments(build_configuration=False, build_type=True)
+    def update_jquery(self, build_type: BuildType):
+        return self.jquery_test_runner("update", build_type)
 
     @Command('compare_dromaeo',
              description='Compare outputs of two runs of ./mach test-dromaeo command',
@@ -543,7 +499,7 @@ class MachCommands(CommandBase):
                     print("{}|{}|{}|{}".format(a1.ljust(width_col1), str(b1).ljust(width_col2),
                           str(c1).ljust(width_col3), str(d1).ljust(width_col4)))
 
-    def jquery_test_runner(self, cmd, release, dev):
+    def jquery_test_runner(self, cmd, build_type: BuildType):
         base_dir = path.abspath(path.join("tests", "jquery"))
         jquery_dir = path.join(base_dir, "jquery")
         run_file = path.join(base_dir, "run_jquery.py")
@@ -558,11 +514,11 @@ class MachCommands(CommandBase):
             ["git", "-C", jquery_dir, "pull"])
 
         # Check that a release servo build exists
-        bin_path = path.abspath(self.get_binary_path(release, dev))
+        bin_path = path.abspath(self.get_binary_path(build_type))
 
         return call([run_file, cmd, bin_path, base_dir])
 
-    def dromaeo_test_runner(self, tests, release, dev):
+    def dromaeo_test_runner(self, tests, build_type: BuildType):
         base_dir = path.abspath(path.join("tests", "dromaeo"))
         dromaeo_dir = path.join(base_dir, "dromaeo")
         run_file = path.join(base_dir, "run_dromaeo.py")
@@ -581,7 +537,7 @@ class MachCommands(CommandBase):
             ["make", "-C", dromaeo_dir, "web"])
 
         # Check that a release servo build exists
-        bin_path = path.abspath(self.get_binary_path(release, dev))
+        bin_path = path.abspath(self.get_binary_path(build_type))
 
         return check_call(
             [run_file, "|".join(tests), bin_path, base_dir])
@@ -832,10 +788,10 @@ tests/wpt/mozilla/tests for Servo-only tests""" % reference_path)
              category='testing')
     @CommandArgument('params', nargs='...',
                      help="Command-line arguments to be passed through to Servo")
-    def smoketest(self, params):
+    @CommandBase.common_command_arguments(build_configuration=False, build_type=True)
+    def smoketest(self, build_type: BuildType, params):
         # We pass `-f` here so that any thread panic will cause Servo to exit,
         # preventing a panic from hanging execution. This means that these kind
         # of panics won't cause timeouts on CI.
-        params = params + ['-f', 'tests/html/close-on-load.html']
-        return self.context.commands.dispatch(
-            'run', self.context, params=params)
+        return self.context.commands.dispatch('run', self.context, build_type=build_type,
+                                              params=params + ['-f', 'tests/html/close-on-load.html'])

python/wpt/__init__.py

@@ -27,8 +27,6 @@ import wptrunner.wptcommandline  # noqa: E402
 
 def create_parser():
     parser = wptrunner.wptcommandline.create_parser()
-    parser.add_argument('--release', default=False, action="store_true",
-                        help="Run with a release build of servo")
     parser.add_argument('--rr-chaos', default=False, action="store_true",
                         help="Run under chaos mode in rr until a failure is captured")
     parser.add_argument('--pref', default=[], action="append", dest="prefs",

python/wpt/run.py

@@ -33,24 +33,13 @@ TRACKER_API_ENV_VAR = "INTERMITTENT_TRACKER_API"
 TRACKER_DASHBOARD_SECRET_ENV_VAR = "INTERMITTENT_TRACKER_DASHBOARD_SECRET"
 
 
-def determine_build_type(kwargs: dict, target_dir: str):
-    if kwargs["release"]:
-        return "release"
-    elif kwargs["debug"]:
-        return "debug"
-    elif os.path.exists(os.path.join(target_dir, "debug")):
-        return "debug"
-    elif os.path.exists(os.path.join(target_dir, "release")):
-        return "release"
-    return "debug"
-
-
 def set_if_none(args: dict, key: str, value):
     if key not in args or args[key] is None:
         args[key] = value
 
 
-def run_tests(**kwargs):
+def run_tests(default_binary_path: str, **kwargs):
+    print(default_binary_path)
     # By default, Rayon selects the number of worker threads based on the
     # available CPU count. This doesn't work very well when running tests on CI,
     # since we run so many Servo processes in parallel. The result is a lot of
@@ -79,17 +68,6 @@ def run_tests(**kwargs):
 
     kwargs["user_stylesheets"].append(os.path.join(SERVO_ROOT, "resources", "ahem.css"))
 
-    if "CARGO_TARGET_DIR" in os.environ:
-        target_dir = os.path.join(os.environ["CARGO_TARGET_DIR"])
-    else:
-        target_dir = os.path.join(SERVO_ROOT, "target")
-
-    binary_name = ("servo.exe" if sys.platform == "win32" else "servo")
-
-    default_binary_path = os.path.join(
-        target_dir, determine_build_type(kwargs, target_dir), binary_name
-    )
-
     set_if_none(kwargs, "binary", default_binary_path)
     set_if_none(kwargs, "webdriver_binary", default_binary_path)