Use changes-client's collect-targets logic

Summary: In the past, we had a collect-targets script that we pass to Changes client. But now that Changes client has its own collect-targets script, we use that instead.

Test Plan: unit tests

Reviewers: anupc

Reviewed By: anupc

Subscribers: changesbot, anupc, kylec

Differential Revision: https://tails.corp.dropbox.com/D226439

changes/buildsteps/default.py

@@ -196,10 +196,11 @@ def get_label(self):
     def get_test_stats_from(self):
         return self.test_stats_from
 
-    def _custom_bin_path(self):
+    @classmethod
+    def custom_bin_path(cls):
         """The path in which to look for custom binaries we want to run.
-        This is used in LXC where we bind mount custom binaries
-        (currently only blacklist-remove)"""
+        This is used in LXC where we bind mount custom binaries, such as
+        blacklist-remove and collect-targets."""
         return ''
 
     def _other_repo_clone_commands(self, other_repos):
@@ -270,7 +271,7 @@ def iter_all_commands(self, job):
             for command in self.other_repo_clone_commands:
                 yield command
 
-        blacklist_remove_path = os.path.join(self._custom_bin_path(), 'blacklist-remove')
+        blacklist_remove_path = os.path.join(self.custom_bin_path(), 'blacklist-remove')
         yield FutureCommand(
             script=blacklist_remove_path + ' "' + job.project.get_config_path() + '"',
             path=self.repo_path,

changes/buildsteps/lxc.py

@@ -17,7 +17,8 @@ def get_label(self):
     def get_client_adapter(self):
         return 'lxc'
 
-    def _custom_bin_path(self):
+    @classmethod
+    def custom_bin_path(cls):
         # This is where we mount custom binaries in the container
         return '/var/changes/input/'
 

changes/models/jobplan.py

@@ -1,6 +1,7 @@
 from __future__ import absolute_import
 
 import logging
+import os
 import uuid
 
 from collections import defaultdict
@@ -227,6 +228,7 @@ def get_build_step_for_job(cls, job_id):
                     {'script': get_bazel_setup(), 'type': 'setup'},
                     {'script': sync_encap_pkgs(project_config), 'type': 'setup'},  # TODO(anupc): Make this optional
                     {'script': collect_bazel_targets(
+                        collect_targets_executable=os.path.join(LXCBuildStep.custom_bin_path(), 'collect-targets'),
                         bazel_targets=project_config['bazel.targets'],
                         bazel_exclude_tags=bazel_exclude_tags,
                         max_jobs=2 * bazel_cpus), 'type': 'collect_bazel_targets'},

changes/utils/bazel_setup.py

@@ -1,5 +1,3 @@
-import os
-
 from flask import current_app
 
 
@@ -30,7 +28,7 @@
 (sudo apt-get -y update || sudo apt-get -y update) >/dev/null 2>&1
 sudo apt-get install -y --force-yes {bazel_apt_pkgs} python >/dev/null 2>&1
 
-python -c "{script}" "{bazel_root}" "{bazel_targets}" "{bazel_exclude_tags}" "{max_jobs}" 2> /dev/null
+"{collect_targets_executable}" --output-user-root="{bazel_root}" {bazel_targets} {bazel_exclude_tags} --jobs="{max_jobs}" 2> /dev/null
 """.strip()
 
 
@@ -47,7 +45,7 @@ def get_bazel_setup():
     )
 
 
-def collect_bazel_targets(bazel_targets, bazel_exclude_tags, max_jobs):
+def collect_bazel_targets(collect_targets_executable, bazel_targets, bazel_exclude_tags, max_jobs):
     """Construct a command to query the Bazel dependency graph to expand bazel project
     config into a set of individual test targets.
 
@@ -59,20 +57,16 @@ def collect_bazel_targets(bazel_targets, bazel_exclude_tags, max_jobs):
     - exclude-tags: List of target tags. Targets matching any of these tags are not returned.
       By default, this list is empty.
     """
-    # type: (List[str], List[str], int) -> str
-    package_dir = os.path.dirname(__file__)
-    bazel_target_py = os.path.join(package_dir, "collect_bazel_targets.py")
-
-    with open(bazel_target_py, 'r') as script:
-        return COLLECT_BAZEL_TARGETS.format(
-            apt_spec=current_app.config['APT_SPEC'],
-            bazel_apt_pkgs=' '.join(current_app.config['BAZEL_APT_PKGS']),
-            bazel_root=current_app.config['BAZEL_ROOT_PATH'],
-            bazel_targets=','.join(bazel_targets),
-            script=script.read().replace(r'"', r'\"').replace(r'$', r'\$').replace(r'`', r'\`'),
-            bazel_exclude_tags=','.join(bazel_exclude_tags),
-            max_jobs=max_jobs,
-        )
+    # type: (str, List[str], List[str], int) -> str
+    return COLLECT_BAZEL_TARGETS.format(
+        apt_spec=current_app.config['APT_SPEC'],
+        bazel_apt_pkgs=' '.join(current_app.config['BAZEL_APT_PKGS']),
+        bazel_root=current_app.config['BAZEL_ROOT_PATH'],
+        bazel_targets=' '.join(['--target-patterns={}'.format(t) for t in bazel_targets]),
+        collect_targets_executable=collect_targets_executable,
+        bazel_exclude_tags=' '.join(['--exclude-tags={}'.format(t) for t in bazel_exclude_tags]),
+        max_jobs=max_jobs,
+    )
 
 
 def sync_encap_pkgs(project_config, encap_dir='/usr/local/encap/'):

