Bug 1402540 - Review Request: git-fame - Pretty-print git repository collaborators sorted by contributions
Summary: Review Request: git-fame - Pretty-print git repository collaborators sorted b...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Igor Gnatenko
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2016-12-07 18:49 UTC by Fabio Alessandro Locati
Modified: 2016-12-19 07:52 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-12-19 06:02:40 UTC
Type: ---
Embargoed:
ignatenko: fedora-review+


Attachments (Terms of Use)


Links
System ID Private Priority Status Summary Last Updated
Red Hat Bugzilla 1405639 0 unspecified CLOSED python-docopt requires devel/test libraries 2021-02-22 00:41:40 UTC

Internal Links: 1405639

Description Fabio Alessandro Locati 2016-12-07 18:49:50 UTC
Spec URL: https://fale.fedorapeople.org/rpms/git-fame.spec
SRPM URL: https://fale.fedorapeople.org/rpms/git-fame-1.1.0-1.fc25.src.rpm
Description: Pretty-print git repository collaborators sorted by contributions
Fedora Account System Username: fale

Comment 1 Raphael Groner 2016-12-07 22:43:07 UTC
Informal review:

> License:        [MPLv2.0](https://mozilla.org/MPL/2.0/)

Please use License: MPLv2.0
https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#Good_Licenses

> # https://github.com/casperdcl/git-fame/pull/2
> Source1:        LICENSE.txt
> …
> %files
> %license LICENSE.txt

It's not allowed to maintainers to add their own license. You should wait for upstream to properly add a license file. I'm not sure if it's allowed to redistribute MPLv2.0 without a proper license text bundled.
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text

> Requires:       python3-setuptools

Why need setuptools for runtime?

Comment 2 Vasiliy Glazov 2016-12-09 11:17:16 UTC
It is crash on big repo (~4GB).

$ git fame
Blame:   0%|                                                                            | 5/15258 [00:01<1:02:24,  4.07it/s]
Traceback (most recent call last):
  File "/usr/libexec/git-core/git-fame", line 9, in <module>
    load_entry_point('git-fame==1.1.0', 'console_scripts', 'gitfame')()
  File "/usr/lib/python3.5/site-packages/gitfame/_gitfame.py", line 239, in main
    run(args)
  File "/usr/lib/python3.5/site-packages/gitfame/_gitfame.py", line 181, in run
    auths = RE_AUTHS.findall(blame_out)
TypeError: cannot use a string pattern on a bytes-like object

Comment 3 Fabio Alessandro Locati 2016-12-09 11:30:36 UTC
@Rapahel: AFAIK, there is no such requrement

@Vasiliy: The problem is not the repo size, but the presence of UTF characters. I'm working with upstream about that, as for https://github.com/casperdcl/git-fame/issues/3 and https://github.com/casperdcl/git-fame/pull/4. The patch is ready and working.

I'll try to have a new version with both the License inclusion and the bugfix.

Cheers

Comment 4 Igor Gnatenko 2016-12-09 11:40:58 UTC
* Consider taking REAL sources, not the PyPI waste
* /usr/libexec -> %{_libexecdir}
* /usr/bin -> %{_bindir}
* %{python3_sitelib}/gitfame -> %{python3_sitelib}/gitfame/
* %{python3_sitelib}/git_fame-%{version}-py?.?.egg-info -> %{python3_sitelib}/git_fame-*/
* I think Requires: git is missing (or even git-core, depends what it really requires)
* Consider using Cython version, because it's faster (BR: python3-Cython and --cython to the %py3_build/install executions)
* I would also add Recommends: python3-tabulate
* Tests are not ran

Comment 5 Fabio Alessandro Locati 2016-12-10 17:36:55 UTC
Thanks Igor :)

* Consider taking REAL sources, not the PyPI waste
* /usr/libexec -> %{_libexecdir}
* /usr/bin -> %{_bindir}
* %{python3_sitelib}/gitfame -> %{python3_sitelib}/gitfame/
* %{python3_sitelib}/git_fame-%{version}-py?.?.egg-info -> %{python3_sitelib}/git_fame-*/

Should all be done till here

* I think Requires: git is missing (or even git-core, depends what it really requires)

Git is actually not required

* Consider using Cython version, because it's faster (BR: python3-Cython and --cython to the %py3_build/install executions)

Not sure if this would give us real advantages

