set env variables directly in the command dictionary in the bazel set…

…up script

Summary: This avoids shell-related errors, such as forgetting to put `export` or escape properly.

Test Plan: unit tests

Reviewers: anupc

Reviewed By: anupc

Subscribers: changesbot, kylec, wwu

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

changes/models/jobplan.py

@@ -225,19 +225,27 @@ def get_build_step_for_job(cls, job_id):
             bazel_test_flags = current_app.config['BAZEL_MANDATORY_TEST_FLAGS']
             # TODO(anupc): Allow additional flags to be passed in via project config.
 
+            vcs = job.project.repository.get_vcs()
             implementation = LXCBuildStep(
                 cluster=current_app.config['DEFAULT_CLUSTER'],
                 commands=[
                     {'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,
-                        bazel_test_flags=bazel_test_flags,
-                        vcs=job.project.repository.get_vcs(),
-                        ), 'type': 'collect_bazel_targets'},
+                    {
+                        '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,
+                            bazel_test_flags=bazel_test_flags,
+                            ),
+                        'type': 'collect_bazel_targets',
+                        'env': {
+                            'VCS_CHECKOUT_TARGET_REVISION_CMD': vcs.get_buildstep_checkout_revision('master'),
+                            'VCS_CHECKOUT_PARENT_REVISION_CMD': vcs.get_buildstep_checkout_parent_revision('master'),
+                            'VCS_GET_CHANGED_FILES_CMD': vcs.get_buildstep_changed_files('master'),
+                            },
+                        },
                 ],
                 artifacts=[],  # only for collect_target step, which we don't expect artifacts
                 artifact_suffix=current_app.config['BAZEL_ARTIFACT_SUFFIX'],

changes/utils/bazel_setup.py

@@ -28,10 +28,6 @@
 (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
 
-VCS_CHECKOUT_TARGET_REVISION="{checkout_target_revision}"
-VCS_CHECKOUT_PARENT_REVISION="{checkout_parent_revision}"
-VCS_GET_CHANGED_FILES="{get_changed_files}"
-
 "{collect_targets_executable}" --output-user-root="{bazel_root}" {bazel_targets} {bazel_exclude_tags} {bazel_test_flags} --jobs="{max_jobs}" 2> /dev/null
 """.strip()
 
@@ -49,7 +45,7 @@ def get_bazel_setup():
     )
 
 
-def collect_bazel_targets(collect_targets_executable, bazel_targets, bazel_exclude_tags, bazel_test_flags, max_jobs, vcs):
+def collect_bazel_targets(collect_targets_executable, bazel_targets, bazel_exclude_tags, bazel_test_flags, max_jobs):
     """Construct a command to query the Bazel dependency graph to expand bazel project
     config into a set of individual test targets.
 
@@ -71,9 +67,6 @@ def collect_bazel_targets(collect_targets_executable, bazel_targets, bazel_exclu
         bazel_exclude_tags=' '.join(['--exclude-tags={}'.format(t) for t in bazel_exclude_tags]),
         bazel_test_flags=' '.join(['--test-flags={}'.format(tf) for tf in bazel_test_flags]),
         max_jobs=max_jobs,
-        checkout_target_revision=vcs.get_buildstep_checkout_revision('master'),
-        checkout_parent_revision=vcs.get_buildstep_checkout_parent_revision('master'),
-        get_changed_files=vcs.get_buildstep_changed_files('master'),
     )
 
 

tests/changes/models/test_job.py

@@ -86,10 +86,6 @@ def test_autogenerated_commands(self, get_config):
 (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
 
-VCS_CHECKOUT_TARGET_REVISION="git checkout master"
-VCS_CHECKOUT_PARENT_REVISION="git checkout master^"
-VCS_GET_CHANGED_FILES="git diff --name-only master^..master"
-
 "/var/changes/input/collect-targets" --output-user-root="/bazel/root/path" --target-patterns=//aa/bb/cc/... --target-patterns=//aa/abc/...  --test-flags=--spawn_strategy=sandboxed --test-flags=--genrule_strategy=sandboxed --test-flags=--keep_going --jobs="8" 2> /dev/null
 """.strip()
 
@@ -108,6 +104,9 @@ def test_autogenerated_commands(self, get_config):
 
         assert implementation.commands[2].type == CommandType.collect_bazel_targets
         assert implementation.commands[2].script == collect_targets_expected
+        assert implementation.commands[2].env['VCS_CHECKOUT_TARGET_REVISION_CMD'] == 'git checkout master'
+        assert implementation.commands[2].env['VCS_CHECKOUT_PARENT_REVISION_CMD'] == 'git checkout master^'
+        assert implementation.commands[2].env['VCS_GET_CHANGED_FILES_CMD'] == 'git diff --name-only master^..master'
 
     @mock.patch('changes.models.project.Project.get_config')
     def test_autogenerated_commands_with_exclusions(self, get_config):
@@ -135,10 +134,6 @@ def test_autogenerated_commands_with_exclusions(self, get_config):
 (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
 
-VCS_CHECKOUT_TARGET_REVISION="git checkout master"
-VCS_CHECKOUT_PARENT_REVISION="git checkout master^"
-VCS_GET_CHANGED_FILES="git diff --name-only master^..master"
-
 "/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 --test-flags=--spawn_strategy=sandboxed --test-flags=--genrule_strategy=sandboxed --test-flags=--keep_going --jobs="4" 2> /dev/null
 """.strip()