Bug 507660

Summary: Review Request: xylib - Library for reading x-y data from several file formats
Product: [Fedora] Fedora Reporter: Marcin Wojdyr <wojdyr>
Component: Package ReviewAssignee: Mamoru TASAKA <mtasaka>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: low    
Version: rawhideCC: andrew, fedora-package-review, mtasaka, notting, susi.lehtola, wojdyr
Target Milestone: ---Flags: mtasaka: fedora-review+
kevin: fedora-cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: 0.4-4.fc11 Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2009-07-23 16:03:36 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 511758    

Description Marcin Wojdyr 2009-06-23 16:53:03 UTC
Spec URL: http://www.unipress.waw.pl/~wojdyr/spec/xylib.spec
SRPM URL: http://www.unipress.waw.pl/~wojdyr/spec/xylib-0.4-1.fc11.src.rpm
Description: 
C++ library for reading files that contain x-y data from powder diffraction,
spectroscopy or other experimental methods. The supported formats include:
VAMAS, pdCIF, Bruker UXD and RAW, Philips UDF and RD, Rigaku DAT,
Sietronics CPI, DBWS/DMPLOT, Koalariet XDD and others.

The latest version of fityk (older version of fityk is in extras) depends on this lib.

Seeking a sponsor.

Comment 1 Andrew Colin Kissa 2009-06-23 19:24:26 UTC
Hi Marcin,

I am not able to sponsor your package but I can hopefully offer some useful
help.

* BuildRequires: gcc-c++

Is does not have to be declared - http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2

* Timestamps

You should consider using use of INSTALL="install -p" when you make install

Comment 2 Marcin Wojdyr 2009-06-23 19:48:49 UTC
Hi Andrew, 
thanks for your comments.
I added gcc-c++ to build the same RPM on OpenSuse Build Service. I know that it's not necessary (on Fedora), although it's not clear to me if having it is allowed or not.

When I prepared this RPM I looked at a few other RPMS in Fedora. Some of them have "install -p" and some don't. I have no idea when it should be used, but I'll add it to the SPEC.

Comment 3 Susi Lehtola 2009-06-26 10:07:14 UTC
- Don't call libtool manually. Remove the last three lines in %install, add instead BuildRequires: automake and 
 sed -i "s|noinst_|bin_|g" Makefile.am
 automake
to %setup, this will make the build process install xyconv to %{_bindir}.

- Use SMP make flags. Change 
 make
to
 make %{?_smp_mflags}

