Bug 951817 (libreatlas) - Review Request: libreatlas - Digital atlas and geography education tool
Summary: Review Request: libreatlas - Digital atlas and geography education tool
Keywords:
Status: CLOSED ERRATA
Alias: libreatlas
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Dan Mashal
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2013-04-13 12:26 UTC by Micah Roth
Modified: 2013-05-03 02:41 UTC (History)
5 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2013-05-03 01:53:59 UTC
Type: ---
Embargoed:
dan.mashal: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Micah Roth 2013-04-13 12:26:03 UTC
Spec URL: multiseatlibrary.distract.org/files/libreatlas.spec
SRPM URL: multiseatlibrary.distract.org/files/libreatlas-1.0.0a-1.fc18.src.rpm
Description: LibreAtlas is an open source Geography Education application built on top of SpatiaLite and RasterLite. It uses LibreAtlas databases which are a digital alternative to a paper atlas.
Fedora Account System Username: ndroftheline

Comment 1 Micah Roth 2013-04-13 12:42:17 UTC
Spec URL: http://multiseatlibrary.distract.org/files/libreatlas.spec
SRPM URL: http://multiseatlibrary.distract.org/files/libreatlas-1.0.0a-1.fc18.src.rpm
Description: LibreAtlas is an open source Geography Education application built on top of SpatiaLite and RasterLite. It uses LibreAtlas databases which are a digital alternative to a paper atlas.
Fedora Account System Username: ndroftheline

EDIT: added http:// for the URLS so they are actually links (:

Comment 2 Micah Roth 2013-04-13 13:12:42 UTC
Spec URL: http://multiseatlibrary.distract.org/files/libreatlas.spec
SRPM URL: http://multiseatlibrary.distract.org/files/libreatlas-1.0.0a-1.fc18.src.rpm
Description: LibreAtlas is an open source Geography Education application built on top of SpatiaLite and RasterLite. It uses LibreAtlas databases which are a digital alternative to a paper atlas.
Fedora Account System Username: ndroftheline

EDIT: added http:// for the URLS so they are actually links (:

Comment 3 Dan Mashal 2013-04-15 00:39:13 UTC
Naming: OK
Licensing: OK (GPLv3+)

RPM installs and works.

Rawhide koji build:
https://koji.fedoraproject.org/koji/taskinfo?taskID=5250647

F18: koji build:
https://koji.fedoraproject.org/koji/taskinfo?taskID=5250654


$ rpmlint libreatlas-1.0.0a-1.fc20.x86_64.rpm 
libreatlas.x86_64: E: zero-length /usr/share/doc/libreatlas-1.0.0a/README
libreatlas.x86_64: W: no-manual-page-for-binary LibreAtlas
1 packages and 0 specfiles checked; 1 errors, 1 warnings.

$ rpmlint libreatlas-1.0.0a-1.fc20.src.rpm 
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint libreatlas.spec 
0 packages and 1 specfiles checked; 0 errors, 0 warnings.

Under the %install section please change "%make_install" to:

make install DESTDIR=%{buildroot}

Were you planning to package this for EPEL?

Comment 4 Micah Roth 2013-04-17 15:08:37 UTC
Spec URL: http://multiseatlibrary.distract.org/files/libreatlas.spec
SRPM URL: http://multiseatlibrary.distract.org/files/libreatlas-1.0.0a-2.fc18.src.rpm

I had not intended to package this for EPEL. 

I went with make install DESTDIR=$RPM_BUILD_ROOT for consistency in the spec file. 

rpmlint is now clean except for missing manpage. 

Thank you for your review!

Upstream replied to my bug reports and feature request with exciting news about the future of libreatlas, which will undergo a significant update soon along with the new version of spatialite and rasterlite. He is happy libreatlas is being considered for inclusion in Fedora.

Comment 5 Micah Roth 2013-04-17 15:11:24 UTC
Also, I am just curious and would like to learn: why is it better to use the 'make install' command instead of the %make_install macro? Is it always better to avoid the %make_install macro?

