Bug 543425 - Review Request: gource - Software version control visualization
Summary: Review Request: gource - Software version control visualization
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 565838 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-12-02 11:16 UTC by Siddhesh Poyarekar
Modified: 2015-09-14 00:21 UTC (History)
11 users (show)

Fixed In Version: gource-0.24-1.fc12
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2010-02-18 06:52:04 UTC
Type: ---
mtasaka: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)

Description Siddhesh Poyarekar 2009-12-02 11:16:46 UTC
SPEC: http://people.redhat.com/spoyarek/gource/gource.spec
SRPM: http://people.redhat.com/spoyarek/gource/gource-0.18-1.fc12.src.rpm

Description:

OpenGL-based 3D visualisation tool for source control repositories.
The repository is displayed as a tree where the root of the repository is
the centre, directories are branches and files are leaves. Contributors
to the source code appear and disappear as they contribute to specific
files and directories.

Comment 1 Siddhesh Poyarekar 2009-12-02 11:40:27 UTC
Updated the SPEC and SRPM after rpmlint'ing the packages and specfile. Thanks Rahul Sundaram!

Comment 2 Rakesh Pandit 2009-12-03 06:09:24 UTC
From last comment it seems you have done some update. If you change your spec or srpm, please do a release tag bump and write down changes in ChangeLog and post the new urls.

Thanks,

Comment 3 Rakesh Pandit 2009-12-03 06:21:53 UTC
While skimming I found an unowned directory /usr/share/gource/ 

https://fedoraproject.org/wiki/Packaging:UnownedDirectories

Comment 4 Rakesh Pandit 2009-12-03 06:30:24 UTC
""
autoreconf
%configure --prefix=/usr
make
""

needs to be replaced with

""
%configure
make %{?_smp_mflags}
""

%configure macro already has --prefix=/usr and other options and a lone make is not advisable.

moreover timestamp needs to be saved, while copying files into BUILDROOT in %install section.

I would also suggest to try and get rid of patch for version correction, as running autoreconf is not advised. If you really care about package version, try sed in configure script.

Thanks,

Comment 5 Robert Scheck 2009-12-03 06:43:43 UTC
Running autoreconf is not a real problem. I assume, that patch makes it
anyway upstream and won't be required in the next release of gource, yes?
Keeping the timestamps is not a must, it's just suggested by the way...

