cli.py: Make source checkout more consistent with artifact checkout

This MR changes the behaviour of source checkout so that it is more
like artifact checkout. That is, a --directory option can be provided,
rather than a mandatory LOCATION.

In addition to this, the --tar option is no longer a boolean flag,
rather it expects a tarfile name (just like artifact checkout)

The appropriate tests have also been altered so that they support
the new API

3 files changed, 42 insertions, 28 deletions

diff --git a/src/buildstream/_frontend/cli.py b/src/buildstream/_frontend/cli.py
index 631eff7d3..9fa482666 100644
--- a/src/buildstream/_frontend/cli.py
+++ b/src/buildstream/_frontend/cli.py
@@ -807,28 +807,31 @@ def source_track(app, elements, deps, except_, cross_junctions):
 @click.option('--deps', '-d', default='none', show_default=True,
               type=click.Choice(['build', 'none', 'run', 'all']),
               help='The dependencies whose sources to checkout')
-@click.option('--tar', 'tar', is_flag=True,
-              help='Create a tarball from the element\'s sources instead of a '
-                   'file tree.')
+@click.option('--tar', default=None, metavar='LOCATION',
+              type=click.Path(),
+              help="Create a tarball containing the sources instead "
+                   "of a file tree.")
 @click.option('--include-build-scripts', 'build_scripts', is_flag=True)
+@click.option('--directory', default='source-checkout',
+              type=click.Path(file_okay=False),
+              help="The directory to checkout the sources to")
 @click.argument('element', required=False, type=click.Path(readable=False))
-@click.argument('location', type=click.Path(), required=False)
 @click.pass_obj
