Bug 1310607 - Review Request: qutebrowser - A keyboard-driven, vim-like browser based on PyQt5 and QtWebKit
Review Request: qutebrowser - A keyboard-driven, vim-like browser based on Py...
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Bohuslav "Slavek" Kabrda
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2016-02-22 05:51 EST by Tomas Orsava
Modified: 2016-02-23 05:28 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2016-02-23 05:28:55 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
bkabrda: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Tomas Orsava 2016-02-22 05:51:24 EST
Spec URL: https://torsava.fedorapeople.org/qutebrowser.spec
SRPM URL: torsava.fedorapeople.org/qutebrowser-0.5.1-1.fc23.src.rpm

Description: qutebrowser is a keyboard-focused browser with a minimal GUI. It’s based on Python, PyQt5 and QtWebKit and free software, licensed under the GPL. It was inspired by other browsers/addons like dwb and Vimperator/Pentadactyl.

Fedora Account System Username: torsava

Koji build rawhide: http://koji.fedoraproject.org/koji/taskinfo?taskID=13091206
Koji build f23: http://koji.fedoraproject.org/koji/taskinfo?taskID=13091190
Koji build f22: http://koji.fedoraproject.org/koji/taskinfo?taskID=13091184
Comment 1 Bohuslav "Slavek" Kabrda 2016-02-22 05:52:33 EST
Taking this for review.
Comment 2 Bohuslav "Slavek" Kabrda 2016-02-22 06:14:45 EST
- All the dependencies are used in python3 version, but cssutils is python2 - is there any reason for that? IMO it's a typo.
- There are quite a lot hashbangs that point to /usr/bin/env. While there is nothing in guidelines (that I know of) that would prohibit this, I'd advise changing these to point to /usr/bin/python3. Consider this - someone might try to run this in an environment where "/usr/bin/env python3" points to something else than /usr/bin/python3. qutebrowser would likely fail since it wouldn't find dependencies (dependencies that it has been built with and tested with by you as a packager). By replacing the hashbangs, qutebrowser can keep working even in environments like this (well, for the most part, anyway).

Otherwise everything looks good - the package builds and runs fine, rpmlint output is clean, licensing is good and specfile looks very nice. Once the above problems are solved, this package can be approved (oh, and I like that it's Python 3!)
Comment 3 Tomas Orsava 2016-02-22 09:49:08 EST
(In reply to Bohuslav "Slavek" Kabrda from comment #2)
> - All the dependencies are used in python3 version, but cssutils is python2
> - is there any reason for that? IMO it's a typo.

Since cssutils is only an optional dependency, I have commented that line out to be readded in the future when cssutils is packaged for Python 3.

> - There are quite a lot hashbangs that point to /usr/bin/env. While there is
> nothing in guidelines (that I know of) that would prohibit this, I'd advise
> changing these to point to /usr/bin/python3. Consider this - someone might
> try to run this in an environment where "/usr/bin/env python3" points to
> something else than /usr/bin/python3. qutebrowser would likely fail since it
> wouldn't find dependencies (dependencies that it has been built with and
> tested with by you as a packager). By replacing the hashbangs, qutebrowser
> can keep working even in environments like this (well, for the most part,
> anyway).

I have added a line into the %build section that changes those hashbangs accordingly.

> 
> Otherwise everything looks good - the package builds and runs fine, rpmlint
> output is clean, licensing is good and specfile looks very nice. Once the
> above problems are solved, this package can be approved (oh, and I like that
> it's Python 3!)

Thanks for all your input, hopefully now it'll be all ok!

Spec URL: https://torsava.fedorapeople.org/qutebrowser.spec
SRPM URL: https://torsava.fedorapeople.org/qutebrowser-0.5.1-1.fc23.src.rpm
Comment 4 Bohuslav "Slavek" Kabrda 2016-02-22 10:10:42 EST
LGTM, APPROVED.

Note for future reviews, that it is customary to include a changelog entry and mention your changes during review there (for every review round, if there are more). Let's leave the specfile as it is for this instance.
Comment 5 Tomas Orsava 2016-02-22 10:22:08 EST
Ah, sorry, next time I'll make it so. Thanks!
Comment 6 Gwyn Ciesla 2016-02-22 14:50:53 EST
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/qutebrowser

Note You need to log in before you can comment on or make changes to this bug.