Bug 1200384 - Review Request: ocaml-config-file - Configuration file management for OCaml
Summary: Review Request: ocaml-config-file - Configuration file management for OCaml
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Tomas Heinrich
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-03-10 13:07 UTC by Nikos Mavrogiannopoulos
Modified: 2016-09-20 04:52 UTC (History)
4 users (show)

Fixed In Version: ocaml-config-file-1.2-4.fc21
Clone Of:
Environment:
Last Closed: 2015-03-13 14:25:49 UTC
Type: ---
Embargoed:
theinric: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Nikos Mavrogiannopoulos 2015-03-10 13:07:10 UTC
Spec URL: http://people.redhat.com/nmavrogi/fedora/ocaml-config-file.spec
SRPM URL: http://people.redhat.com/nmavrogi/fedora/ocaml-config-file-1.2-1.fc21.src.rpm
Description: Config_file is an OCaml library used to manage the configuration file(s) of an application. You simply define your options and it performs the
loading and saving of the options. Each option is defined from an option
class (for example an "int" option) or from a combination of classes
(for example to create "list of int" options).
Fedora Account System Username: nmav

Comment 1 Richard W.M. Jones 2015-03-10 17:08:08 UTC
There seems to be a certain amount of copy and paste in the spec
file which is not necessary as far as I can see.

These entire sections seem to be unnecessary and should be deleted:

# Prevent RPM from stripping the binaries & debuginfo.
#
# NB: Only required because this package uses the obsolete -custom
# parameter and builds a bytecode 'ocamlrpcgen'.  I tried to fix the
# build to make a native code 'ocamlrpcgen' but the build system got
# the better of me.
%global debug_package %{nil}
%global __strip /bin/true

#%global __ocaml_requires_opts -i Asttypes -i Outcometree -i Parsetree
%global __ocaml_requires_opts -c -f '%{buildroot}%{_bindir}/ocamlrun %{buildroot}%{_bindir}/ocamlobjinfo'
%global __ocaml_provides_opts -f '%{buildroot}%{_bindir}/ocamlrun %{buildroot}%{_bindir}/ocamlobjinfo'

I also think the commented-out lines in the %files sections should
be deleted if they are not needed.

Comment 3 Tomas Heinrich 2015-03-12 15:05:01 UTC
rpmlint:
(rpmlint-1.6-2.fc22.noarch)

> rpms/x86_64/ocaml-config-file-1.2-2.fc22.x86_64.rpm
> ocaml-config-file.x86_64: E: no-binary
> ocaml-config-file.x86_64: W: only-non-binary-in-usr-lib
> 1 packages and 0 specfiles checked; 1 errors, 1 warnings.

Should this package actually be noarch? Can you provide a rationale why, either way?

> rpms/x86_64/ocaml-config-file-debuginfo-1.2-2.fc22.x86_64.rpm
> ocaml-config-file-debuginfo.x86_64: E: debuginfo-without-sources
> 1 packages and 0 specfiles checked; 1 errors, 0 warnings.

Can't really say why this one is happening.

> rpms/x86_64/ocaml-config-file-devel-1.2-2.fc22.x86_64.rpm
> 1 packages and 0 specfiles checked; 0 errors, 0 warnings.

> srpms/ocaml-config-file-1.2-2.fc22.src.rpm
> ocaml-config-file.src:40: W: configure-without-libdir-spec
> 1 packages and 0 specfiles checked; 0 errors, 1 warnings.

Looks benign, but I guess it won't hurt to add an explicit --libdir=...

