[Bug 606498] Review Request: hwloc - portable abstraction of hierarchical architectures

Fri Jul 9 14:09:55 UTC 2010

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

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

--- Comment #5 from Jiri Hladky <jhladky at redhat.com> 2010-07-09 10:09:54 EDT ---
Hi Mamoru,

thanks a lot for looking into it!

I have followed your advises and fixed the issues you have pointed out. Updated
version can be downloaded here:

Spec URL: http://jhladky.fedorapeople.org/hwloc.spec-18
MD5: 4c9fbaa104dd6106cbbb7386baa5dfec

Spec URL: http://jhladky.fedorapeople.org/hwloc-1.0.1-18.fc12.src.rpm
MD5: 16cfa71f386a3b27539d2ef25e047702

See my comments below:

> Some initial comments:
> 
> ? Release number
>   - By the way, why does the release number of your spec file
>     begin with 17 (instead of 1)?
> 

I'm already using this package on RHEL systems. I had lots of building issues
on RHEL4 because of faulty evolution28-cairo-devel rpm package. That's why
Release number is so high. Sorry for it.

> ! EPEL4 handling
>   - Well, as I don't have RHEL product, I don't care for EPEL handling.
>     However some notes:
>     * Maybe it is better that you create another spec file for EPEL4 only and
>       remove all %{?rhel} handling, because (Build)Requires are so different,
>       however this is not a blocker (however see below)

Done. I have now standalone SPEC for RHEL4. I have dropped vector graphic
support on RHEL4. Posted spec file is for Fedora, RHEL5 and RHEL6.

>     * Would you explain why
>       - You have disabled AutoReq

That was only for RHEL4. I have removed it. (reason: broken
evolution28-cairo-devel rpm package.)

>       - And why main package (hwloc) has some Requires for development-purpose
>         packages (i.e. has "Requires: foo-devel")? This seems strange because
>         on Fedora side you don't write such Requires.

That was only for RHEL4. I have removed it.

> * make build.log more verbose
>   - Currently build.log does not show how linkage between binaries is done
>     during compiling.
>     - Also for EPEL4 build, build.log shows like:
> ------------------------------------------------------------
> + /usr/bin/make -j8
> Making all in src
> make[1]: Entering directory `/builddir/build/BUILD/hwloc-1.0.1/src'
>   CC     topology.lo
>   CC     traversal.lo
>   CC     topology-synthetic.lo
>   CC     cpuset.lo
>   CC     misc.lo
>   CC     bind.lo
> ------------------------------------------------------------
>       From this we cannot check if compilation flags are honored correctly
>       or not.
>     Please add "V=1" to "make %{?_smp_mflags}" to make build.log more verbose.

Done.

>* Requires for -devel subpackage
>  - Would you check if Requires on -devel subpackage is needed, other than
>    libxml2-devel?
>    * Installed header files don't seem to require any of these, and the >installed
>      pkgconfig .pc file only requires libxml-2.0 (as Requires.private)
>
>  ! Note
>    - On Fedora 12+, rpmbuild checks the dependencies on pkgconfig file and
>      "Requires: pkgconfig(libxml-2.0)" is automatically added to -devel
>subpackage,
>      so (on Fedora) "Requires: libxml2-devel" for -devel subpackage is not
>      (explicitly) needed.

I let now rpmbuild to decide about Requires.

> * Macros
>   - Please use macros for standard directories, %{_prefix} for /usr, %{_bindir}
>     for /usr/bin:
>     https://fedoraproject.org/wiki/Packaging/RPMMacros
>     - and %{_prefix}/share should be %{_datadir}
>   - And please use macros consistently:
>     - If you use %{__rm}, %{__make} or so, please also use %{__sed}.
>       and both %{__rm} and rm are used, please choose one.

Done.

> * Timestamp
>   - When using cp or install command, also add "-p" option to keep timestamps
>     on installed files:
>     https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps

I have -p already in the spec file:
%{__make} install DESTDIR=%{buildroot} INSTALL="%{__install} -p"
Is anything wrong with the line above?

> * Rpath handling
>   - So are you going to remove by this way instead of the following one?
>     (Note: not a blocker)
>     http://lists.fedoraproject.org/pipermail/packaging/2010-June/007187.html

I have tried to use the solution proposed in the mailing thread above. However,
there are some issues:

1) I'm not sure if I can do it for all archs - should I add conditional to do
it only on x86_64? To be honest I don't know about ARM, MIPS, Parisc - are
libraries on these archs stored at /usr/lib64 ??
https://fedoraproject.org/wiki/Architectures
Therefore I would prefer to have a solution common for all archs.
2) rpmlint --verbose --info SPECS/hwloc.spec
SPECS/hwloc.spec:42: E: hardcoded-library-path in /lib

It's complaining about this line:
%{__sed} -i.libdir_syssearch -e '/sys_lib_dlsearch_path_spec/s|/usr/lib
|/usr/lib /usr/lib64 /lib /lib64|' configure

Because of these issues I have stuck with solutions proposed at
http://fedoraproject.org/wiki/RPath_Packaging_Draft

> * Directory ownership issue
>   - %{_datadir}/%{name} itself is not owned by any packages.
>   - -devel subpackage need now own the directory

I have fixed it by adding
%dir %{_datadir}/%{name}
to %files for hwloc package.

> %{_defaultdocdir}/%{name}-%{version},
>     this is already owned by main package and -devel subpackage depends on
>     main package. 

I have removed %dir %{_defaultdocdir}/%{name}-%{version} from %files section of
hwloc-devel package.

Please have a look at new spec file.

Thanks a lot!
Jirka

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.