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.
Details
Diff Detail
- Repository
- rZORG LLVM Github Zorg
Event Timeline
zorg/buildbot/builders/AnnotatedBuilder.py | ||
---|---|---|
93 | 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? |
zorg/buildbot/builders/AnnotatedBuilder.py | ||
---|---|---|
93 | 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. |
zorg/buildbot/builders/AnnotatedBuilder.py | ||
---|---|---|
94 | I'd also make script arguments configurable, too, defaulting to [WithProperties("--jobs=%(jobs:-)s")] so existing users don't need to change anything. |
zorg/buildbot/builders/AnnotatedBuilder.py | ||
---|---|---|
94 | 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. |
zorg/buildbot/builders/AnnotatedBuilder.py | ||
---|---|---|
94 | Oh. I've missed the extra_args. Moving the jobs there by default would make sense. |
zorg/buildbot/builders/AnnotatedBuilder.py | ||
---|---|---|
94 | I've made a change to make --jobs argument to be a toggle. By default it is true. |
zorg/buildbot/builders/AnnotatedBuilder.py | ||
---|---|---|
94 | 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. |
zorg/buildbot/builders/AnnotatedBuilder.py | ||
---|---|---|
94 | 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. |
Perhaps we should instead add a script_interpreter="python" argument to the function and change the invocation to just
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.