Bug 233298 - devel/py-sarge: Update to 0.1.5
Summary: devel/py-sarge: Update to 0.1.5
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Kyle Evans
Keywords: easy, needs-qa
Depends on:
Blocks: 233770
  Show dependency treegraph
Reported: 2018-11-18 18:38 UTC by Mattias Lindvall
Modified: 2018-12-20 02:10 UTC (History)
3 users (show)

See Also:
kevans: maintainer-feedback+

update to version 0.1.5.post0 (946 bytes, text/plain)
2018-11-18 18:38 UTC, Mattias Lindvall
no flags Details
py-sarge.diff (946 bytes, patch)
2018-11-18 18:42 UTC, Mattias Lindvall
kevans: maintainer-approval+
Details | Diff
py-sarge.diff (1010 bytes, patch)
2018-12-11 12:54 UTC, Mattias Lindvall
koobs: maintainer-approval+
Details | Diff
py-sarge.diff (1.71 KB, patch)
2018-12-14 12:13 UTC, Mattias Lindvall
koobs: maintainer-approval+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mattias Lindvall 2018-11-18 18:38:24 UTC
Created attachment 199320 [details]
update to version 0.1.5.post0

Update from 0.1.4 to 0.1.5.post0
Comment 1 Mattias Lindvall 2018-11-18 18:42:29 UTC
Created attachment 199321 [details]
Comment 2 Kyle Evans freebsd_committer 2018-12-10 13:58:24 UTC
LGTM; we'll need to find a ports committer to help shuffle it in
Comment 3 Kubilay Kocak freebsd_committer freebsd_triage 2018-12-10 14:46:05 UTC
@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:

Comment 4 Kyle Evans freebsd_committer 2018-12-10 15:00:16 UTC
(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. =)
Comment 5 Kubilay Kocak freebsd_committer freebsd_triage 2018-12-11 03:10:22 UTC
(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.
Comment 6 Kyle Evans freebsd_committer 2018-12-11 04:28:43 UTC
(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. =)
Comment 7 Mattias Lindvall 2018-12-11 12:54:26 UTC
Created attachment 200037 [details]
Comment 8 Mattias Lindvall 2018-12-11 12:57:56 UTC
(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 9 Kubilay Kocak freebsd_committer freebsd_triage 2018-12-12 04:27:01 UTC
Comment on attachment 200037 [details]

Reviewed by: koobs
Approved by: koobs (ports)
Comment 10 Ruslan Makhmatkhanov freebsd_committer 2018-12-12 21:14:51 UTC
Add # of PR that is blocked with this update
Comment 11 Ruslan Makhmatkhanov freebsd_committer 2018-12-12 21:16:50 UTC
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).
Comment 12 Mattias Lindvall 2018-12-13 21:50:29 UTC
(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!
Comment 13 Mattias Lindvall 2018-12-14 12:13:10 UTC
Created attachment 200113 [details]

Fixed additional review comments!
Comment 14 Kyle Evans freebsd_committer 2018-12-18 03:29:18 UTC
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 15 Kubilay Kocak freebsd_committer freebsd_triage 2018-12-18 03:35:22 UTC
Comment on attachment 200113 [details]

Reviewed by: koobs (ports)
Approved by: koobs (ports)
Comment 16 Kyle Evans freebsd_committer 2018-12-19 14:18:46 UTC
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.
Comment 17 Mattias Lindvall 2018-12-19 23:54:45 UTC
(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
Comment 18 commit-hook freebsd_committer 2018-12-20 02:07:54 UTC
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

  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)

Comment 19 Kyle Evans freebsd_committer 2018-12-20 02:10:39 UTC
(In reply to Mattias Lindvall from comment #17)

Clearly it is my eyes that are empty. =) Committed, thanks!