Re: [dpdk-dev] [PATCH 03/16] pkg: add recipe for RPM

[Thomas Monjalon <thomas.monjalon@6wind.com>]

Hello,

Sorry for the long delay.

2014-02-24 08:52, Chris Wright:
> >  pkg/rpm.spec |  143
> 
> This should be dpdk.spec

Actually it should be dpdk-core.spec.
Since it is a file hosted in the project, is it mandatory to have such naming? 
Could you explain why?
When building it with "rpmbuild -ta dpdk.tar.gz", the .spec name has no 
importance.

> > +ExclusiveArch: i686, x86_64
> > +%define target %{_arch}-default-linuxapp-gcc
> 
> What is this for?

Multi-targets builds are supported in DPDK Makefiles. So the target must be 
explicited when building.
Is it OK to define such variable here?

> > +%description
> > +Dummy main package. Make only subpackages.
> 
> I think the core package should have the .so, the -devel package w/
> headers, and no -static package.  Including a .a file is discouraged in
> Fedora

OK

> > +%package core-runtime
> 
> perhaps just 'runtime'
[...]
> > +%package core-devel
> 
> name this devel

No, dpdk-core is a name to differentiate the main component and the extensions 
such as dpdk-memnic.
So we should have dpdk-core and dpdk-core-devel. Do you agree?

> > +# debuginfo packaging is broken
> > +%define debug_package %{nil}
> 
> Can you add a better comment why it's broken?

Yes, I think it's due to direct use of ld linker.

> > +make O=%{target} T=%{target} config
> 
> Did you try to update the build system to output into $RPM_BUILD_ROOT
> directly?  It's much preferred compared to the cp/rmdir below

Probably we could change the build system but it should be another patch.

> > +%install
> > +rm -rf %{buildroot}
> > +make           O=%{target}     DESTDIR=%{destdir}
> 
> Why make again (IOW, why not in %build section)?

This one is for installing files only. Isn't it the role of %install?

> > +mv    %{destdir}/%{target}/kmod/*.ko   %{buildroot}%{moddir}
> 
> Normally I'd use install command (-d will create directories)

Yes but here the strategy is to dispatch some files and keep the remaining 
ones in %{datadir}.

Thanks for the review
-- 
Thomas