test target (for Python packages) should invoke a specific version of Python, by whatever method, rather than LOCALBASE scripts, which can/may point to any (and incorrect) Python versions at any time.

Examples:

• ${PYTHON_CMD} -m {trial|trial.whatever}
• ${PYTHON_CMD} ${PYDISTUTILS_SETUP} test (if supported)

Also, please confirm QA (ive added portlint, maketest lines to TEST PLAN section)

Lastly, and this has been coming a long-time, but buildbot* ports have been non-compliance with Python packages since their birth:

• naming, not including a py- prefix, AND
• not using PYTHON_PKGNAMEPREFIX

With buildbot now supporting Python 2/3 this is effectively a blocker as multiple installed versions will conflict (and there's no better time than now, less ideally, after this commit).

This also means that buildbot ports will need to become USE_PYTHON=concurrent safe.

Alternatively, I'm happy to defer this to a separate logically isolated commit, to land immediately after this, as long as the relaxing of the USES=python version is removed, and lands with that next change. This removes a window within which multiple concurrent versions 'can' be attempted, but wont (but should) work due to conflicts

Edit:

Having had time for it to soak in a bit more, I definately prefer to see the rename (move), prefixing and concurrent enabling changes in a separate commit, ideally prior to this, but if that's too annoying, after this change is OK

In D12479#258626, @asomers wrote:

It looks like devel/buildbot/files/patch-buildbot-secrets is out of date. It still applies, but the result is that buildbot/secrets/providers/{base.py, file.py, vault.py} get duped, and there may be similar errors in other files too. Running "make test" in devel/buildbot detects that error. You should investigate. We can probably delete the entire patch.

This patch is no longer needed since this issue was fixed: https://github.com/buildbot/buildbot/issues/3086

Also, can gridview be added separately (after landing 0.9.11), or does 0.9.11 require it?

@rodrigc Thanks for the detail, though logs aren't explicitly sought (at least by me :])

Putting in "OK's" (or summaries) is fine for TEST PLAN.

This looks OK to land, unless the gridview port addition can be made in a separate commit (preferred), either before this commit if it supports the current port version, or after if it requires 0.9.11.

If this change requires gridview to be added (a new compulsory module, etc), then looks good to go (pending maintainer approval)

In D12479#258670, @koobs wrote:

This looks OK to land, unless the gridview port addition can be made in a separate commit (preferred), either before this commit if it supports the current port version, or after if it requires 0.9.11.

If this change requires gridview to be added (a new compulsory module, etc), then looks good to go (pending maintainer approval)

buildbot-gridview needs to go in.

If @grembo is happy with (solely) my review and doesn't have time, he can just accept, in which case, I'm happy for this to land pending:

• Confirmation that maketest passes
• Updating of relevant Property: lines in commit log message (SUMMARY section)

Note: Don't forget to remove underscores in those Property_lines on commit otherwise it wont auto-close :)

portlint complained about the patch file, so I regenerated it.

If the 4 failing look like false positives, non-trivial or non-concerning, feel free to defer (Disclaimer: I am not MAINTAINER yet)

The 4 failing tests looks pretty minor and not worth blocking progress on this.

@koobs why wasn't this review updated after I committed rP450642 ?

In D12479#259604, @rodrigc wrote:

@koobs why wasn't this review updated after I committed rP450642 ?

Did not remove underscore in Differential_Revision: line in copy/pasted SUMMARY on on commit

Unfortunately, Phabricator doesn't allow 'Differential Revision' in fields, even in code blocks.

The post commit hook could have its regex syntax relaxed to allow (match) it.