tests/changes/buildsteps/test_lxc.py

@@ -1,12 +1,19 @@
 from __future__ import absolute_import
 
-from changes.buildsteps.default import DEFAULT_PATH
+from changes.buildsteps.default import (
+    DEFAULT_ARTIFACTS, DEFAULT_ENV, DEFAULT_PATH,
+)
 from changes.buildsteps.lxc import LXCBuildStep
+from changes.constants import Status
+from changes.models.command import CommandType
 from changes.testutils import TestCase
 
 
 class LXCBuildStepTest(TestCase):
-    def get_buildstep(self):
+    def get_buildstep(self, **kwargs):
+        kwargs['release'] = 'trusty'
+        kwargs['cpus'] = 8
+        kwargs['memory'] = 9000
         return LXCBuildStep(
             commands=[
                 dict(
@@ -19,10 +26,51 @@ def get_buildstep(self):
                 dict(
                     script='echo "hello world 1"',
                 ),
-            ],
-            release='trusty',
-            cpus=8,
-            memory=9000)
+            ], **kwargs)
+
+    def test_execute(self):
+        build = self.create_build(self.create_project(name='foo'), label='buildlabel')
+        job = self.create_job(build)
+
+        buildstep = self.get_buildstep(cluster='foo', repo_path='source', path='tests')
+        buildstep.execute(job)
+
+        assert job.phases[0].label == 'buildlabel'
+        step = job.phases[0].steps[0]
+
+        assert step.data['release'] == 'trusty'
+        assert step.status == Status.pending_allocation
+        assert step.cluster == 'foo'
+        assert step.label == 'buildlabel'
+
+        commands = step.commands
+        assert len(commands) == 3
+
+        idx = 0
+        # blacklist remove command
+        assert commands[idx].script == '/var/changes/input/blacklist-remove "foo.yaml"'
+        assert commands[idx].cwd == 'source'
+        assert commands[idx].type == CommandType.infra_setup
+        assert commands[idx].artifacts == []
+        assert commands[idx].env == DEFAULT_ENV
+        assert commands[idx].order == idx
+
+        idx += 1
+        assert commands[idx].script == 'echo "hello world 2"'
+        assert commands[idx].cwd == '/usr/test/1'
+        assert commands[idx].type == CommandType.setup
+        assert tuple(commands[idx].artifacts) == ('artifact1.txt', 'artifact2.txt')
+        assert commands[idx].env['PATH'] == '/usr/test/1'
+        for k, v in DEFAULT_ENV.items():
+            if k != 'PATH':
+                assert commands[idx].env[k] == v
+
+        idx += 1
+        assert commands[idx].script == 'echo "hello world 1"'
+        assert commands[idx].cwd == 'source/tests'
+        assert commands[idx].type == CommandType.default
+        assert tuple(commands[idx].artifacts) == tuple(DEFAULT_ARTIFACTS)
+        assert commands[idx].env == DEFAULT_ENV
 
     def test_get_allocation_params(self):
         project = self.create_project()

tests/changes/models/test_job.py

@@ -7,7 +7,6 @@
 from changes.models.command import CommandType
 from changes.models.jobplan import JobPlan
 from changes.testutils import TestCase
-from changes.utils.bazel_setup import COLLECT_BAZEL_TARGETS
 
 
 class AutogeneratedJobTest(TestCase):
@@ -48,9 +47,7 @@ def test_autogenerated_commands(self, get_config):
             'bazel.max-executors': 1,  # Default
         }
 