- Change
 %{_bindir}/*
to
 %{_bindir}/xyconv
and
 %{_includedir}/*
to
 %{_includedir}/xylib/
(no sense in using wildcards in these cases)

- The attributes
 %defattr(-,root,root)
should be
 %defattr(-,root,root,-)


SHOULD: Use a more recommended version of the BuildRoot tag
http://fedoraproject.org/wiki/Packaging/Guidelines#BuildRoot_tag

Comment 4 Marcin Wojdyr 2009-06-26 18:05:47 UTC
I made all the changes you suggested above, with one exception: I kept gcc-g++ in BuildRequires, I hope it's not a problem.

Thank you both.

Spec URL: http://www.unipress.waw.pl/~wojdyr/spec/xylib.spec
SRPM URL: http://www.unipress.waw.pl/~wojdyr/spec/xylib-0.4-1.fc11.src.rpm

Comment 5 Susi Lehtola 2009-06-26 18:19:10 UTC
As you're a new packager you should learn to run rpmlint on your packages: https://fedoraproject.org/wiki/Packaging/Guidelines#Use_rpmlint

You've written %defattr in the changelog, which may be expanded. rpmlint complains from this among other things. In the changelog you should use %% to prevent the macro from being expanded.

rpmlint does sometimes generate noise, for instance it will comlain about the -devel package having no documentation.

Comment 6 Marcin Wojdyr 2009-06-26 18:41:48 UTC
Yes, I know about rpmlint, but the last time I checked only RPMs, not the spec.
Updated spec and SRPM are in the same place.

Comment 7 Mamoru TASAKA 2009-07-03 17:54:41 UTC
Your srpm does not build on dist-f12:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1453152

By the way please make it sure that you change the release number
of your spec file every time you modify your spec file
(and post the URLs of new spec/srpm files) to avoid confusion.

Comment 8 Marcin Wojdyr 2009-07-03 19:06:44 UTC
Thanks for looking at this rpm.

The problem is with version mismatch of automake. Should I call libtool manually, like in the first version, i.e.
./libtool --mode=install install -m 755 xyconv $RPM_BUILD_ROOT%{_bindir}

or rather run more autotools after changing Makefile.am?

Comment 9 Susi Lehtola 2009-07-03 19:31:34 UTC
(In reply to comment #8)
> Thanks for looking at this rpm.
> 
> The problem is with version mismatch of automake. Should I call libtool
> manually, like in the first version, i.e.
> ./libtool --mode=install install -m 755 xyconv $RPM_BUILD_ROOT%{_bindir}
> 
> or rather run more autotools after changing Makefile.am?  

The first option is of course to get upstream to change the makefile to install the binary by default.

The second one is to run more autotools.

(The third one is requiring the necessary version of automake to build the package. Automake 1.10 doesn't seem to be available on F12, though...)

Funny, I tried the build with F11 which has the same version of automake as F12, so there shouldn't be any problem with that...

Comment 10 Marcin Wojdyr 2009-07-03 20:04:01 UTC
Jussi,
I'm just wondering why changing Makefile.am is preferred to calling 
./libtool --mode=install install ...
The latter seems more portable to me.
I couldn't find anything about it in Fedora guides.
Marcin

Comment 11 Susi Lehtola 2009-07-03 20:09:39 UTC
It's because it isn't supposed to be used that way. It defeats the whole purpose of autotools: a simple(ish) way of configuring and building software on many platforms.

Comment 12 Marcin Wojdyr 2009-07-04 14:44:28 UTC
(In reply to comment #11)
> It's because it isn't supposed to be used that way. It defeats the whole
> purpose of autotools: a simple(ish) way of configuring and building software on
> many platforms.  

Sorry, but do you really mean that using well-documented, portable libtool script "defeats the whole purpose of autotools"? It's much more elegant way than hacking Makefile.am and running autotools.

If for some reasons using libtool is not recommended in fedora project (but I haven't seen such a statement in docs and there is a number of specs in fedora CVS that _are_ using libtool in this way), the second most portable way is to just call install.

Marcin

Comment 13 Marcin Wojdyr 2009-07-06 18:58:05 UTC
I changed the spec to avoid using automake.

Spec URL: http://www.unipress.waw.pl/~wojdyr/spec/xylib.spec
SRPM URL: http://www.unipress.waw.pl/~wojdyr/spec/xylib-0.4-3.fc11.src.rpm

Comment 14 Susi Lehtola 2009-07-06 21:07:16 UTC
btw on fedora-devel there has been a discussion raging on whether autotools files are OK to patch and rerun autotools vs hacking the preprocessed files manually, which is analogous to this matter.

Comment 15 Mamoru TASAKA 2009-07-09 17:13:34 UTC
For -3:

* Requires
  - -devel subpackage should have "Requires: boost-devel" because
    xylib/cache.h contains:
----------------------------------------------------
    23  #if 0
    24  #include <tr1/memory>
    25  using std::tr1::shared_ptr;
    26  #else
    27  #include <boost/shared_ptr.hpp>
    28  using boost::shared_ptr;
    29  #endif
----------------------------------------------------

Then:

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

This package will be accepted with another few 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 16 Marcin Wojdyr 2009-07-15 11:08:17 UTC
I added the missing Require:

Spec URL: http://www.unipress.waw.pl/~wojdyr/spec/xylib.spec
SRPM URL: http://www.unipress.waw.pl/~wojdyr/spec/xylib-0.4-4.fc11.src.rpm

and will try to make another work soon.

Comment 17 Mamoru TASAKA 2009-07-15 16:41:04 UTC
Okay. Then I will wait for your another review request or
your pre-review of other person's review request.

Comment 18 Marcin Wojdyr 2009-07-15 20:07:54 UTC
I didn't do exactly what you asked yet, but today FTBFS bug was submitted for fityk package, which I'm an upstream author. I made several changes to the fityk spec and submitted a patch as an attachment 353890 [details] to bug 511758.

Comment 19 Mamoru TASAKA 2009-07-16 13:21:23 UTC
(In reply to comment #18)
> I didn't do exactly what you asked yet, but today FTBFS bug was submitted for
> fityk package, which I'm an upstream author. I made several changes to the
> fityk spec and submitted a patch as an attachment 353890 [details] to bug 511758.  

Ah, okay.

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

Please follow the procedure written on:
http://fedoraproject.org/wiki/PackageMaintainers/Join
from "Get a Fedora Account".
After you request for sponsorship a mail will be sent to sponsor 
members automatically (which is invisible for you) which notifies 
that you need a sponsor. After that, please also write on
this bug for confirmation that you requested for sponsorship and
your FAS (Fedora Account System) name. Then I will sponsor you.

If you want to import this package into Fedora 10/11, 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.

Comment 20 Marcin Wojdyr 2009-07-17 22:39:09 UTC
Thanks. I requested sponsorship. FAS name: wojdyr

Comment 21 Mamoru TASAKA 2009-07-18 14:24:17 UTC
Okay, now I am sponsoring you. Please follow "Join" wiki again.

Comment 22 Marcin Wojdyr 2009-07-18 18:20:56 UTC
New Package CVS Request
=======================
Package Name: xylib
Short Description: Library for reading x-y data from several file formats
Owners: wojdyr
Branches: F-10 F-11
InitialCC:

Comment 23 Kevin Fenzi 2009-07-19 20:56:18 UTC
cvs done.

Comment 24 Fedora Update System 2009-07-20 17:31:47 UTC
xylib-0.4-4.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/xylib-0.4-4.fc11

Comment 25 Fedora Update System 2009-07-20 17:31:53 UTC
xylib-0.4-4.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/xylib-0.4-4.fc10

Comment 26 Fedora Update System 2009-07-22 22:02:32 UTC
xylib-0.4-4.fc11 has been pushed to the Fedora 11 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update xylib'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F11/FEDORA-2009-7894

Comment 27 Fedora Update System 2009-07-22 22:04:22 UTC
xylib-0.4-4.fc10 has been pushed to the Fedora 10 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update xylib'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F10/FEDORA-2009-7903

Comment 28 Mamoru TASAKA 2009-07-23 16:03:36 UTC
Now closing.

Comment 29 Fedora Update System 2009-08-08 19:23:02 UTC
xylib-0.4-4.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 30 Fedora Update System 2009-08-08 19:31:12 UTC
xylib-0.4-4.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.