Bug 600467 - Review Request: jags - Just Another Gibbs Sampler
Summary: Review Request: jags - Just Another Gibbs Sampler
Keywords:
Status: CLOSED NOTABUG
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Martin Gieseking
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 600529
TreeView+ depends on / blocked
 
Reported: 2010-06-04 19:07 UTC by chris desjardins
Modified: 2013-10-19 14:42 UTC (History)
3 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2011-02-14 15:28:00 UTC
Type: ---
Embargoed:
martin.gieseking: fedora-review?


Attachments (Terms of Use)

Description chris desjardins 2010-06-04 19:07:14 UTC
Spec URL: http://dl.dropbox.com/u/1501309/jags.spec
SRPM URL: http://dl.dropbox.com/u/1501309/jags-2.1.0-2.1.src.rpm
Description: 
JAGS (Just Another Gibbs Sampler) is a program for the analysis of
Bayesian statistical models using Markov Chain Monte Carlo (MCMC)
simulation.

JAGS is similar to the BUGS (Bayesian Inference using Gibbs Sampling)
program developed at the MRC Biostatistics Unit, Cambridge, and uses a
dialect of the BUGS language to describe Bayesian hierarchical models.

Comment 1 chris desjardins 2010-06-04 19:08:21 UTC
Also, I've sent my rpmlint warnings to the developer upstream re: the absence of a manpage and exit().

Comment 2 chris desjardins 2010-06-04 22:59:57 UTC
Also this is my first package so I need a sponsor. I've also submitted 2 other packages allowing a user to interact from R with JAGS. These review requests are 

https://bugzilla.redhat.com/show_bug.cgi?id=600517
https://bugzilla.redhat.com/show_bug.cgi?id=600529

Comment 3 chris desjardins 2010-06-07 18:47:27 UTC
I've updated the SRPM and the spec file. 
Spec URL: http://dl.dropbox.com/u/1501309/jags.spec
Source URL: http://dl.dropbox.com/u/1501309/Fedora/jags-2.1.0-2.fc13.src.rpm

This addresses the issues with the man page and an explanation of the exit() warning from the upstream author (see the Changelog). The man page will be included in newer releases.

Additionally, the upstream author believes that the final outstanding rpmlint warning associated with hidden files has to do with libtool and not JAGS.

Comment 4 Steve Traylen 2010-06-08 18:18:24 UTC
This is an informal review, as I understand from a mail you are
seeking sponsership and should block this bug on FE-NEEDSPONSER accordingly.

Just looking at the .spec file for now 

1) The .la should be removed somehow.

See:
http://fedoraproject.org/wiki/PackagingGuidelines#Packaging_Static_Libraries

2) Your release tag is 2.1 which is odd, it typically starts at 1. :-)

See: http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Package_Version
In a nutshell it's the number of .spec file modifications or builds in fact
since the Version was last update.

3) Not important I would say but in principal packages take the name of 
the tar ball so JAGS but I would never not approve based on that.
http://fedoraproject.org/wiki/Packaging:NamingGuidelines

4) BuildRequires: gcc-c++ certainly is not needed, see
http://fedoraproject.org/wiki/PackagingGuidelines#Exceptions_2

5) I see you have a .tex file in your docs have you considered converting
   it to something more readable such as ps or pdf.

 Steve.

Comment 5 chris desjardins 2010-06-08 21:09:33 UTC
Hi Steve,
Thanks for for taking a quick look at this. Regarding:

1) I am really not sure how to deal with this one. I tried to configure it w/ --disable-static but those two *.la files still showed up. The two *.la files:

libjags.la
libjrmath.la

are JAGS specific I believe and looking here:
http://fedoraproject.org/wiki/PackagingGuidelines#Packaging_Static_Libraries

I can't determine exactly what I should do. Create a *-static package, contact upstream or something else. Help with this would be very useful!

2) I know. I didn't realize that until after I uploaded it. This should be fixed now.

3) and 4)  Fixed in JAGS-2.1.0-3.fc13.src.rpm[1]

5) I haven't really given this much consideration. It would be a good idea ... but what format? http://fedoraproject.org/wiki/Packaging:Guidelines doesn't seem to provide any real guidance.

[1] Updated Spec and Source URL
Spec URL: http://dl.dropbox.com/u/1501309/Fedora/JAGS.spec
Source URL: http://dl.dropbox.com/u/1501309/Fedora/JAGS-2.1.0-3.fc13.src.rpm

Comment 6 Steve Traylen 2010-06-08 21:47:25 UTC
(In reply to comment #5)
> Hi Steve,
> Thanks for for taking a quick look at this. Regarding:
> 
> 1) I am really not sure how to deal with this one. I tried to configure it w/
> --disable-static but those two *.la files still showed up. The two *.la files:
> 
> libjags.la
> libjrmath.la
> 
> are JAGS specific I believe and looking here:
> http://fedoraproject.org/wiki/PackagingGuidelines#Packaging_Static_Libraries
> 
> I can't determine exactly what I should do. Create a *-static package, contact
> upstream or something else. Help with this would be very useful!