-        with mock.patch('changes.utils.bazel_setup.COLLECT_BAZEL_TARGETS') as mock_collect_bazel_target_command:
-            mock_collect_bazel_target_command.format.return_value = 'test script'
-            _, implementation = JobPlan.get_build_step_for_job(self._create_job_and_jobplan().id)
+        _, implementation = JobPlan.get_build_step_for_job(self._create_job_and_jobplan().id)
 
         bazel_setup_expected = """#!/bin/bash -eux
 # Clean up any existing apt sources
@@ -69,6 +66,19 @@ def test_autogenerated_commands(self, get_config):
 sudo mkdir -p /usr/local/encap/
 sudo /usr/bin/rsync -a --delete rsync://example.com/encap/package1 /usr/local/encap/
 sudo /usr/bin/rsync -a --delete rsync://example.com/encap/pkg-2 /usr/local/encap/
+""".strip()
+
+        collect_targets_expected = """#!/bin/bash -eu
+# Clean up any existing apt sources
+sudo rm -rf /etc/apt/sources.list.d >/dev/null 2>&1
+# Overwrite apt sources
+(echo "deb http://example.com/debian distribution component1" | sudo tee /etc/apt/sources.list) >/dev/null 2>&1
+
+# apt-get update, and try again if it fails first time
+(sudo apt-get -y update || sudo apt-get -y update) >/dev/null 2>&1
+sudo apt-get install -y --force-yes bazel python >/dev/null 2>&1
+
+"/var/changes/input/collect-targets" --output-user-root="/bazel/root/path" --target-patterns=//aa/bb/cc/... --target-patterns=//aa/abc/...  --jobs="8" 2> /dev/null
 """.strip()
 
         assert len(implementation.commands) == 3
@@ -85,22 +95,7 @@ def test_autogenerated_commands(self, get_config):
         assert implementation.commands[1].script == sync_encap_expected
 
         assert implementation.commands[2].type == CommandType.collect_bazel_targets
-        assert implementation.commands[2].script == 'test script'
-
-        kwargs = dict(
-            apt_spec='deb http://example.com/debian distribution component1',
-            bazel_apt_pkgs='bazel',
-            bazel_root='/bazel/root/path',
-            bazel_targets='//aa/bb/cc/...,//aa/abc/...',
-            script=mock.ANY,  # this one is really not worth checking
-            bazel_exclude_tags='',
-            max_jobs=8,
-        )
-        mock_collect_bazel_target_command.format.assert_called_once_with(**kwargs)
-
-        # make sure string formatting apply cleanly
-        _, kwargs = mock_collect_bazel_target_command.format.call_args
-        COLLECT_BAZEL_TARGETS.format(**kwargs)
+        assert implementation.commands[2].script == collect_targets_expected
 
     @mock.patch('changes.models.project.Project.get_config')
     def test_autogenerated_commands_with_exclusions(self, get_config):
@@ -118,9 +113,20 @@ def test_autogenerated_commands_with_exclusions(self, get_config):
             'bazel.max-executors': 3,
         }
 
-        with mock.patch('changes.utils.bazel_setup.COLLECT_BAZEL_TARGETS') as mock_collect_bazel_target_command:
-            mock_collect_bazel_target_command.format.return_value = 'test script'
-            _, implementation = JobPlan.get_build_step_for_job(self._create_job_and_jobplan().id)
+        collect_tests_expected = """#!/bin/bash -eu
+# Clean up any existing apt sources
+sudo rm -rf /etc/apt/sources.list.d >/dev/null 2>&1
+# Overwrite apt sources
+(echo "deb http://example.com/debian distribution component1" | sudo tee /etc/apt/sources.list) >/dev/null 2>&1
+
+# apt-get update, and try again if it fails first time
+(sudo apt-get -y update || sudo apt-get -y update) >/dev/null 2>&1
+sudo apt-get install -y --force-yes bazel python >/dev/null 2>&1
+
+"/var/changes/input/collect-targets" --output-user-root="/bazel/root/path" --target-patterns=//foo/bar/baz/... --target-patterns=//bar/bax/... --exclude-tags=flaky --exclude-tags=another_tag --jobs="4" 2> /dev/null
+""".strip()
+
+        _, implementation = JobPlan.get_build_step_for_job(self._create_job_and_jobplan().id)
 
         assert implementation.max_executors == 3
 
@@ -135,22 +141,7 @@ def test_autogenerated_commands_with_exclusions(self, get_config):
         assert implementation.commands[0].type == CommandType.setup
         assert implementation.commands[1].type == CommandType.setup
         assert implementation.commands[2].type == CommandType.collect_bazel_targets
-        assert implementation.commands[2].script == 'test script'
-
-        kwargs = dict(
-            apt_spec='deb http://example.com/debian distribution component1',
-            bazel_apt_pkgs='bazel',
-            bazel_root='/bazel/root/path',
-            bazel_targets='//foo/bar/baz/...,//bar/bax/...',
-            script=mock.ANY,  # this one is really not worth checking
-            bazel_exclude_tags='flaky,another_tag',
-            max_jobs=4,
-        )
-        mock_collect_bazel_target_command.format.assert_called_once_with(**kwargs)
-
-        # make sure string formatting apply cleanly
-        _, kwargs = mock_collect_bazel_target_command.format.call_args
-        COLLECT_BAZEL_TARGETS.format(**kwargs)
+        assert implementation.commands[2].script == collect_tests_expected
 
     @mock.patch('changes.models.project.Project.get_config')
     def test_invalid_cpus(self, get_config):