[zorg] Support other relative scripts in AnnotatedBuilder

[ashi1]

Summary

AnnotatedBuilder should be able to launch both python and bash
scripts. It should launch with python for scripts that specify
the script_interpreter as python, or allow the user to specify
another interpreter. Otherwise it will execute empty
script_interpreter scripts directly. Also, move --jobs option
to default of extra_args, otherwise allow the user to override
extra_args.

[tra]

zorg/buildbot/builders/AnnotatedBuilder.py

95

Perhaps we should instead add a script_interpreter="python" argument to the function and change the invocation to just

command = [script_interpreter, script_path, WithProperties("--jobs=%(jobs:-)s")]

This would avoid second-guessing the user's intent. I.e. what if my script a .py, but requires particular python version?
Actually, It would also make sense to handle script arguments the same way, so users can override them if necessary.

[ashi1]

zorg/buildbot/builders/AnnotatedBuilder.py

95

I've added the script_interpreter and I agree with you that it would better to handle script arguments in this way. Also, I've make it so that if the script_interpreter is empty, it should execute the script directly.

[tra]

zorg/buildbot/builders/AnnotatedBuilder.py

96

I'd also make script arguments configurable, too, defaulting to [WithProperties("--jobs=%(jobs:-)s")] so existing users don't need to change anything.
Generally speaking we can't assume that all the python scripts would need or want --jobs argument.

[ashi1]

zorg/buildbot/builders/AnnotatedBuilder.py

96

This script seems to have arguments passed in by extra_args and then populated into extra_args_with_props. Would it work to remove [WithProperties("--jobs=%(jobs:-)s")] and add it to extra_args_with_props, when extra_args==None (around line 82). This jobs property seems to be added to all builds regardless of extra args at the moment.

[tra]

zorg/buildbot/builders/AnnotatedBuilder.py

96

Oh. I've missed the extra_args. Moving the jobs there by default would make sense.

[ashi1]

zorg/buildbot/builders/AnnotatedBuilder.py

96

I've made a change to make --jobs argument to be a toggle. By default it is true.

[tra]

zorg/buildbot/builders/AnnotatedBuilder.py

96

But why does --jobs deserve a special flag. AFAICT, user should be able to pass it via extra_args already. It just happened to be the default argument we've been passing by default so far.

I think all we want now is:

if extra_args is None:
    # We used to add --jobs to all script invocations. Preserve this for cases when user 
    # did not specify extra_args, but allow overriding it with user did specify extra_args.
    extra_args = [WithProperties("--jobs=%(jobs:-)s")]

AFAICT, util.WithProperties should be available to whoever would call getAnnotatedBuildFactory, so they can add the argument explicitly if they need it in addition to other arguments they may need to pass.
Currently only libc-linux-py appears to use extra_args and the script itself does not seem to use --jobs argument for anything, so I'm not sure that we need yet another option for adding --jobs.

[ashi1]

zorg/buildbot/builders/AnnotatedBuilder.py

96

I see, I saw libc-linux-py was using extra_args, but didn't check whether their script actually used --jobs or not. Since they don't use it, I am okay with this approach. Will update this, thanks.

[ashi1]

Revised to Artem's comments, moved --jobs to extra_args when are unspecified.

[tra]

LGTM. I'll leave the patch approval to @gkistanova .

[gkistanova]

Thanks! Looks good now.

zorg/buildbot/builders/AnnotatedBuilder.py

 from buildbot.process.properties import WithProperties
 from buildbot.steps.shell import SetProperty
 from zorg.buildbot.commands.AnnotatedCommand import AnnotatedCommand
 from zorg.buildbot.process.factory import LLVMBuildFactory
 
 
 def getAnnotatedBuildFactory(
     script,
     clean=False,
     depends_on_projects=None,
     env=None,
     extra_args=None,
     timeout=1200,
-    checkout_llvm_sources=True):
+    checkout_llvm_sources=True,
+    script_interpreter="python",
+    add_jobs_arg=True):
     """
     Returns a new build factory that uses AnnotatedCommand, which
     allows the build to be run by version-controlled scripts that do
     not require a buildmaster restart to update.
 
     script: script under "builders/annotated" to be run by python
     clean: set to true for a clean build of llvm
     depends_on_projects: which subprojects to enable
         llvm must be first in the list
         (default: ["llvm", "clang", "compiler-rt", "libcxx",
                    "libcxxabi", "libunwind", "lld"])
     env: environment overrides (map; default is no overrides)
     extra_args: extra arguments to pass to the script (default: [])
     timeout: specifies the builder's timeout in seconds (default: 1200)
+    script_interpreter: specifies the interpreter to run scripts (default: "python")
+    add_jobs_arg: specifies whether to add --jobs flag (default: True)
     """
 
     if depends_on_projects is None:
         depends_on_projects = [
             "llvm",
             "clang",
             "compiler-rt",
             "libcxx",
             "libcxxabi",
             "libunwind",
             "lld"]
     if extra_args is None:
         extra_args = []
 
+    if add_jobs_arg:
+        extra_args.insert(0, "--jobs=%(jobs:-)s")
+
     f = LLVMBuildFactory(
         clean=clean,
         depends_on_projects=depends_on_projects)
 
     if clean:
         f.addStep(SetProperty(property='clean', command='echo 1'))
 
     # We normally use the clean property to indicate that we want a
if checkout_llvm_sources:
       f.addGetSourcecodeSteps()
 
     extra_args_with_props = [WithProperties(arg) for arg in extra_args]
     # Explicitly use '/' as separator, because it works on *nix and Windows.
     if script.startswith('/'):
       command = [script]
     else:
       script_path = "../llvm-zorg/zorg/buildbot/builders/annotated/%s" % (script)
-      command = ["python", script_path, WithProperties("--jobs=%(jobs:-)s")]
+      # Handle scripts with script_interpreter, otherwise execute the script directly.
+      if script_interpreter:
+        command = [script_interpreter, script_path]
+      else:
+        command = [script_path]
+
     command += extra_args_with_props
 
     f.addStep(AnnotatedCommand(name="annotate",
                                description="annotate",
                                timeout=timeout,
                                haltOnFailure=True,
                                command=command,
                                env=merged_env))
     return f