If --disable-static does not work, just delete them
in the %build section after your build. i.e

rm %{_builddir}%{_libdir}/*.la

> 
> 2) I know. I didn't realize that until after I uploaded it. This should be
> fixed now.
> 
> 3) and 4)  Fixed in JAGS-2.1.0-3.fc13.src.rpm[1]
> 
> 5) I haven't really given this much consideration. It would be a good idea ...
> but what format? http://fedoraproject.org/wiki/Packaging:Guidelines doesn't
> seem to provide any real guidance.

Your choice even if it to do it or not , I would probably convert to
pdf , of course you will need to pull in pdflatex as build dependencies
if you choose to do this.
> 
> [1] Updated Spec and Source URL
> Spec URL: http://dl.dropbox.com/u/1501309/Fedora/JAGS.spec
> Source URL: http://dl.dropbox.com/u/1501309/Fedora/JAGS-2.1.0-3.fc13.src.rpm    

Will look after the next iteration.

Comment 7 chris desjardins 2010-06-08 23:11:30 UTC
Updated.
SPEC URL: http://dl.dropbox.com/u/1501309/Fedora/JAGS.spec
Source URL: http://dl.dropbox.com/u/1501309/Fedora/JAGS-2.1.0-4.fc13.src.rpm

However, I still wasn't able to get:

rm %{_builddir}%{_libdir}/*.la


to work no matter where I put it. Also are all files that end in .la static? Are they all problematic? If so there are several files in JAGS that have .la in them.

[root@goddard x86_64]# rpm -ql JAGS | grep la
/usr/lib64/JAGS/modules-2.1.0/basemod.la
/usr/lib64/JAGS/modules-2.1.0/bugs.la
/usr/lib64/JAGS/modules-2.1.0/dic.la
/usr/lib64/JAGS/modules-2.1.0/glm.la
/usr/lib64/JAGS/modules-2.1.0/lecuyer.la
/usr/lib64/JAGS/modules-2.1.0/mix.la
/usr/lib64/JAGS/modules-2.1.0/msm.la
/usr/lib64/libjags.la
/usr/lib64/libjrmath.la

To be honest, I am a little stumped and unsure about how to deal with this. I looked at the Debian package and it had these .la files in there too. Any further help you can give me is greatly appreciated!

Comment 8 chris desjardins 2010-06-08 23:14:40 UTC
Oh the difference between -3 and -4 is that I now build the .tex files into PDF and some other subtle differences.

Comment 9 Martin Gieseking 2010-06-23 10:13:52 UTC
Hi Chris,

.la files must not be packaged in Fedora according to the packaging guidelines. I suggest to remove them in the %install section after "make install", e.g. with
find %{buildroot}%{_libdir} -name "*.la" -exec rm -f {} \;

Also, drop them in the %files section.

Here are some further comments:

- in the devel package, use a fully versioned dependency:
  Requires: %{name} = %{version}-%{release} 
  (rather than Requires: JAGS  = %{version})

- According to the source file headers, the license is GPLv2+ because 
  of the addition "or (at your option) any later version".

- add file COPYING to the base package (as %doc)

- the Group of the devel package should be 'Development/Libraries'

- as a minor cosmetic addition, I suggest to append a slash to directory 
  names, e.g. %{_includedir}/JAGS/
  This way it's easier to recognize that a folder is added rather than a 
  single file

Comment 10 Martin Gieseking 2010-06-23 10:28:11 UTC
Some more comments:
- %setup -q should be sufficient in %prep
- add %{_smp_mflags} to the first 'make'
- replace %defattr(-,root,root) with %defattr(-,root,root,-)
- add AUTHORS, ChangeLog, NEWS and TODO as %doc to the base package

Comment 11 chris desjardins 2010-06-23 13:58:20 UTC
Hi Martin,
Thank you very much for reviewing this package! I am really excited about hopefully getting this package into Fedora. 

I've updated my *.spec and *.src.rpm files to reflect all the changes that you've mentioned above. Please let me know what else I can do. 

Spec URL: http://dl.dropbox.com/u/1501309/Fedora/JAGS/JAGS.spec
Source URL: http://dl.dropbox.com/u/1501309/Fedora/JAGS/JAGS-2.1.0-5.fc13.src.rpm

Comment 12 Martin Gieseking 2010-06-23 16:04:49 UTC
(In reply to comment #11)
> Thank you very much for reviewing this package! I am really excited about
> hopefully getting this package into Fedora. 

Hi Chris,

I'm not a sponsor, so I can't formally review this package. Nonetheless, polishing the spec file will certainly help to find one. In addition, you usually need to offer some more packages and comment on other packager's review requests to show that you're familiar with the packaging guidelines.

Here are a couple of further remarks on your package:

- In Source0, replace JAGS-2.1.0.tar.gz with JAGS-%{version}.tar.gz or
  %{name}-%{version}.tar.gz. It simplifies future updates of the package

- I think, you can also add %{_smp_mflags} to "make docs". It enables 
  parallel builds if possible, and thus speeds up the build process on 
  multi-processor systems.

- remove the trailing slash from %{_bindir}/jags/ because jags is not
  a directory here but a binary file

- you can drop jags_installation_manual.pdf as it's not of much use in 
  a binary package

- also adapt the %defattr parameters of the devel package as mentioned 
  in comment #10

- The tarball contains a copy of libltdl which should be removed in favor 
  of the package provided by Fedora (libtool-ltdl-devel).

Comment 13 chris desjardins 2010-06-23 17:53:03 UTC
Thanks. I will update the package once I get the libltdl situation figured out. AI am not entirely sure that removing the *.la files is correct and I have contacted the upstream author to verify this doesn't cripple the program. (It doesn't appear to). 

Also, I wanted to mention I am seeking sponsorship for these packages as well:

https://bugzilla.redhat.com/show_bug.cgi?id=600529
https://bugzilla.redhat.com/show_bug.cgi?id=600517

I would like to maintain several R packages in Fedora beyond these (including R-lme4 and R-MCMCglmm) but wanted to focus on these 3 first. Should I offer more packages too?

Comment 14 Martin Gieseking 2010-06-23 18:27:48 UTC
(In reply to comment #13)
> AI am not entirely sure that removing the *.la files is correct and I have
> contacted the upstream author to verify this doesn't cripple the program. (It
> doesn't appear to). 

Removing the libtool archives isn't harmful at all. They are only used when linking a library with libtool. Alternatively, you could also explicitly link against the corresponding .a or .so files. Once the binary is built, the .la files are not required any longer, especially since Fedora primarily distributes shared libs which are linked via .so files. And the latter are part of the devel package. 


> I would like to maintain several R packages in Fedora beyond these (including
> R-lme4 and R-MCMCglmm) but wanted to focus on these 3 first. Should I offer
> more packages too?    

OK, that's fine. As far as I know, three packages should be enough, but this decision is up to your sponsor.

Comment 15 chris desjardins 2010-06-23 18:38:25 UTC
Hi Martin,
Thanks for your speedy reply. Do you know how I would do this?

> - The tarball contains a copy of libltdl which should be removed in favor 
>  of the package provided by Fedora (libtool-ltdl-devel).  

In the .spec file? I am unclear how to do this. Thanks!

Comment 16 Martin Gieseking 2010-06-23 19:11:20 UTC
I haven't had a deeper look into the tarball and the autotools scripts, but you can probably do it this way:

- add BuildRequires: libtool-ltdl-devel autoconf automake

- in %prep: 
  * remove the libltdl directory
  * remove all references to this directory from configure.ac and Makefile.am 
    by patching these files accordingly 
   (don't modify the files in the tarball but add .patch/.diff files
    describing the changes to your package)
  * run 'autoreconf'

This is just the general idea. I haven't tried it, so there might be further things to be addressed.

Comment 17 chris desjardins 2010-06-23 21:02:35 UTC
I have addressed all of the outstanding comments (#12 and #16) in the latest version.

SPEC URL: http://dl.dropbox.com/u/1501309/Fedora/JAGS/JAGS.spec
Source URL: http://dl.dropbox.com/u/1501309/Fedora/JAGS/JAGS-2.1.0-6.fc13.src.rpm

I have patched configure.ac and Makefile.am as per Martin's requests. Also, I removed ChangeLog as it is empty. The binary installs fine and works.

Comment 18 chris desjardins 2010-06-24 15:36:51 UTC
Updated:

SPEC URL: http://dl.dropbox.com/u/1501309/Fedora/JAGS/JAGS.spec
Source URL: http://dl.dropbox.com/u/1501309/Fedora/JAGS/JAGS-2.1.0-7.fc13.src.rpm

The change from -6 to -7 is that I removed the patches for ltdl. The upstream author informed that while JAGS ships with its own copy of ltdl if it detects a system copy then it will automatically use that and not the version in the sources. He said that I need to only add libtoool-ltdl-devel in the BuildRequires. Therefore, I believed this rendered my patches unnecessary. This is reflected in the new .src.rpm and .spec file. Also he thinks it's probably fine to remove those two .la files.

Comment 19 Martin Gieseking 2010-06-24 17:26:39 UTC
(In reply to comment #18)
> He said that I need to only add libtoool-ltdl-devel in the
> BuildRequires. Therefore, I believed this rendered my patches unnecessary.

Hi Chris,

you should not rely on this. Since using bundled libraries is not permitted in Fedora, you have to ensure that these are not linked. You usually do this by removing the bundled libs. Thus, I suggest to revert to release 6.

Comment 20 chris desjardins 2010-06-24 19:11:12 UTC
OK reverting to release 6.

SPEC URL: http://dl.dropbox.com/u/1501309/Fedora/JAGS/JAGS.spec
Source URL: http://dl.dropbox.com/u/1501309/Fedora/JAGS/JAGS-2.1.0-6.fc13.src.rpm

Comment 21 chris desjardins 2011-02-14 15:28:00 UTC
Sorry I don't have time to do this anymore. Graduate school!!! So I have to close this bug.


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