-def source_checkout(app, element, location, force, deps, except_,
+def source_checkout(app, element, directory, force, deps, except_,
                     tar, build_scripts):
     """Checkout sources of an element to the specified location
 
     When this command is executed from a workspace directory, the default
     is to checkout the sources of the workspace element.
     """
-    if not element and not location:
-        click.echo("ERROR: LOCATION is not specified", err=True)
+
+    if tar and directory != "source-checkout":
+        click.echo("ERROR: options --directory and --tar conflict", err=True)
         sys.exit(-1)
 
-    if element and not location:
-        # Nasty hack to get around click's optional args
-        location = element
-        element = None
+    # Set the location depending on whether --tar/--directory were specified
+    # Note that if unset, --directory defaults to "source-checkout"
+    location = tar if tar else directory
 
     with app.initialized():
         if not element:
@@ -841,7 +844,7 @@ def source_checkout(app, element, location, force, deps, except_,
                                    force=force,
                                    deps=deps,
                                    except_targets=except_,
-                                   tar=tar,
+                                   tar=bool(tar),
                                    include_build_scripts=build_scripts)
 
 
diff --git a/tests/frontend/source_checkout.py b/tests/frontend/source_checkout.py
index 87ea9e3e2..e13adc910 100644
--- a/tests/frontend/source_checkout.py
+++ b/tests/frontend/source_checkout.py
@@ -52,7 +52,7 @@ def test_source_checkout(datafiles, cli, tmpdir_factory, with_workspace, guess_e
     else:
         ws_cmd = []
 
-    args = ws_cmd + ['source', 'checkout', '--deps', 'none', *elm_cmd, checkout]
+    args = ws_cmd + ['source', 'checkout', '--deps', 'none', '--directory', checkout, *elm_cmd]
     result = cli.run(project=project, args=args)
     result.assert_success()
 
@@ -69,7 +69,11 @@ def test_source_checkout_force(datafiles, cli, force_flag):
     # Make the checkout directory with 'some-thing' inside it
     os.makedirs(os.path.join(checkout, 'some-thing'))
 
-    result = cli.run(project=project, args=['source', 'checkout', force_flag, target, '--deps', 'none', checkout])
+    result = cli.run(project=project, args=['source', 'checkout',
+                                            force_flag,
+                                            '--deps', 'none',
+                                            '--directory', checkout,
+                                            target])
     result.assert_success()
 
     assert os.path.exists(os.path.join(checkout, 'checkout-deps', 'etc', 'buildstream', 'config'))
@@ -78,15 +82,18 @@ def test_source_checkout_force(datafiles, cli, force_flag):
 @pytest.mark.datafiles(DATA_DIR)
 def test_source_checkout_tar(datafiles, cli):
     project = str(datafiles)
-    checkout = os.path.join(cli.directory, 'source-checkout.tar')
+    tar = os.path.join(cli.directory, 'source-checkout.tar')
     target = 'checkout-deps.bst'
 
-    result = cli.run(project=project, args=['source', 'checkout', '--tar', target, '--deps', 'none', checkout])
+    result = cli.run(project=project, args=['source', 'checkout',
+                                            '--tar', tar,
+                                            '--deps', 'none',
+                                            target])
     result.assert_success()
 
-    assert os.path.exists(checkout)
-    with tarfile.open(checkout) as tf:
-        expected_content = os.path.join(checkout, 'checkout-deps', 'etc', 'buildstream', 'config')
+    assert os.path.exists(tar)
+    with tarfile.open(tar) as tf:
+        expected_content = os.path.join(tar, 'checkout-deps', 'etc', 'buildstream', 'config')
         tar_members = [f.name for f in tf]
         for member in tar_members:
             assert member in expected_content
@@ -99,7 +106,10 @@ def test_source_checkout_deps(datafiles, cli, deps):
     checkout = os.path.join(cli.directory, 'source-checkout')
     target = 'checkout-deps.bst'
 
-    result = cli.run(project=project, args=['source', 'checkout', target, '--deps', deps, checkout])
+    result = cli.run(project=project, args=['source', 'checkout',
+                                            '--directory', checkout,
+                                            '--deps', deps,
+                                            target])
     result.assert_success()
 
     # Sources of the target
@@ -127,10 +137,11 @@ def test_source_checkout_except(datafiles, cli):
     checkout = os.path.join(cli.directory, 'source-checkout')
     target = 'checkout-deps.bst'
 
-    result = cli.run(project=project, args=['source', 'checkout', target,
+    result = cli.run(project=project, args=['source', 'checkout',
+                                            '--directory', checkout,
                                             '--deps', 'all',
                                             '--except', 'import-bin.bst',
-                                            checkout])
+                                            target])
     result.assert_success()
 
     # Sources for the target should be present
@@ -162,7 +173,7 @@ def test_source_checkout_fetch(datafiles, cli):
 
     args = ['source', 'checkout']
     args += [target, checkout]
-    result = cli.run(project=project, args=args)
+    result = cli.run(project=project, args=['source', 'checkout', '--directory', checkout, target])
 
     result.assert_success()
     assert os.path.exists(os.path.join(checkout, 'remote-import-dev', 'pony.h'))
@@ -175,7 +186,7 @@ def test_source_checkout_build_scripts(cli, tmpdir, datafiles):
     normal_name = 'source-bundle-source-bundle-hello'
     checkout = os.path.join(str(tmpdir), 'source-checkout')
 
-    args = ['source', 'checkout', '--include-build-scripts', element_name, checkout]
+    args = ['source', 'checkout', '--include-build-scripts', '--directory', checkout, element_name]
     result = cli.run(project=project_path, args=args)
     result.assert_success()
 
@@ -192,7 +203,7 @@ def test_source_checkout_tar_buildscripts(cli, tmpdir, datafiles):
     normal_name = 'source-bundle-source-bundle-hello'
     tar_file = os.path.join(str(tmpdir), 'source-checkout.tar')
 
-    args = ['source', 'checkout', '--include-build-scripts', '--tar', element_name, tar_file]
+    args = ['source', 'checkout', '--include-build-scripts', '--tar', tar_file, element_name]
     result = cli.run(project=project_path, args=args)
     result.assert_success()
 
diff --git a/tests/sourcecache/source-checkout.py b/tests/sourcecache/source-checkout.py
index c2c7fe3cd..4e3391c12 100644
--- a/tests/sourcecache/source-checkout.py
+++ b/tests/sourcecache/source-checkout.py
@@ -49,7 +49,7 @@ def test_source_checkout(tmpdir, datafiles, cli):
     repo = create_element_size('target.bst', project_dir, element_path, [], 100000)
 
     # check implicit fetching
-    res = cli.run(project=project_dir, args=['source', 'checkout', 'target.bst', target_dir])
+    res = cli.run(project=project_dir, args=['source', 'checkout', '--directory', target_dir, 'target.bst'])
     res.assert_success()
     assert "Fetching from" in res.stderr
 
@@ -60,7 +60,7 @@ def test_source_checkout(tmpdir, datafiles, cli):
     shutil.rmtree(source_dir)
 
     res = cli.run(project=project_dir,
-                  args=['source', 'checkout', 'target.bst', target_dir])
+                  args=['source', 'checkout', '--directory', target_dir, 'target.bst'])
     res.assert_success()
     assert "Fetching from" not in res.stderr
 
@@ -69,5 +69,5 @@ def test_source_checkout(tmpdir, datafiles, cli):
     shutil.rmtree(os.path.join(cache_dir, 'cas'))
 
     res = cli.run(project=project_dir,
-                  args=['source', 'checkout', 'target.bst', target_dir])
+                  args=['source', 'checkout', '--directory', target_dir, 'target.bst'])
     res.assert_task_error(ErrorDomain.PLUGIN, None)