Comment 6 Dan Mashal 2013-04-17 22:12:50 UTC
(In reply to comment #5)
> Also, I am just curious and would like to learn: why is it better to use the
> 'make install' command instead of the %make_install macro? Is it always
> better to avoid the %make_install macro?

It is deprecated.

http://lists.fedoraproject.org/pipermail/packaging/2010-August/007317.html

Also so is $RPM_BUILD_ROOT vs %{buildroot}

Regardless I will give this package conditional approval after some furthr checks and getting you a sponsor.

APPROVED

Comment 7 Dan Mashal 2013-04-18 03:00:17 UTC
Micah, 

Before you get sponsored can you please review the following:
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group?rd=PackageMaintainers/HowToGetSponsored

We can discuss on IRC about you doing some informal reviews.

Comment 8 Micah Roth 2013-04-19 08:16:00 UTC
https://bugzilla.redhat.com/show_bug.cgi?id=952632
I started this informal review. I am looking for other "low hanging fruit" to look at, I am very happy to hear your suggestions, here or on IRC or email or whatever. 

wrt $RPM_BUILD_ROOT, (http://fedoraproject.org/wiki/Packaging:Guidelines#Using_.25.7Bbuildroot.7D_and_.25.7Boptflags.7D_vs_.24RPM_BUILD_ROOT_and_.24RPM_OPT_FLAGS) seems to indicate that is is exactly the same - should we ask the FPC to update the guidelines to indicate that $RPM_BUILD_ROOT is deprecated?

wrt $make_install, (http://fedoraproject.org/wiki/Packaging:Guidelines#Why_the_.25makeinstall_macro_should_not_be_used) indicates that $makeinstall should NOT be used but that $make_install is the exact same as "make DESTDIR=$RPM_BUILD_ROOT install". Again, should we ask the FPC to update the guidelines?

Thank you,

~Micah

Comment 9 Rex Dieter 2013-04-20 16:21:00 UTC
per comment #8, let me clarify some things:

$RPM_BUILD_ROOT is not deprecated.  The only requirement is that one be consistent and not mix $RPM_BUILD_ROOT and %buildroot , per the referenced guideline text :
"There is very little value in choosing one style over the other, since they will resolve to the same values in all scenarios. You should pick a style and use it consistently throughout your packaging."

%make_install is ok.  The aforementioned guideline explicitly says:
"... Instead, Fedora packages should use: %make_install (Note the "_" !), make DESTDIR=%{buildroot} install or make DESTDIR=$RPM_BUILD_ROOT install. Those all do the same thing."

Does that help? 


that said, I'll try to review this review :), and if all goes well, sponsor you.

Comment 10 Rex Dieter 2013-04-20 23:48:00 UTC
Since this is (one of) your first reviews, let me also offer some small comments and advice:


1. since Version contains a non-numeric component, consider looking over:

https://fedoraproject.org/wiki/Packaging:NamingGuidelines#NonNumericRelease

But, as long as care is taken that subsequent package versions are always considered newer (according to rpm), then all is well.

2.  Please document need/purpose of this in %prep:
sed -i '1d' gnome_resource/LibreAtlas.desktop


That said, I've now sponsored you.  Welcome to fedora!

Comment 11 Dan Mashal 2013-04-21 12:45:11 UTC
Thanks Rex!

Micah,

Set the fedora-cvs flag to ? and follow the instructions here:

https://fedoraproject.org/wiki/Package_SCM_admin_requests

If you would like me to be a co-maintainer add my FAS id (vicodan) after yours in the owners field.

You can leave the initialCC field blank.

Comment 12 Micah Roth 2013-04-21 13:47:28 UTC
> 1. since Version contains a non-numeric component, consider looking over:
> 
> https://fedoraproject.org/wiki/Packaging:NamingGuidelines#NonNumericRelease
> 
> But, as long as care is taken that subsequent package versions are always
> considered newer (according to rpm), then all is well.
> 
volter helped me understand RPM's version hierarchy logic and we checked with upstream on what their release progression is going to be, and I believe we have everything under control. 

> 2.  Please document need/purpose of this in %prep:
> sed -i '1d' gnome_resource/LibreAtlas.desktop
> 
Done.
Spec URL: http://multiseatlibrary.distract.org/files/libreatlas.spec
SRPM URL: http://multiseatlibrary.distract.org/files/libreatlas-1.0.0a-3.fc18.src.rpm

> 
> That said, I've now sponsored you.  Welcome to fedora!

Thank you Rex for your sponsorship. I am very happy to be part of the team. 

Dan: I must be missing the "owners" field. Is that on this bugzilla page?

Comment 14 Micah Roth 2013-04-21 14:30:16 UTC
Oh. Figured that out by looking at the link you gave me. Duh.

New Package SCM Request
=======================
Package Name: libreatlas
Short Description: Map and Geography Education tool
Owners: ndroftheline, vicodan
Branches: f17 f18 f19
InitialCC:

Comment 15 Gwyn Ciesla 2013-04-22 13:28:14 UTC
Git done (by process-git-requests).

Comment 16 Fedora Update System 2013-04-24 17:44:18 UTC
libreatlas-1.0.0a-3.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/libreatlas-1.0.0a-3.fc18

Comment 17 Fedora Update System 2013-04-24 17:45:49 UTC
libreatlas-1.0.0a-3.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/libreatlas-1.0.0a-3.fc19

Comment 18 Dan Mashal 2013-04-24 18:43:30 UTC
Please let bodhi close the review automatically.

Comment 19 Fedora Update System 2013-04-25 00:37:48 UTC
libreatlas-1.0.0a-3.fc18 has been pushed to the Fedora 18 testing repository.

Comment 20 Fedora Update System 2013-05-03 01:54:01 UTC
libreatlas-1.0.0a-3.fc18 has been pushed to the Fedora 18 stable repository.

Comment 21 Fedora Update System 2013-05-03 02:41:20 UTC
libreatlas-1.0.0a-3.fc19 has been pushed to the Fedora 19 stable repository.


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