Page MenuHomePhabricator

[zorg] Support other relative scripts in AnnotatedBuilder
ClosedPublic

Authored by ashi1 on Apr 16 2021, 11:16 AM.

Details

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.

Diff Detail

Event Timeline

ashi1 requested review of this revision.Apr 16 2021, 11:16 AM
ashi1 created this revision.
tra added inline comments.Apr 16 2021, 11:31 AM
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?
Actually, It would also make sense to handle script arguments the same way, so users can override them if necessary.

ashi1 updated this revision to Diff 338856.Apr 20 2021, 7:05 AM
ashi1 retitled this revision from [zorg] Support relative non .py scripts in AnnotatedBuilder to [zorg] Support other relative scripts in AnnotatedBuilder.
ashi1 edited the summary of this revision. (Show Details)
ashi1 marked an inline comment as done.
ashi1 added inline comments.
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.

tra added inline comments.Apr 20 2021, 9:41 AM
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.
Generally speaking we can't assume that all the python scripts would need or want --jobs argument.

ashi1 marked an inline comment as done.Apr 20 2021, 10:29 AM
ashi1 added inline comments.
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.

tra added inline comments.Apr 20 2021, 10:44 AM
zorg/buildbot/builders/AnnotatedBuilder.py
94

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

ashi1 marked 2 inline comments as done.Apr 20 2021, 1:20 PM
ashi1 added inline comments.
zorg/buildbot/builders/AnnotatedBuilder.py
94

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

ashi1 updated this revision to Diff 338976.Apr 20 2021, 1:20 PM
ashi1 marked an inline comment as done.
ashi1 edited the summary of this revision. (Show Details)
tra added inline comments.Apr 20 2021, 1:58 PM
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.
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 added inline comments.Apr 21 2021, 9:57 AM
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.

ashi1 updated this revision to Diff 339295.Apr 21 2021, 10:06 AM
ashi1 marked an inline comment as done.

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

ashi1 edited the summary of this revision. (Show Details)Apr 21 2021, 10:07 AM
tra added a comment.Apr 21 2021, 10:26 AM

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

gkistanova accepted this revision.Apr 22 2021, 12:22 AM

Thanks! Looks good now.

This revision is now accepted and ready to land.Apr 22 2021, 12:22 AM