Product: Fedora https://bugzilla.redhat.com/show_bug.cgi?id=959926 Michael Schwendt <mschwendt@xxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|nobody@xxxxxxxxxxxxxxxxx |mschwendt@xxxxxxxxx Flags| |fedora-review? --- Comment #2 from Michael Schwendt <mschwendt@xxxxxxxxx> --- A first look at the spec file. Mostly minor issues (related to Fedora Packaging Guidelines): * rpmlint output is clean except for false positives about spelling errors and shared-lib-calls-exit which is known and covered by a git pull request already. > BuildRequires: boost-devel%{?_isa} > BuildRequires: cmake%{?_isa} >= 2.8 > BuildRequires: doxygen%{?_isa} > BuildRequires: fdupes%{?_isa} > BuildRequires: gcc-c++%{?_isa} > BuildRequires: graphviz%{?_isa} > BuildRequires: pkgconfig%{?_isa} Using %_isa in BuildRequires is more confusing than beneficial. First of all, these "BuildRequires" become the src.rpm's "Requires": $ rpm -qpR libyui-3.0.3*.src.rpm boost-devel(x86-64) cmake(x86-64) >= 2.8 doxygen(x86-64) fdupes(x86-64) gcc-c++(x86-64) graphviz(x86-64) pkgconfig(x86-64) rpmlib(FileDigests) <= 4.6.0-1 rpmlib(CompressedFileNames) <= 3.0.4-1 And usually one publishes a single src.rpm in a common "source" repository for all target architectures. The expanded %_isa value is the one from the build environment the src.rpm has been built on. That may be wrong and confusing. It could even be from a build machine with a completely different architecture. For example, "boost-devel(x86-64)" is not available for i686, where the requirement ought to be "boost-devel(x86-32)". It would be strictly required to rebuild the src.rpm to fix its "Requires". Even if no conditional arch-dependent build requirements are used in the spec file. Conclusively: Not using %_isa in BuildRequires is fine and much more common, too. > BuildRequires: gcc-c++%{?_isa} gcc-c++ (as well as gcc and a few more) are expected to be available in the "minimum build environment": https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRequires -> https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2 > %package devel > > Requires: glibc-devel%{?_isa} > Requires: libstdc++-devel%{?_isa} Same here. Except that are indirectly required by "gcc gcc-c++" already. > Requires: %{name}%{?_isa} = %{version} At Fedora, the base package dependency is more strict and includes %{release}, too. This is because either the base package or the subpackage may be patched, and a strict dependency enforces that they match eachother: https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package > Summary: Header files for %{name} More accurate would be something like Summary: Files needed for developing with %{name} since the package clearly contains more than just the headers. ;-) > %package devel > > URL: https://github.com/%{name}/%{name}/ No need to duplicate the URL tag here. It is inherited from the base package URL. > %package doc > > Group: Development/Libraries > > URL: https://github.com/%{name}/%{name}/ Same here. Plus, group "Documentation" would be appropriate: https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation > %build > CFLAGS="${CFLAGS:-%optflags}" ; export CFLAGS ; > CXXFLAGS="${CXXFLAGS:-%optflags}" ; export CXXFLAGS ; > FFLAGS="${FFLAGS:-%optflags%{?_fmoddir: -I%_fmoddir}}" ; export FFLAGS ; > FCFLAGS="${FCFLAGS:-%optflags%{?_fmoddir: -I%_fmoddir}}" ; export FCFLAGS ; > %{?__global_ldflags:LDFLAGS="${LDFLAGS:-%__global_ldflags}" ; export LDFLAGS ;} > cmake .. \ Check out the %cmake macro which makes all this easier (rpm --eval %cmake): http://fedoraproject.org/wiki/Packaging:Cmake > %install > cd build > rm -rf %{buildroot} It is emptied automatically already: https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRoot_tag > make DESTDIR=%{buildroot} install Nowadays there's a %make_install macro for that. > install -m0644 Using also option -p can be helpful for packages, not just if they contain old/ancient files: https://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps > %clean > rm -rf %{buildroot} https://fedoraproject.org/wiki/Packaging:Guidelines#.25clean > %dir %{_datadir}/%{name} > > -- Theme-Dir: /usr/share/libyui/theme It would make sense to include this [empty] directory in this package, because there is code in the lib that uses this path. A pkgconfig variable is defined for it, too. > %doc %dir %{_defaultdocdir}/%{name}-%{version} It's a directory and never marked as %doc. Files are to be marked as %doc. > %defattr(-,root,root) https://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions > %files devel > %doc %dir %{_defaultdocdir}/%{name}-%{version} > %doc %{_defaultdocdir}/%{name}-%{version}/C* https://fedoraproject.org/wiki/Packaging:Guidelines#Duplicate_Files Same for the -doc subpackage. > %{_libdir}/cmake/%{name} https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership https://fedoraproject.org/wiki/Packaging:UnownedDirectories * Doing a test-build with a modified spec file using the %cmake macro, build.log output gets more verbose and reveals that somewhere "-O3 -g3" is being appended: https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags -- You are receiving this mail because: You are on the CC list for the bug. Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=SPCByHLopl&a=cc_unsubscribe _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review