Bug 1184040 - Review Request: blivet-gui - Tool for data storage configuration
Summary: Review Request: blivet-gui - Tool for data storage configuration
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jaroslav Škarvada
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-01-20 14:09 UTC by Vojtech Trefny
Modified: 2015-01-29 12:44 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2015-01-29 12:44:44 UTC
Type: ---
Embargoed:
jskarvad: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Vojtech Trefny 2015-01-20 14:09:45 UTC
Spec URL: https://github.com/rhinstaller/blivet-gui/blob/master/blivet-gui.spec
SRPM URL: https://vtrefny.fedorapeople.org/srpms/blivet-gui-0.2.0-3.fc21.src.rpm
Description: Graphical (GTK) tool for manipulation and configuration of data storage (disks, LVMs, RAIDs) based on blivet library.
Fedora Account System Username: vtrefny

This is my first package for Fedora and I'm looking for a sponsor.

Additional info:
 - upstream: https://github.com/rhinstaller/blivet-gui
 - copr repository: http://copr.fedoraproject.org/coprs/vtrefny/blivet-gui/
 - koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=8672111

Comment 1 Vratislav Podzimek 2015-01-21 09:14:17 UTC
I'll do the review.

Comment 2 Jaroslav Škarvada 2015-01-21 12:47:11 UTC
I will look on it (agreed with Vratislav :)

Comment 3 Jaroslav Škarvada 2015-01-21 15:10:30 UTC
I will sponsor you after we finish the review.

Issues found:

- SPEC file provided in comment 0 is different than SPEC file in SRPM, diff:
@@ -33,5 +33,4 @@
 
 %install
-rm -rf %{buildroot}
 make DESTDIR=%{buildroot} install
 
@@ -41,4 +40,7 @@
 %find_lang %{name}
 
+%clean
+rm -rf %{buildroot}
+
 %files -f %{name}.lang
 %{_mandir}/man1/blivet-gui.1*

- also please link the SPEC file directly (raw), not HTML.

- checksums of archive included in SRPM and upstream archive are different:
6e5dd8581fb8b73c17bd0981cd367a18  blivet-gui-0.2.0.tar.gz
9ded8f2490c7561adbfe170bc4736262  blivet-gui-blivet-gui-0.2.0.tar.gz

Also the provided source0 URL serves blivet-gui-blivet-gui-0.2.0.tar.gz, not blivet-gui-0.2.0.tar.gz. Fix the source0 URL or add workaround to SPEC, e.g.:
https://fedoraproject.org/wiki/Packaging:SourceURL?rd=Packaging/SourceURL#Troublesome_URLs

- find_lang macro should be in %install section, not %check

- desktop_file_validate should be also in %install section, please use the %check section only if there is working %check target in your makefile with useful built-time tests

- in files section use macros, e.g. %{_datadir} for /usr/share, %{_bindir} for /usr/bin

- you don't need to run 'rm -rf %{buildroot}' in %install section

- your package must not own /usr/share/applications and /usr/share/polkit-1/actions directories, only own individual files/subdirs


Minor, mostly cosmetic issues:

- BuildRequires:intltool
  Please add space after colon to be consistent

- Url: http://github.com/rhinstaller/blivet-gui
  Please rather use URL: (i.e. in capitals).


Other:

