devel/buildbot*: Update to 0.9.11, Add
ClosedPublic

Authored by rodrigc on Sep 23 2017, 7:51 PM.

Details

Reviewers
koobs
grembo
asomers
sunpoet
Group Reviewers
Python
Summary
devel/buildbot*: Update to 0.9.11, Add buildbot-grid-view

* Update to buildbot 0.9.11
* Add required devel/py-buildbot-grid-view dependency
* Clean up stuff from individual Makefiles that was copy/pasted from the main buildbot Makefile
* buildbot-www, buildbot-grid-view, buildbot-console-view, buildbot-waterfall-view cannot be tested outside
   of buildbot, so remove stuff from their Makefiles to simplify.

Reviewed_by: koobs, ???
Approved by:
Differential_Revision: D12479
Test Plan
  • portlint: OK (all ports)
  • testport: OK (poudriere: 10.3-RELEASE-amd64, Python 2.7)
  • maketest: OK (4 tests failing, 5060 passing for buildbot, 0 tests failing, 229 passing for buildbot-worker)

Diff Detail

Repository
rP FreeBSD ports repository
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11715
Build 12061: arc lint + arc unit
rodrigc created this revision.Sep 23 2017, 7:51 PM
rodrigc edited the summary of this revision. (Show Details)Sep 23 2017, 7:56 PM
rodrigc edited the test plan for this revision. (Show Details)Sep 23 2017, 7:58 PM
rodrigc edited the test plan for this revision. (Show Details)
asomers requested changes to this revision.Sep 23 2017, 9:37 PM

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.