* I would also add Recommends: python3-tabulate

In v1.2 is now needed, so I've included as required

* Tests are not ran

They need some packages that are not present in Fedora (ie: nose-timer)

SPEC: https://fale.fedorapeople.org/rpms/git-fame.spec
SRPM: https://fale.fedorapeople.org/rpms/git-fame-1.2.0-1.fc25.src.rpm

Comment 6 Igor Gnatenko 2016-12-10 19:50:07 UTC
git is required at least for directory ownership. but it's spawned by subprocess as well.

Comment 7 Gwyn Ciesla 2016-12-12 14:07:49 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/git-fame

Comment 8 Raphael Groner 2016-12-12 19:41:08 UTC
Please fix (again) while importing:

- Please use License: MPLv2.0

- Remove unneeded Requires: python3-setuptools

- I think also the package has to explicitly 'Require: git' as said to provide
  a proper function.

Comment 9 Fedora Update System 2016-12-12 20:01:40 UTC
git-fame-1.2.0-1.fc25 has been submitted as an update to Fedora 25. https://bodhi.fedoraproject.org/updates/FEDORA-2016-8fd51877c2

Comment 10 Fedora Update System 2016-12-12 20:01:50 UTC
git-fame-1.2.0-1.fc24 has been submitted as an update to Fedora 24. https://bodhi.fedoraproject.org/updates/FEDORA-2016-a20a6ee060

Comment 11 Fedora Update System 2016-12-13 05:28:25 UTC
git-fame-1.2.0-1.fc24 has been pushed to the Fedora 24 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-a20a6ee060

Comment 12 Fedora Update System 2016-12-13 05:31:45 UTC
git-fame-1.2.0-1.fc25 has been pushed to the Fedora 25 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2016-8fd51877c2

Comment 13 Raphael Groner 2016-12-16 20:32:49 UTC
DNF tells me about required dependencies to install the git-fame package:
 git-core
 python-rpm-macros                                             
 python3-devel                                                 
 python3-docopt                                                
 python3-nose                                                  
 python3-py                                                    
 python3-pytest                                                
 python3-rpm-macros                                            
 python3-tabulate                                              
 python3-tqdm                                                  

Why python-rpm-macros, python3-devel, python3-nose, python3-py, python3-pytest and python3-rpm-macros? Normally, those packages are needed to build only but not for runtime.

Comment 14 Igor Gnatenko 2016-12-16 21:21:02 UTC
(In reply to Raphael Groner from comment #13)
> DNF tells me about required dependencies to install the git-fame package:
>  git-core
>  python-rpm-macros                                             
>  python3-devel                                                 
>  python3-docopt                                                
>  python3-nose                                                  
>  python3-py                                                    
>  python3-pytest                                                
>  python3-rpm-macros                                            
>  python3-tabulate                                              
>  python3-tqdm                                                  
> 
> Why python-rpm-macros, python3-devel, python3-nose, python3-py,
> python3-pytest and python3-rpm-macros? Normally, those packages are needed
> to build only but not for runtime.

Looks like some transient deps. Can you file a new bug against git-fame and after debugsolving I will reassign to proper component?

Comment 15 Igor Gnatenko 2016-12-16 22:44:45 UTC
(In reply to Raphael Groner from comment #13)
> DNF tells me about required dependencies to install the git-fame package:
>  git-core
>  python-rpm-macros                                             
>  python3-devel                                                 
>  python3-docopt                                                
>  python3-nose                                                  
>  python3-py                                                    
>  python3-pytest                                                
>  python3-rpm-macros                                            
>  python3-tabulate                                              
>  python3-tqdm                                                  
> 
> Why python-rpm-macros, python3-devel, python3-nose, python3-py,
> python3-pytest and python3-rpm-macros? Normally, those packages are needed
> to build only but not for runtime.

Fixed in https://bugzilla.redhat.com/show_bug.cgi?id=1405639.

Comment 16 Fedora Update System 2016-12-19 06:02:40 UTC
git-fame-1.2.0-1.fc25 has been pushed to the Fedora 25 stable repository. If problems still persist, please make note of it in this bug report.

Comment 17 Fedora Update System 2016-12-19 07:52:46 UTC
git-fame-1.2.0-1.fc24 has been pushed to the Fedora 24 stable repository. If problems still persist, please make note of it in this bug report.


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