Comment 4 Richard W.M. Jones 2015-03-12 15:16:25 UTC
(In reply to Tomas Heinrich from comment #3)
> rpmlint:
> (rpmlint-1.6-2.fc22.noarch)
> 
> > rpms/x86_64/ocaml-config-file-1.2-2.fc22.x86_64.rpm
> > ocaml-config-file.x86_64: E: no-binary
> > ocaml-config-file.x86_64: W: only-non-binary-in-usr-lib
> > 1 packages and 0 specfiles checked; 1 errors, 1 warnings.
> 
> Should this package actually be noarch? Can you provide a rationale why,
> either way?

In this very specific instance (but not in OCaml libraries in
general) the ocaml-config-file package contains only bytecode.
However we have found in the past that bytecode isn't completely
non-arch-specific, so I'd be very dubious about making the
subpackage noarch.  It would require you to build on arm/x86/x86-64
and then manually compare the files to check there are really
no differences.

The -devel subpackage has a *.cmxs file which is really a shared
library of native code, so that's certainly not noarch.

I would not advise making either subpackage noarch.

> > rpms/x86_64/ocaml-config-file-debuginfo-1.2-2.fc22.x86_64.rpm
> > ocaml-config-file-debuginfo.x86_64: E: debuginfo-without-sources
> > 1 packages and 0 specfiles checked; 1 errors, 0 warnings.
> 
> Can't really say why this one is happening.

Have you got the filelist handy?  Since OCaml 4, the compiler
supports fairly good DWARF generation, but our debuginfo
tooling doesn't think *.ml is a source file.

Also you may have to change the invocation of ocamlc & ocamlopt
to pass -g everywhere.  Typically upstream OCaml packages
don't do this consistently.  If you're not passing -g to
everything, then you'll end up with empty/broken debuginfo
which might be what's happening here.

> > rpms/x86_64/ocaml-config-file-devel-1.2-2.fc22.x86_64.rpm
> > 1 packages and 0 specfiles checked; 0 errors, 0 warnings.
> 
> > srpms/ocaml-config-file-1.2-2.fc22.src.rpm
> > ocaml-config-file.src:40: W: configure-without-libdir-spec
> > 1 packages and 0 specfiles checked; 0 errors, 1 warnings.
> 
> Looks benign, but I guess it won't hurt to add an explicit --libdir=...

It depends if the configure script supplied by upstream is a
real autotools configure, or something else.  It might choke
on --libdir.

Comment 5 Tomas Heinrich 2015-03-12 15:18:07 UTC
The 'Requires:' in -devel should be made arch-specific:
Requires:  %{name}%{?_isa} = %{version}-%{release}
https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

Couple of minor things:
- You can make the build process more verbose by adding "V=1", e.g.
  make V=1 opt
- You can disregard and trim the %changelog section until an actual first release

Comment 6 Tomas Heinrich 2015-03-12 15:37:15 UTC
(In reply to Richard W.M. Jones from comment #4)

Thanks for the comments.

> In this very specific instance (but not in OCaml libraries in
> general) the ocaml-config-file package contains only bytecode.
> However we have found in the past that bytecode isn't completely
> non-arch-specific, so I'd be very dubious about making the
> subpackage noarch.  It would require you to build on arm/x86/x86-64
> and then manually compare the files to check there are really
> no differences.
> 
> The -devel subpackage has a *.cmxs file which is really a shared
> library of native code, so that's certainly not noarch.
> 
> I would not advise making either subpackage noarch.

OK, just want to have this documented here.

> > > rpms/x86_64/ocaml-config-file-debuginfo-1.2-2.fc22.x86_64.rpm
> > > ocaml-config-file-debuginfo.x86_64: E: debuginfo-without-sources
> > > 1 packages and 0 specfiles checked; 1 errors, 0 warnings.
> > 
> > Can't really say why this one is happening.
> 
> Have you got the filelist handy?  Since OCaml 4, the compiler
> supports fairly good DWARF generation, but our debuginfo
> tooling doesn't think *.ml is a source file.

$ rpm -qlp rpms/x86_64/ocaml-config-file-debuginfo-1.2-2.fc22.x86_64.rpm
/usr/lib/debug
/usr/lib/debug/.build-id
/usr/lib/debug/.build-id/49
/usr/lib/debug/.build-id/49/e2bfe8f16e0685485f0c8fa717cc2a8b61feb1
/usr/lib/debug/.build-id/49/e2bfe8f16e0685485f0c8fa717cc2a8b61feb1.debug
/usr/lib/debug/usr
/usr/lib/debug/usr/lib64
/usr/lib/debug/usr/lib64/ocaml
/usr/lib/debug/usr/lib64/ocaml/config-file
/usr/lib/debug/usr/lib64/ocaml/config-file/config_file.cmxs.debug
$ find build/config-file-1.2/ | sort
<...>
build/config-file-1.2/config_file.cmi
build/config-file-1.2/config_file.cmo
build/config-file-1.2/config_file.cmx
build/config-file-1.2/config_file.cmxs
build/config-file-1.2/config_file.ml
build/config-file-1.2/config_file.mli
build/config-file-1.2/config_file.o
build/config-file-1.2/config_file_parser.ml4
build/config-file-1.2/debugfiles.list
build/config-file-1.2/debuglinks.list
build/config-file-1.2/debugsources.list
build/config-file-1.2/elfbins.list
build/config-file-1.2/example.ml
<...>

> Also you may have to change the invocation of ocamlc & ocamlopt
> to pass -g everywhere.  Typically upstream OCaml packages
> don't do this consistently.  If you're not passing -g to
> everything, then you'll end up with empty/broken debuginfo
> which might be what's happening here.

Looking at the Makefile, I guess it would have to be patched to pass "-g" everywhere.

> > Looks benign, but I guess it won't hurt to add an explicit --libdir=...
> 
> It depends if the configure script supplied by upstream is a
> real autotools configure, or something else.  It might choke
> on --libdir.

Checked the configure script and tried a buildrun with the flag, didn't notice any issues.

Comment 7 Nikos Mavrogiannopoulos 2015-03-12 15:39:21 UTC
> Looks benign, but I guess it won't hurt to add an explicit --libdir=...
Done, it seems that this was an autoconf file after all. 

> The 'Requires:' in -devel should be made arch-specific:
> Requires:  %{name}%{?_isa} = %{version}-%{release}
Done.

>  make V=1 opt
Done.

> Also you may have to change the invocation of ocamlc & ocamlopt
> to pass -g everywhere
Done.

Spec URL: http://people.redhat.com/nmavrogi/fedora/ocaml-config-file.spec
SRPM URL: http://people.redhat.com/nmavrogi/fedora/ocaml-config-file-1.2-3.fc21.src.rpm

Comment 8 Tomas Heinrich 2015-03-12 15:54:19 UTC
One more thing (based on the previous srpm):
The datadir (e.g. /usr/lib64/ocaml/config-file) isn't owned by the package.

Comment 9 Nikos Mavrogiannopoulos 2015-03-12 16:00:39 UTC
Updated spec files (with same version):
Spec URL: http://people.redhat.com/nmavrogi/fedora/ocaml-config-file.spec
SRPM URL: http://people.redhat.com/nmavrogi/fedora/ocaml-config-file-1.2-3.fc21.src.rpm

Comment 10 Tomas Heinrich 2015-03-13 12:58:22 UTC
A scratch build passed:

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

I think the package is good to go.

Comment 11 Nikos Mavrogiannopoulos 2015-03-13 12:59:49 UTC
Thanks.

Comment 12 Nikos Mavrogiannopoulos 2015-03-13 13:01:04 UTC
New Package SCM Request
=======================
Package Name: ocaml-config-file
Short Description: Configuration file management for OCaml
Owners: nmav
Branches: f21 epel7
InitialCC:

Comment 13 Nikos Mavrogiannopoulos 2015-03-13 13:01:29 UTC
New Package SCM Request
=======================
Package Name: ocaml-config-file
Short Description: Configuration file management for OCaml
Owners: nmav
Branches: f21 f22 epel7
InitialCC:

Comment 14 Gwyn Ciesla 2015-03-13 13:44:58 UTC
Git done (by process-git-requests).

Comment 15 Fedora Update System 2015-03-13 15:04:59 UTC
ocaml-config-file-1.2-3.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/ocaml-config-file-1.2-3.fc21

Comment 16 Fedora Update System 2015-03-13 16:08:03 UTC
ocaml-config-file-1.2-3.el7 has been submitted as an update for Fedora EPEL 7.
https://admin.fedoraproject.org/updates/ocaml-config-file-1.2-3.el7

Comment 17 Fedora Update System 2015-03-19 15:52:36 UTC
ocaml-config-file-1.2-3.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/ocaml-config-file-1.2-3.fc22

Comment 18 Fedora Update System 2015-03-23 08:18:33 UTC
ocaml-config-file-1.2-4.fc22 has been submitted as an update for Fedora 22.
https://admin.fedoraproject.org/updates/ocaml-config-file-1.2-4.fc22

Comment 19 Fedora Update System 2015-03-23 15:14:18 UTC
ocaml-config-file-1.2-4.fc21 has been submitted as an update for Fedora 21.
https://admin.fedoraproject.org/updates/ocaml-config-file-1.2-4.fc21

Comment 20 Fedora Update System 2015-03-30 07:07:10 UTC
ocaml-config-file-1.2-4.fc22 has been pushed to the Fedora 22 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 21 Fedora Update System 2015-04-01 01:58:19 UTC
ocaml-config-file-1.2-3.el7 has been pushed to the Fedora EPEL 7 stable repository.

Comment 22 Fedora Update System 2015-04-05 14:29:12 UTC
ocaml-config-file-1.2-4.fc21 has been pushed to the Fedora 21 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.