- Is the source code python 3 ready? In such case you should package it conditionally for python 3 (http://fedoraproject.org/wiki/Packaging:Python). If not, it's OK as is.

- Source package should contains license text, as upstream please add it.

- Directories /usr/share/help and /usr/share/help/C are not owned, but this is something you cannot resolve. AFAIK /usr/share/help is not in FHS standard. This should be provided by some package. Question is which package, gtk? gtk-doc? Any opinion? Bug should be filled against the identified package to include these directories.

Comment 4 Vojtech Trefny 2015-01-22 10:27:36 UTC
(In reply to Jaroslav Škarvada from comment #3)
> I will sponsor you after we finish the review.

New spec/srpm:

https://vtrefny.fedorapeople.org/spec/blivet-gui.spec
https://vtrefny.fedorapeople.org/srpms/blivet-gui-0.2.0-5.fc21.src.rpm

> 
> Issues found:
> 
> - SPEC file provided in comment 0 is different than SPEC file in SRPM, diff:
> @@ -33,5 +33,4 @@
>  
>  %install
> -rm -rf %{buildroot}
>  make DESTDIR=%{buildroot} install
>  
> @@ -41,4 +40,7 @@
>  %find_lang %{name}
>  
> +%clean
> +rm -rf %{buildroot}
> +
>  %files -f %{name}.lang
>  %{_mandir}/man1/blivet-gui.1*
> 
> - also please link the SPEC file directly (raw), not HTML.
> 
> - checksums of archive included in SRPM and upstream archive are different:
> 6e5dd8581fb8b73c17bd0981cd367a18  blivet-gui-0.2.0.tar.gz
> 9ded8f2490c7561adbfe170bc4736262  blivet-gui-blivet-gui-0.2.0.tar.gz

The problem here is I don't have localisation files in git repository -- MO files are in SPRM because I download them from Zanata during "make archive" (and upstream archives are made automatically by GitHub for every tag).

> 
> Also the provided source0 URL serves blivet-gui-blivet-gui-0.2.0.tar.gz, not
> blivet-gui-0.2.0.tar.gz. Fix the source0 URL or add workaround to SPEC, e.g.:
> https://fedoraproject.org/wiki/Packaging:SourceURL?rd=Packaging/
> SourceURL#Troublesome_URLs

Fixed.

> 
> - find_lang macro should be in %install section, not %check
> 
> - desktop_file_validate should be also in %install section, please use the
> %check section only if there is working %check target in your makefile with
> useful built-time tests
> 
> - in files section use macros, e.g. %{_datadir} for /usr/share, %{_bindir}
> for /usr/bin
> 
> - you don't need to run 'rm -rf %{buildroot}' in %install section
> 
> - your package must not own /usr/share/applications and
> /usr/share/polkit-1/actions directories, only own individual files/subdirs
>

Fixed.

> 
> Minor, mostly cosmetic issues:
> 
> - BuildRequires:intltool
>   Please add space after colon to be consistent
> 
> - Url: http://github.com/rhinstaller/blivet-gui
>   Please rather use URL: (i.e. in capitals).

Fixed.

> 
> 
> Other:
> 
> - Is the source code python 3 ready? In such case you should package it
> conditionally for python 3 (http://fedoraproject.org/wiki/Packaging:Python).
> If not, it's OK as is.

Not it isn't because python-blivet is not python3 read yet.

> 
> - Source package should contains license text, as upstream please add it.
> 
> - Directories /usr/share/help and /usr/share/help/C are not owned, but this
> is something you cannot resolve. AFAIK /usr/share/help is not in FHS
> standard. This should be provided by some package. Question is which
> package, gtk? gtk-doc? Any opinion? Bug should be filled against the
> identified package to include these directories.

Comment 5 Jaroslav Škarvada 2015-01-22 11:20:21 UTC
(In reply to Vojtech Trefny from comment #4)

> > 6e5dd8581fb8b73c17bd0981cd367a18  blivet-gui-0.2.0.tar.gz
> > 9ded8f2490c7561adbfe170bc4736262  blivet-gui-blivet-gui-0.2.0.tar.gz
> 
> The problem here is I don't have localisation files in git repository -- MO
> files are in SPRM because I download them from Zanata during "make archive"
> (and upstream archives are made automatically by GitHub for every tag).
> 
It needs to match, could you host the resulting release anywhere? E.g. at fedorahosted.org, or use github release feature:

https://github.com/blog/1547-release-your-software

Comment 7 Jaroslav Škarvada 2015-01-22 14:56:36 UTC
(In reply to Jaroslav Škarvada from comment #3)
> - Directories /usr/share/help and /usr/share/help/C are not owned, but this
> is something you cannot resolve. AFAIK /usr/share/help is not in FHS
> standard. This should be provided by some package. Question is which
> package, gtk? gtk-doc? Any opinion? Bug should be filled against the
> identified package to include these directories.

FYI initially filed bug against yelp (bug 1184954), let's see what happen :) At least it's tracked now.

Comment 8 Jaroslav Škarvada 2015-01-22 15:15:56 UTC
(In reply to Vojtech Trefny from comment #6)
> Fixed source (using GitHub releases):
> 
> https://vtrefny.fedorapeople.org/srpms/blivet-gui-0.2.0-6.fc21.src.rpm
> https://vtrefny.fedorapeople.org/spec/blivet-gui.spec

Thanks, one note:

- please do not use Release tag in upstream release, just name the upstream release: %{name}-%{version}.tar.gz, i.e. blivet-gui-0.2.0.tar.gz, the Release tag is Fedora specific, it's there to distinguish updates related to packaging, rebuilds, etc. 

This is usptream thing and not blocking the review, thus approving.

You are now sponsored, welcome aboard :)

Please continue from step 7 in: http://fedoraproject.org/wiki/New_package_process_for_existing_contributors

Comment 9 Vojtech Trefny 2015-01-26 12:25:52 UTC
New Package SCM Request
=======================
Package Name: blivet-gui
Short Description: GUI tool for data storage configuration
Upstream URL: https://github.com/rhinstaller/blivet-gui
Owners: vtrefny
Branches: f22 f21
InitialCC:

Comment 10 Gwyn Ciesla 2015-01-26 14:05:18 UTC
Git done (by process-git-requests).

Comment 11 Vojtech Trefny 2015-01-29 12:44:44 UTC
fedpkg build successfully finished, closing this bug. Jaroslav, thanks for review and sponsoring!


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