devel/buildbot-grid-view/Makefile
13 ↗(On Diff #33364)

MY_DEPENDS is unconventional, and portlint complains about it. The more typical thing to do would be:

BUILD_DEPENDS=        buildbot-pkg>=${PORTVERSION}:devel/buildbot-pkg
RUN_DEPENDS=         ${BUILD_DEPENDS}
devel/buildbot-grid-view/pkg-descr
2 ↗(On Diff #33364)

Trailing period, please.

devel/buildbot/Makefile
28

You should add buildbot-www, otherwise dozens of tests get skipped.

This revision now requires changes to proceed.Sep 23 2017, 9:37 PM
koobs retitled this revision from Update buildbot to 0.9.11 and add buildbot-grid-view to devel/buildbot*: Update to 0.9.11, Add buildbot-grid-view.Sep 24 2017, 12:34 AM
koobs edited the test plan for this revision. (Show Details)
rodrigc added inline comments.Sep 24 2017, 12:35 AM
devel/buildbot-grid-view/Makefile
13 ↗(On Diff #33364)

Portlint complains about what you wrote:

RUN_DEPENDS should not be set to ${BUILD_DEPENDS} as ${BUILD_DEPENDS} includes other implicit dependencies.  Instead, copy the explicit dependencies from BUILD_DEPENDS to RUN_DEPENDS.  See http://www.freebsd.org/doc/en_US.ISO8859-1/books/porters-handbook/makefile-depend.html#AEN2154 for more details.
koobs requested changes to this revision.Sep 24 2017, 12:35 AM
koobs edited the summary of this revision. (Show Details)

Could you please add change of maintainer to MAINTAINER=koobs@f.o (per Bugzilla #220564 Comment 10)

devel/buildbot-grid-view/pkg-descr
6 ↗(On Diff #33364)

Use https URL (where/when supported)

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)

koobs added a comment.EditedSep 24 2017, 12:45 AM

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

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

rodrigc updated this revision to Diff 33367.Sep 24 2017, 12:51 AM
  • Add missing periods.
  • Remove patch which is no longer needed.
  • Remove MY_DEPENDS
rodrigc updated this revision to Diff 33368.Sep 24 2017, 1:15 AM
  • Use https://
  • Use ${PYTHON_CMD} -m twisted.trial to invoke test utility
koobs added a comment.Sep 24 2017, 2:03 AM

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

rodrigc updated this revision to Diff 33369.Sep 24 2017, 2:30 AM
  • Remove concurrent
  • Add back python:-2.7
rodrigc edited the test plan for this revision. (Show Details)Sep 24 2017, 2:37 AM
rodrigc edited the summary of this revision. (Show Details)Sep 24 2017, 2:39 AM
koobs added a comment.Sep 24 2017, 2:40 AM

@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.

koobs added a comment.Sep 24 2017, 2:43 AM

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)

koobs accepted this revision.Sep 24 2017, 2:47 AM
koobs edited the test plan for this revision. (Show Details)

Missed this due to log output:

How does make test go?

koobs added 1 blocking reviewer(s): grembo.Sep 24 2017, 2:48 AM

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.

koobs retitled this revision from devel/buildbot*: Update to 0.9.11, Add buildbot-grid-view to devel/buildbot*: Update to 0.9.11, Add.Sep 24 2017, 2:58 AM
koobs edited the summary of this revision. (Show Details)
koobs added a comment.Sep 24 2017, 3:02 AM

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 :)

asomers accepted this revision.Sep 24 2017, 3:17 AM

Apart from buildbot-worker's patch file change, it all looks good to me. FWIW, I also tested the WWW server and worker with the example pyflakes project.

devel/buildbot-worker/files/patch-setup.py
3

Was it really necessary to recreate this patch file? I usually don't recreate patch files that still I apply; I figure it just adds line noise to the diff.

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

rodrigc edited the test plan for this revision. (Show Details)Sep 24 2017, 3:46 AM
koobs added a comment.Sep 24 2017, 3:49 AM

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

rodrigc updated this revision to Diff 33370.Sep 24 2017, 4:08 AM

Fix test targets. Cannot run inside WRKSRC, must run against installed versions.
Put trial temporary files in a separate tmp directory.

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

grembo accepted this revision.Sep 24 2017, 11:28 AM

@koobs Just glanced at it, trusting your judgement.

One more thing the ports inherited and that I wanted to change, but didn't get to is in buildbot-www/Makefile:

post-patch:
        @${FIND} ${WRKSRC} -type f | ${XARGS} -n 10 ${REINPLACE_CMD} -e \
                's|/usr/bin/python|${PYTHON_CMD}|g'

This adds newlines at the end of files which don't have it, which includes files in buildbot_www/static/fonts - as a result those are corrupted (fonts still load, but cause errors on the browser console).

This revision is now accepted and ready to land.Sep 24 2017, 11:28 AM
sunpoet accepted this revision.Sep 24 2017, 12:37 PM
sunpoet added a subscriber: sunpoet.

LGTM with the minor changes. Thanks!

devel/buildbot-console-view/Makefile
15

You could use RUN_DEPENDS:=${BUILD_DEPENDS} instead.

17

Please use USES=python:2.7.

devel/buildbot-pkg/Makefile
14

Please use USES=python:2.7.

devel/buildbot-waterfall-view/Makefile
15

You could use RUN_DEPENDS:=${BUILD_DEPENDS} instead.

17

Please use USES=python:2.7.

devel/buildbot-worker/Makefile
20

Please use USES=python:2.7.

devel/buildbot-www/Makefile
22

Please use USES=python:2.7.

devel/buildbot/Makefile
32

Please use USES=python:2.7.

41–43

Please add

REINPLACE_ARGS=-i ''

in USES section to avoid creating .bak files.

devel/py-buildbot-grid-view/Makefile
16

Please use USES=python:2.7.

mat added inline comments.Sep 25 2017, 8:11 PM
devel/buildbot-console-view/Makefile
15

Or even better:

BUILD_DEPENDS=${RUN_DEPENDS}

and you would not have to use :=

devel/buildbot-waterfall-view/Makefile
15

or:

BUILD_DEPENDS= ${RUN_DEPENDS}
devel/buildbot-worker/Makefile
24

Why += ?

devel/buildbot-worker/files/patch-setup.py
3

When the line numbers change, it is usually better to update the patches, so that things do not drift too much appart.

devel/buildbot/Makefile
41–43

Or USES=shebangfix

rodrigc marked 2 inline comments as done.Sep 27 2017, 6:49 PM

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

koobs added a comment.EditedSep 29 2017, 2:17 PM

@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.