Comment 6 Ralf Corsepius 2009-12-03 07:01:14 UTC
(In reply to comment #5)
> Running autoreconf is not a real problem.
I disagree - People who do so are exposing themselves and their package's users to avoidable risks.

> Keeping the timestamps is not a must, it's just suggested by the way...  
This suggestions is based on ill assumptions. Keeping timestamps is technically non-required eye-candy.

Comment 7 Robert Scheck 2009-12-03 08:34:32 UTC
Siddhesh, I had a look to your spec file. A few thoughts and comments:

- You are currently mixing $RPM_BUILD_ROOT and %{buildroot}, choose one and
  please keep it consistent (https://fedoraproject.org/wiki/PackagingGuidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS)

- I can't see a reason to have the "Requires:" lines inside of the spec file.
  If your software links to a library, rpmbuild is catching up the dependency
  in the end of the build process and adds "Requires:" to the required libs.

- Why do you do "%configure --prefix=/usr" rather "%configure"? Is there a real
  reason for? If you do "rpm --eval '%configure'", you see what %configure will
  be expanded to.

- You want to use parallel make for making your package. If the software does
  not support it right now, please make a comment and otherwise please use it:
  https://fedoraproject.org/wiki/PackagingGuidelines#Parallel_make

- You might want to preserve timestamps by appending INSTALL="install -p" to
  the "make install" command. As Ralf agreed with me, it is not required on a
  technically base, Guidelines just suggest it, see e.g.
  https://fedoraproject.org/wiki/PackagingGuidelines#Timestamps

- As far as I can see, you're missing "BuildRequires: freetype-devel", otherwise
  I had trouble to rebuild the package

- Can you please choose a valid BuildRoot tag from the available list? 
  https://fedoraproject.org/wiki/PackagingGuidelines#BuildRoot_tag

- You are missing the pareparation of BuildRoot in %install section, see
  https://fedoraproject.org/wiki/PackagingGuidelines#Prepping_BuildRoot_For_.25install

- I'm wondering about the following lines in your spec file:
  %dir %{_datadir}/gource/*
  %dir %{_mandir}/man*/*
  %dir %{_datadir}/gource/fonts/*
  Have a look at: https://fedoraproject.org/wiki/Packaging:UnownedDirectories

- Can you please communicate with upstream and ensure that your patch really
  makes it into the next release of gource? Thank you.

Comment 8 Robert Scheck 2009-12-03 08:40:38 UTC
According to FAS, you're not yet a packager, so we need to block FE-NEEDSPONSOR
bug report according to https://fedoraproject.org/wiki/Package_Review_Process.

I'm resetting the fedora‑review flag to empty, as only the reviewer, not the
packager should ever set and change it.

Sorry, but I'm not a sponsor for packaging, so I'm now removing myself from 
assigned. Nevertheless you should have a look to my comments and work on it.

Comment 9 Siddhesh Poyarekar 2009-12-03 09:18:47 UTC
The upstream developer has accepted the patch, so we should see it in the next upstream version:

http://code.google.com/p/gource/issues/detail?id=21

I'll work on the spec file and revert with an update (with a bumped release number this time :) ). Thanks Robert, Rakesh and Ralf!

Comment 10 Siddhesh Poyarekar 2009-12-05 06:31:33 UTC
Uploaded updated spec files based on feedback in all comments above:

http://people.redhat.com/spoyarek/gource/gource.spec
http://people.redhat.com/spoyarek/gource/gource-0.18-3.fc12.src.rpm

I've kept the autoreconf in place for now to cater for any autotools file patches in the future. I've also dropped the configure.ac patch for now since it does not really break anything. Accordingly, I've also dropped autoreconf since it's not necessary anymore.

Comment 11 Siddhesh Poyarekar 2009-12-05 06:32:57 UTC
Sorry for the mixed up comment earlier, I'll rephrase that:

I've dropped the configure.ac patch for now since it does not really break anything. Accordingly, I've also dropped autoreconf since it's not necessary anymore.

Comment 12 Stefan Riemens 2009-12-11 11:18:04 UTC
just a few comments:
- Please use %global instead of %define
- (already mentioned) If parallel make doesn't work, please mention that in a comment. Otherwise, the make line should just read make %{?_smp_mflags}

I'm not a sponsor either...
Stefan

Comment 13 Siddhesh Poyarekar 2009-12-29 12:15:25 UTC
Thanks Stefan. Implemented your suggestions.

Updated spec and source package. Rebased to new upstream release too:

http://www.siddhesh.in/hacks/gource.spec
http://www.siddhesh.in/hacks/gource-0.23-1.fc13.src.rpm

Comment 14 Mamoru TASAKA 2010-01-11 18:19:29 UTC
Well,

* Version specific BuildRequires
  - %sdl_image_version, %sdl_version are not needed because
    SDL{_image} packges in currently supported Fedora branch
    all satisfies this depedency

  ! For ftgl, F-11 ftgl is still 2.1.2, so you may want to
    write this version specific BuildRequires.

* Virtual BuildRequires
  - For some reasons, we prefers to use
    - "BuildRequires: libGL-devel" instead of mesa-libGL-devel
    - and "libGLU-devel" instead of mesa-libGLU-devel

* Preserving timestamps
  - Using alias won't work when using Makefile because the
    subprocess executed by make won't recognize such shell
    builtin commands.

    For this case you have to modify Makefile.in directly
    at %prep like;
---------------------------------------------------
%prep
%setup -q
sed -i.cp -e 's|cp |cp -p |' Makefile.in
---------------------------------------------------

* Enclosed fonts
  - Currently Fedora bans to use fonts enclosed in the non-font
    source tarball and you must use fonts provided from
    font related rpms shipped in Fedora.

    Fedora uses dejanu fonts by default. If you choose to
    use /usr/share/fonts/dejavu/DejaVuSans.ttf (in 
    dejavu-sans-fonts rpm), you have to
    - Add "Requires: dejavu-sans-fonts" to this rpm
    - Replace FreeSans.ttf under %_datadir/gource/fonts/ to
      the symlink to DejaVuSans.ttf.

%files entry
  - For example %files contains
---------------------------------------------------
%files
%dir %{_datadir}/gource/fonts/*
---------------------------------------------------
    However the installed objects under %{_datadir}/gource/fonts/
    is actually a file, not a directory. So here using %dir
    is wrong.

    ! Note:  As I said above, this file should be replaced with
             a symlink
    ! Note2: You may say "but actually rpmbuild succeeds with this
             %files entry". Well, I don't know why (I commented on
             bug 505995 comment 3)

    Also. the %files entry
---------------------------------------------------
%dir %{_datadir}/gource/*
---------------------------------------------------
    also contains some files and this entry should also be fixed
    ( again due to a bug in rpm currently rpmbuild succeeds... )

Comment 15 Paul W. Frields 2010-01-14 14:52:26 UTC
Not sure if this is stalled.  Symlinking to another font is not the right way to solve this.  The right way to fix the bundled font problem:

* Add a Requires: gnu-free-sans-fonts
* In the %build section, use %configure --enable-ttf-font-dir=%{_datadir}/fonts/gnu-free/
* In the %install section, rm -rf %{buildroot}/%{_datadir}/%{name}/fonts
* Remove the line in %files referring to the bundled fonts

Comment 16 Mamoru TASAKA 2010-01-14 20:01:51 UTC
(In reply to comment #15)
> Not sure if this is stalled.  Symlinking to another font is not the right way
> to solve this.  The right way to fix the bundled font problem:
> 
> * Add a Requires: gnu-free-sans-fonts
> * In the %build section, use %configure
> --enable-ttf-font-dir=%{_datadir}/fonts/gnu-free/
> * In the %install section, rm -rf %{buildroot}/%{_datadir}/%{name}/fonts
> * Remove the line in %files referring to the bundled fonts    

Well, actually it seems that configure accepts --enable-ttf-font-dir,
however as I said in my comment 14 we use dejavu fonts by default so
unless there is some special desire please use dejavu fonts.

Comment 17 Paul W. Frields 2010-01-14 20:36:01 UTC
I don't agree, because that would require much more intrusive patching to the application code itself.  That makes no sense when we already package the requisite font.  Also, keep in mind that the font might be chosen for aesthetic reasons (out of a variety of free fonts available) by the author.

Comment 18 Mamoru TASAKA 2010-01-15 06:59:44 UTC
(In reply to comment #17)
> I don't agree, because that would require much more intrusive patching to the
> application code itself.  
- As I already said, creating symlink is enough (and many other
  packages on Fedora simply do this)

> That makes no sense when we already package the
> requisite font.  Also, keep in mind that the font might be chosen for aesthetic
> reasons (out of a variety of free fonts available) by the author.    

- This is packager's choise. 
    Note that as I already said in the comment 14 as "If you choose to
    use /usr/share/fonts/dejavu/DejaVuSans.ttf", I am not strictly
    objecting to use gnu-free-sans-fonts (one of the packages I
    maintain explicitly uses gnu-free fonts)

Comment 19 Siddhesh Poyarekar 2010-01-17 05:57:04 UTC
Thanks Mamoru san, Paul. Here are the updated spec file and source rpm:

http://www.siddhesh.in/hacks/gource.spec
http://www.siddhesh.in/hacks/gource-0.23-2.fc13.src.rpm

>  ! For ftgl, F-11 ftgl is still 2.1.2, so you may want to
>    write this version specific BuildRequires.

I already have this in place. IIUC I don't have to change anything for this right?

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 20 Mamoru TASAKA 2010-01-17 18:36:20 UTC
(In reply to comment #19)
> >  ! For ftgl, F-11 ftgl is still 2.1.2, so you may want to
> >    write this version specific BuildRequires.
> 
> I already have this in place. IIUC I don't have to change anything for this
> right?

Yes.

Now this package itself seems good, so:

-------------------------------------------------------------
NOTE: Before being sponsored:

This package will be accepted with another few (or no) work. 
But before I accept this package, someone (I am a candidate) 
must sponsor you.

Once you are sponsored, you have the right to review other 
submitters' review requests and approve the packages formally. 
For this reason, the person who want to be sponsored (like you) 
are required to "show that you have an understanding 
of the process and of the packaging guidelines" as is described
on :
http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Usually there are two ways to show this.
A. submit other review requests with enough quality.
B. Do a "pre-review" of other person's review request
   (at the time you are not sponsored, you cannot do
   a formal review)

When you have submitted a new review request or have pre-reviewed other 
person's review request, please write the bug number on this bug report 
so that I can check your comments or review request.

Fedora package collection review requests which are waiting for someone to
review can be checked on my wiki page:
http://fedoraproject.org/wiki/User:Mtasaka#B._Review_request_tickets
(Check "No one is reviewing")

Review guidelines are described mainly on:
http://fedoraproject.org/wiki/Packaging/ReviewGuidelines
http://fedoraproject.org/wiki/Packaging/Guidelines
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets
------------------------------------------------------------

Comment 21 Mamoru TASAKA 2010-01-28 15:35:47 UTC
ping?

Comment 22 Siddhesh Poyarekar 2010-01-29 06:43:13 UTC
Sorry, I haven't had the time to look into this lately. Will get something out soon and link it here, hopefully before the end of this week.

-- 
Fedora Bugzappers volunteer triage team
https://fedoraproject.org/wiki/BugZappers

Comment 23 Siddhesh Poyarekar 2010-01-30 20:18:24 UTC
I've put my review comments as comment 1 on bug 533167.

Comment 24 Mamoru TASAKA 2010-01-31 12:21:38 UTC
(In reply to comment #23)
> I've put my review comments as comment 1 on bug 533167.    

For /sbin/install-info, please check my comment on the bug.
I will approve this package.

------------------------------------------------------------
   This package (gource) is APPROVED by mtasaka
------------------------------------------------------------

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Install the Client Tools (Koji)".

Now I am sponsoring you.

If you want to import this package into Fedora 11/12, you also have
to look at
http://fedoraproject.org/wiki/Infrastructure/UpdatesSystem/Bodhi-info-DRAFT
(after once you rebuilt this package on koji Fedora rebuilding system).

If you have questions, please ask me.

Removing NEEDSPONSOR.

Comment 25 Mamoru TASAKA 2010-02-09 17:00:20 UTC
ping?

Comment 26 Siddhesh Poyarekar 2010-02-12 02:21:00 UTC
Sorry about the delay, here's the koji build:

http://koji.fedoraproject.org/koji/taskinfo?taskID=1978970

Do I request for cvs module for my package now?

Comment 27 Mamoru TASAKA 2010-02-12 12:54:13 UTC
(In reply to comment #26)
> Do I request for cvs module for my package now?    
Yes, please.

Comment 28 Siddhesh Poyarekar 2010-02-12 15:21:17 UTC
New Package CVS Request
=======================
Package Name: gource
Short Description: OpenGL-based 3D visualisation tool for source control repositories
Owners: siddhesh
Branches: F-12
InitialCC:

Comment 29 Kevin Fenzi 2010-02-13 04:15:45 UTC
CVS done (by process-cvs-requests.py).

Comment 30 Mamoru TASAKA 2010-02-16 14:38:29 UTC
*** Bug 565838 has been marked as a duplicate of this bug. ***

Comment 31 Jaroslav Reznik 2010-02-16 14:58:50 UTC
Ops,
sorry for duplicate but I've just checked F12 yum search to find if the package exists. What's the current status? There are no builds yet. Any problem I can help with?

Comment 32 Ondrej Vasik 2010-02-16 15:00:31 UTC
Sorry for duplicate review, anyway - this one imho should add and ship at least %doc COPYING README THANKS ChangeLog

Comment 33 Mamoru TASAKA 2010-02-16 16:00:59 UTC
(In reply to comment #32)
> Sorry for duplicate review, anyway - this one imho should add and ship at least
> %doc COPYING README THANKS ChangeLog    

Oops, sorry for overlooking this. Siddhesh, please add this
when importing this package into Fedora CVS.

Comment 34 Siddhesh Poyarekar 2010-02-18 06:07:46 UTC
Sure, I'll do that. Also rebasing with the latest upstream version.

Comment 35 Fedora Update System 2010-02-18 06:40:28 UTC
gource-0.24-1.fc12 has been submitted as an update for Fedora 12.
http://admin.fedoraproject.org/updates/gource-0.24-1.fc12

Comment 36 Fedora Update System 2010-02-18 06:40:33 UTC
gource-0.24-1.fc13 has been submitted as an update for Fedora 13.
http://admin.fedoraproject.org/updates/gource-0.24-1.fc13

Comment 37 Mamoru TASAKA 2010-02-18 06:52:04 UTC
Closing. Thank you, everyone.

Comment 38 Fedora Update System 2010-02-28 14:48:45 UTC
gource-0.24-1.fc13 has been pushed to the Fedora 13 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 39 Fedora Update System 2010-03-02 00:52:55 UTC
gource-0.24-1.fc12 has been pushed to the Fedora 12 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.