[Bug 882617] Review Request: jsoncpp - An implementation of a JSON reader and writer in C++

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Product: Fedora
https://bugzilla.redhat.com/show_bug.cgi?id=882617

Michael Schwendt <mschwendt@xxxxxxxxx> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mschwendt@xxxxxxxxx

--- Comment #2 from Michael Schwendt <mschwendt@xxxxxxxxx> ---
> Name:		jsoncpp
> Group:		Development/Libraries

As long as we still fill in the Group Tag, library base packages still enter
group "System Environment/Libraries". Group "Development/Libraries" is not for
the run-time libs but e.g. -devel packages.


> %package devel
> Requires:	%{name} = %{version}-%{release}

https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package


> Requires:	pkgconfig

There is an automatic dependency for a long time.


> %package static
> Summary:	Static libraries for %{name}

What's the reason you don't adhere to
https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries
?


> sed 's/CCFLAGS = "-Wall"/CCFLAGS = "%{optflags}"/' -i SConstruct

As convenient as sed (or Perl) substitutions like this may be, a common mistake
is to forget adding a guard. You want to ensure that this expression still
matches. Or else it might not apply anymore and you would not apply %optflags
then. For example, add a corresponding "grep" command that makes the rpmbuild
exit when the 'CCFLAGS = "-Wall"' is not found anymore and could not be
substituted.


> %check
> scons platform=linux-gcc check %{?_smp_mflags}

Good! It's always a pleasure to stumble into new packages where the packager
uses %check to run available test-suites.


> %install
> install -m 0644 include/json/*.h $RPM_BUILD_ROOT%{_includedir}/%{name}/json

https://fedoraproject.org/wiki/Packaging:Guidelines#Timestamps
Even if several of the installed files may be fresh builds.


> %{_libdir}/lib%{name}.so.%{src_release}

Have you done a test-build already?
What is the library SONAME and its version?


> %{_libdir}/lib%{name}.a.%{src_release}

Odd. So, with this file name you would link it via its full path instead of
just "-ljsoncpp"?


> %{_includedir}/%{name}

The more readable form for directories is the one with a trailing slash:

  %{_includedir}/%{name}/

Not so important for an include dir like this but more readable in general and
more explicit where a file entry could also refer to a single file only.


> %files
> %doc AUTHORS LICENSE NEWS.txt README.txt
>
> %files static
> %doc AUTHORS LICENSE NEWS.txt README.txt
>
> %files devel
> %doc AUTHORS LICENSE NEWS.txt README.txt
>
> %files doc
> %doc AUTHORS LICENSE NEWS.txt README.txt

Even if storage space is cheap, these files are duplicated in too many places.
Please revisit
https://fedoraproject.org/wiki/Packaging:Guidelines#Duplicate_Files

-- 
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=jhg6Q59ues&a=cc_unsubscribe
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review



[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]