Bug 233298
| Summary: | devel/py-sarge: Update to 0.1.5 | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Ports & Packages | Reporter: | Mattias Lindvall <mattias.lindvall> | ||||||||||
| Component: | Individual Port(s) | Assignee: | Kyle Evans <kevans> | ||||||||||
| Status: | Closed FIXED | ||||||||||||
| Severity: | Affects Only Me | CC: | kevans, koobs, rm | ||||||||||
| Priority: | --- | Keywords: | easy, needs-qa | ||||||||||
| Version: | Latest | Flags: | kevans:
maintainer-feedback+
|
||||||||||
| Hardware: | Any | ||||||||||||
| OS: | Any | ||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 233770 | ||||||||||||
| Attachments: |
|
||||||||||||
Created attachment 199321 [details]
py-sarge.diff
LGTM; we'll need to find a ports committer to help shuffle it in @Kyle The patch looks fine. If it passes QA (at least [1] portlint, poudriere):
Reviewed by: koobs (ports)
Approved by: koobs (ports)
For future port updates, feel free to create reviews and cc me on them, so that you can usher those changes in yourself. If they're trivial changes (to the port), just CC me in the Bugzilla issue and confirm that QA passes, to save having to duplicate the changes into Phabricator
[1] I note the upstream sources contain tests (test_*.py) and the package supports running python setup.py test command to run them (see setup.py), but the files are not included in the (sdist).
It would be nice to get these files included so that QA for this port/package could be more comprehensive. Asking upstream to add those files to MANIFEST.in should be sufficient to include them in the PyPI source distribution, at which point adding the following to the port should run the tests:
do-test:
cd ${WRKSRC} && ${PYTHON_CMD} ${PYDISTUTILS_SETUP} test
(In reply to Kubilay Kocak from comment #3) Hi Koobs, I'll double-check w/ portlint and poudriere. Can I also shift MAINTAINER over to submitter with your approval pre-commit, or should we submit a new diff with that change for your review? I wasn't sure since it's a one-line change that's fairly hard to goof up. =) (In reply to Kyle Evans from comment #4) Sure you *can*, but not that it needs to be reviewed because its a small change (though we have seen MAINTAINER typo'd in commits in the past), it's good to have positive evidence that Reporter (Submitter) requests/wants MAINTAINER'ship. That ideally takes the form of a patch from Reporter (Submitter) with the MAINTAINER change, that you (current MAINTAINER) then maintainer-approval -> +'s. tldr, its better to have a canonical reference patch (phabricator or bugzilla attachment) that corresponds exactly with the later commit as it reduces ambiguity and is explicit, but practicality can often beat purity. If Matthias can update the patch, that would be great. The previous Approval (from comment 3) still applies (pending QA pass) and no further Approval is necessary. (In reply to Kubilay Kocak from comment #5) Sure, having Mattias update the patch to include the MAINTAINER change makes sense -- previous indications along these lines were all in private communication. `portlint -cAC` (because more flags are better) reports no issues `poudriere testport` has no strange output Mattias: Also, for the new patch, please double-check the line endings before uploading. This one was DOS-style and required some manual massaging to get the patch to apply and highlighted what I can now consider a major oversight in our sed not handling "\r" properly. =) Created attachment 200037 [details]
py-sarge.diff
(In reply to Kyle Evans from comment #6) Indeed the old patch file had CRLF line endings, sorry about that, I really don't know how that happened. Uploaded a new patch, which updates MAINTAINER, and has LF line endings. Comment on attachment 200037 [details]
py-sarge.diff
Reviewed by: koobs
Approved by: koobs (ports)
Add # of PR that is blocked with this update Please also note that license is BSD3C, not BSD2C. And pkg-descr has some nits (empty lines in the head of files, http urls, that are redirecting to https now). (In reply to Ruslan Makhmatkhanov from comment #11) Ok, I will submit a new patch with fixes to these things in the next day or so! Created attachment 200113 [details]
py-sarge.diff
Fixed additional review comments!
Hi koobs, Can I poke you for approval on this one last time, please, based on this latest revision? It addresses the issues pointed out by Ruslan -- I'm not sure how I missed these things when I initially created it. =( Comment on attachment 200113 [details]
py-sarge.diff
Reviewed by: koobs (ports)
Approved by: koobs (ports)
Hey Mattias, Do you happen to know any of the highlights of the 0.1.4 => 0.1.5 update? Their changelog is empty and I'm afraid I don't follow sarge development beyond the initial port and my use of it in OctoPrint. If not, that's also cool- we at least know that it fixed some compatibility nits with 3.7. (In reply to Kyle Evans from comment #16) This link seems to contain the relevant information: https://sarge.readthedocs.io/en/latest/overview.html#change-log A commit references this bug: Author: kevans Date: Thu Dec 20 02:07:34 UTC 2018 New revision: 487826 URL: https://svnweb.freebsd.org/changeset/ports/487826 Log: devel/py-sarge: Update to 0.1.5 Highlights of the update: - Improved errors - Fixed compatibility with Python 3.7 (async is now a keyword) - Updated tutorial example on progress monitoring PR: 233298 Submitted by: Mattias Lindvall <mattias.lindvall@gmail.com> Reviewed by: koobs (ports) Approved by: koobs (ports), kevans (maintainer) Changes: head/devel/py-sarge/Makefile head/devel/py-sarge/distinfo head/devel/py-sarge/pkg-descr (In reply to Mattias Lindvall from comment #17) Clearly it is my eyes that are empty. =) Committed, thanks! |
Created attachment 199320 [details] update to version 0.1.5.post0 Update from 0.1.4 to 0.1.5.post0