Bug 1289760 (drawtk) - Review Request: drawtk - A C library to perform efficient 3D drawings
Summary: Review Request: drawtk - A C library to perform efficient 3D drawings
Keywords:
Status: CLOSED RAWHIDE
Alias: drawtk
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
unspecified
Target Milestone: ---
Assignee: Zbigniew Jędrzejewski-Szmek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: fedora-neuro, NeuroFedora
TreeView+ depends on / blocked
 
Reported: 2015-12-08 21:48 UTC by Dmitry Mikhirev
Modified: 2016-02-02 21:11 UTC (History)
3 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-02-02 21:11:09 UTC
Type: Bug
Embargoed:
zbyszek: fedora-review+


Attachments (Terms of Use)

Description Dmitry Mikhirev 2015-12-08 21:48:02 UTC
Spec URL: http://copr-dist-git.fedorainfracloud.org/cgit/bizdelnick/neuro/drawtk.git/plain/drawtk.spec?id=08590e1ee1bdc91e659a9a9a270712590356c947
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/bizdelnick/neuro/fedora-rawhide-x86_64/00145338-drawtk/drawtk-2.0-1.fc24.src.rpm

Description:

drawtk provides a C library to perform efficient 2D drawings. The drawing is done by OpenGL which allow us fast and nice rendering of basic shapes, text, images and video (file, webcam, network). It has been implemented as a thin layer that hides the complexity of the OpenGL library.

Comment 1 Zbigniew Jędrzejewski-Szmek 2015-12-20 00:16:02 UTC
How alive is this project? It doesn't even seem to have a publicly visible repository, it's hard to find a changelog. Do you need it as a dependency for something else, or do you intend to use it for new development?

Comment 2 Ranjan Maitra 2015-12-21 00:39:54 UTC
Need to answer Commment 1. Please run fedora-review and fix the issues: for your convenience, I have manually reviewed the current issues (to the best of my current knowledge).

Also fedora-review provides the following:

- BuildRequires not needed: gcc

===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig called in %post and %postun if required.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.
[x]: Development (unversioned) .so files in -devel subpackage, if present.


Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[!]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "GPL (v2 or later)", "GPL (v3 or later)", "Unknown or
     generated", "MIT/X11 (BSD like)", "LGPL (v3 or later)", "LGPL (v2.1 or
     later)". 86 files have unknown license. 
[!]: License file installed when any subpackage combination is installed.
[!]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[!]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[?]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[!]: Requires correct, justified where necessary.
     add gcc
[x]: Spec file is legible and written in American English.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[!]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[!]: Final provides and requires are sane (see attachments).
[!]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in drawtk-
     debuginfo
[!]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: SourceX tarball generation or download is documented.
     Note: Package contains tarball without URL, check comments
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[!]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[!]: Rpmlint is run on all installed packages.
     Note: Mock build failed
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Spec file according to URL is the same as in SRPM.

Comment 3 Ranjan Maitra 2015-12-21 00:41:10 UTC
Sorry, forgot to add the legend for the previous comment 2.


Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed

Comment 4 Dmitry Mikhirev 2015-12-29 15:44:22 UTC
There is a public repository on github: https://github.com/nbourdau/drawtk
I archived sources from alioth repo because it contains exactly the same files that were in the latest released tarball that seems lost.

This library is needed only for possible future development.

>[!]: License field in the package spec file matches the actual license.

Thank you, I will fix this.

>[!]: License file installed when any subpackage combination is installed.
>[!]: %build honors applicable compiler flags or justifies otherwise.

What is wrong here?

>[!]: Package contains desktop file if it is a GUI application.

It is not a GUI application.

>[!]: Requires correct, justified where necessary.
>     add gcc

Require gcc? Why?

Comment 5 Zbigniew Jędrzejewski-Szmek 2016-01-07 17:33:14 UTC
Some comments on Ranjan's review (going into a bit more detail than I
usually would):

(In reply to Ranjan Maitra from comment #2)
> Also fedora-review provides the following:
> 
> - BuildRequires not needed: gcc

This changed recently. https://fedorahosted.org/fpc/ticket/490 and
https://fedorahosted.org/fpc/ticket/497 have the full history, but the relevant
part is the following change to guidelines
[https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRequires_2]:

"It is important that your package list all necessary build dependencies using the BuildRequires?: tag. You may assume that enough of an environment exists for RPM to function and execute basic shell scripts, but you should not assume any other packages are present as RPM dependencies and anything brought into the buildroot by the build system may change over time."

fedora-review is wrong here, and one SHOULD have BuildRequires:gcc,
though things work just fine without, and will do so for the forseeable
future. The spec file is correct.

> Generic:
> [x]: Package is licensed with an open-source compatible license and meets
>      other legal requirements as defined in the legal section of Packaging
>      Guidelines.
> [!]: License field in the package spec file matches the actual license.
>      Note: Checking patched sources after %prep for licenses. Licenses
>      found: "GPL (v2 or later)", "GPL (v3 or later)", "Unknown or
>      generated", "MIT/X11 (BSD like)", "LGPL (v3 or later)", "LGPL (v2.1 or
>      later)". 86 files have unknown license. 

The spec file specifies License:LGPLv3+. According to the COPYING file
and to https://github.com/nbourdau/drawtk/commit/46604561b6d3396e067ab
this is correct.

Note: the License tag specifies the *effective* license of the *binary* package [1, 2]. So anything that is not part of the binary rpm (like install/install-sh) doesn't matter at all. If you combine GPLv2+ with BSD or LGPL or MIT, the effective license is GPLv2+. So, if a file in the binary rpm has at least one GPL licensed file, that file is GPL. If a file only had sources with more permissive licenses, than that file would have some other license. So you need to determine the effective license of all files in the binary rpm and put the result in License. What licensecheck returns in the review is just the starting point.

[1] https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License:_field
[2] https://fedoraproject.org/wiki/Licensing:FAQ?rd=Licensing/FAQ#What_is_.22effective_license.22_and_do_I_need_to_know_that_for_the_License:_tag.3F

Licensecheck says:
drawtk-2.0/AUTHORS: UNKNOWN
drawtk-2.0/configure.ac: *No copyright* UNKNOWN
drawtk-2.0/INSTALL: UNKNOWN
drawtk-2.0/Makefile.am: *No copyright* UNKNOWN
drawtk-2.0/README: *No copyright* UNKNOWN
drawtk-2.0/configure: GENERATED FILE
drawtk-2.0/aclocal.m4: GENERATED FILE
drawtk-2.0/tests/Makefile.am: *No copyright* UNKNOWN
drawtk-2.0/tests/navy.png: *No copyright* UNKNOWN
drawtk-2.0/tests/test-video-custom.c: GPL (v3 or later)
drawtk-2.0/tests/navy.png.license: LGPL (v2.1 or later)
drawtk-2.0/tests/test.ogv: UNKNOWN
drawtk-2.0/tests/test1.c: GPL (v3 or later)
drawtk-2.0/tests/Makefile.in: GENERATED FILE
drawtk-2.0/tests/test-video.c: GPL (v3 or later)
drawtk-2.0/tests/test-events.c: GPL (v3 or later)
drawtk-2.0/doc/dtk_addtime.3: UNKNOWN
drawtk-2.0/doc/dtk_nanosleep.3: UNKNOWN
drawtk-2.0/doc/dtk_load_video_tcp.3: UNKNOWN
drawtk-2.0/doc/dtk_bgcolor.3: *No copyright* UNKNOWN
drawtk-2.0/doc/dtk_create_image.3: UNKNOWN
drawtk-2.0/doc/dtk_update_screen.3: UNKNOWN
drawtk-2.0/doc/dtk_move_shape.3: UNKNOWN
drawtk-2.0/doc/dtk_window_getsize.3: UNKNOWN
drawtk-2.0/doc/dtk_create_string.3: UNKNOWN
drawtk-2.0/doc/dtk_set_event_handler.3: *No copyright* UNKNOWN
drawtk-2.0/doc/Makefile.am: *No copyright* UNKNOWN
drawtk-2.0/doc/dtk_create_rectangle_2p.3: UNKNOWN
drawtk-2.0/doc/dtk_difftime_us.3: *No copyright* UNKNOWN
drawtk-2.0/doc/dtk_create_composite_shape.3: UNKNOWN
drawtk-2.0/doc/dtk_gettime.3: UNKNOWN
drawtk-2.0/doc/dtk_relmove_shape.3: *No copyright* UNKNOWN
drawtk-2.0/doc/dtk_load_video_gst.3: UNKNOWN
drawtk-2.0/doc/dtk_load_image.3: UNKNOWN
drawtk-2.0/doc/dtk_create_rectangle_hw.3: *No copyright* UNKNOWN
drawtk-2.0/doc/dtk_destroy_font.3: *No copyright* UNKNOWN
drawtk-2.0/doc/dtk_relrotate_shape.3: *No copyright* UNKNOWN
drawtk-2.0/doc/dtk_video_getstate.3: UNKNOWN
drawtk-2.0/doc/dtk_video_exec.3: UNKNOWN
drawtk-2.0/doc/examples/bars.c: GPL (v3 or later)
drawtk-2.0/doc/examples/bounce.c: GPL (v3 or later)
drawtk-2.0/doc/examples/errormon.c: GPL (v3 or later)
drawtk-2.0/doc/examples/Makefile: *No copyright* UNKNOWN
drawtk-2.0/doc/dtk_process_events.3: UNKNOWN
drawtk-2.0/doc/dtk_load_video_test.3: UNKNOWN
drawtk-2.0/doc/dtk_setcolor_shape.3: UNKNOWN
drawtk-2.0/doc/dtk_create_cross.3: UNKNOWN
drawtk-2.0/doc/dtk_create_complex_shape.3: UNKNOWN
drawtk-2.0/doc/dtk_get_color.3: UNKNOWN
drawtk-2.0/doc/dtk_draw_shape.3: UNKNOWN
drawtk-2.0/doc/dtk_create_circle.3: UNKNOWN
drawtk-2.0/doc/dtk_close.3: *No copyright* UNKNOWN
drawtk-2.0/doc/dtk_create_circle_str.3: UNKNOWN
drawtk-2.0/doc/dtk_create_arrow.3: UNKNOWN
drawtk-2.0/doc/dtk_texture_getsize.3: UNKNOWN
drawtk-2.0/doc/dtk_difftime_ns.3: *No copyright* UNKNOWN
drawtk-2.0/doc/dtk_create_shape.3: UNKNOWN
drawtk-2.0/doc/Makefile.in: GENERATED FILE
drawtk-2.0/doc/dtk_destroy_texture.3: UNKNOWN
drawtk-2.0/doc/dtk_load_video_udp.3: UNKNOWN
drawtk-2.0/doc/dtk_create_window.3: UNKNOWN
drawtk-2.0/doc/dtk_clear_screen.3: *No copyright* UNKNOWN
drawtk-2.0/doc/dtk_destroy_shape.3: UNKNOWN
drawtk-2.0/doc/dtk_load_video_file.3: UNKNOWN
drawtk-2.0/doc/dtk_difftime_s.3: UNKNOWN
drawtk-2.0/doc/dtk_rotate_shape.3: *No copyright* UNKNOWN
drawtk-2.0/doc/dtk_difftime_ms.3: *No copyright* UNKNOWN
drawtk-2.0/doc/dtk_make_current_window.3: UNKNOWN
drawtk-2.0/doc/dtk_load_font.3: UNKNOWN
drawtk-2.0/doc/dtk_create_line.3: UNKNOWN
drawtk-2.0/doc/dtk_create_triangle.3: UNKNOWN
drawtk-2.0/lib/Makefile.am: *No copyright* UNKNOWN
drawtk-2.0/lib/clock_gettime.h: LGPL (v3 or later)
drawtk-2.0/lib/clock_nanosleep.h: LGPL (v3 or later)
drawtk-2.0/lib/clock_gettime.c: LGPL (v3 or later)
drawtk-2.0/lib/convfiletime.h: LGPL (v3 or later)
drawtk-2.0/lib/timespec.h: LGPL (v3 or later)
drawtk-2.0/lib/Makefile.in: GENERATED FILE
drawtk-2.0/lib/clock_nanosleep.c: LGPL (v3 or later)
drawtk-2.0/config/config.h.in: *No copyright* GENERATED FILE
drawtk-2.0/NEWS: *No copyright* UNKNOWN
drawtk-2.0/COPYING: UNKNOWN
drawtk-2.0/Makefile.in: GENERATED FILE
drawtk-2.0/ChangeLog: *No copyright* UNKNOWN
drawtk-2.0/m4/pkg-custom.m4: GPL (v2 or later) (with incorrect FSF address) GENERATED FILE
drawtk-2.0/m4/api-exports.m4: *No copyright* UNKNOWN
drawtk-2.0/m4/ltsugar.m4: UNKNOWN
drawtk-2.0/m4/lt~obsolete.m4: UNKNOWN
drawtk-2.0/m4/libtool.m4: GPL (v2 or later)
drawtk-2.0/m4/ltversion.m4: GENERATED FILE
drawtk-2.0/m4/ltoptions.m4: UNKNOWN
drawtk-2.0/m4/funcarg.m4: *No copyright* UNKNOWN
drawtk-2.0/src/drawtk.pc.in: *No copyright* UNKNOWN
drawtk-2.0/src/video.c: LGPL (v3 or later)
drawtk-2.0/src/colors.c: LGPL (v3 or later)
drawtk-2.0/src/dtk_time.h: LGPL (v3 or later)
drawtk-2.0/src/shapes.h: LGPL (v3 or later)
drawtk-2.0/src/shapes.c: LGPL (v3 or later)
drawtk-2.0/src/imagetex.c: LGPL (v3 or later)
drawtk-2.0/src/texmanager.c: LGPL (v3 or later)
drawtk-2.0/src/Makefile.am: *No copyright* UNKNOWN
drawtk-2.0/src/fonttex.h: LGPL (v3 or later)
drawtk-2.0/src/window.c: LGPL (v3 or later)
drawtk-2.0/src/vidpipe_creation.c: LGPL (v3 or later)
drawtk-2.0/src/time.c: LGPL (v3 or later)
drawtk-2.0/src/dtk_event.h: LGPL (v3 or later)
drawtk-2.0/src/fonttex.c: LGPL (v3 or later)
drawtk-2.0/src/dtk_colors.h: LGPL (v3 or later)
drawtk-2.0/src/drawtk.h: LGPL (v3 or later)
drawtk-2.0/src/create_shape.c: LGPL (v3 or later)
drawtk-2.0/src/vidpipe_creation.h: LGPL (v3 or later)
drawtk-2.0/src/Makefile.in: GENERATED FILE
drawtk-2.0/src/events.c: LGPL (v3 or later)
drawtk-2.0/src/dtk_video.h: LGPL (v3 or later)
drawtk-2.0/src/texmanager.h: LGPL (v3 or later)
drawtk-2.0/src/window.h: LGPL (v3 or later)
drawtk-2.0/build-aux/install-sh: MIT/X11 (BSD like)
drawtk-2.0/build-aux/compile: GPL (v2 or later) GENERATED FILE
drawtk-2.0/build-aux/config.sub: GPL (v2 or later) GENERATED FILE
drawtk-2.0/build-aux/ltmain.sh: GPL (v2 or later) GENERATED FILE
drawtk-2.0/build-aux/depcomp: GPL (v2 or later) GENERATED FILE
drawtk-2.0/build-aux/config.guess: GPL (v2 or later) GENERATED FILE
drawtk-2.0/build-aux/missing: GPL (v2 or later) GENERATED FILE

As mentioned above, we can disregard anything in build-aux/, m4/,
the Makefiles. In the end it seems we have the main sources, which
are LGPLv3+, and some tests, which are GPLv3+. If the tests end
up in the binary rpm, then "and GPLv3" should be in License.

> [!]: License file installed when any subpackage combination is installed.

I believe this comment is wrong. The main package has the COPYING file,
and the -devel subpackage Requires:%{name}%{?_isa} = %{version}-%{release}.
That is enough.

> [!]: %build honors applicable compiler flags or justifies otherwise.

%configure and %make_build macros are designed to DTRT. When
they are used, the result is almost always correct. Of course is
is always good to look at the build logs:

/bin/sh ../libtool --silent --tag=CC   --mode=compile gcc -DHAVE_CONFIG_H -I. -I../config  -I/usr/include/freetype2 -I/usr/include/libpng16  -pthread -I/usr/include/gstreamer-0.10 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/libxml2     -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m32 -march=i686 -mtune=atom -fasynchronous-unwind-tables -c -o colors.lo colors.c

So yes, the build flags include the hardening macros (-Werror=format-security
-Wp,-D_FORTIFY_SOURCE=2 -fstack-protector-strong) and seem fine.

> [?]: Package uses nothing in %doc for runtime.

The best way to verify that is to check the list of files in the binary rpm
for anything suspicious. In this case, /usr/share/doc/drawtk has some
text files and source files, i.e. nothing which looks like it could be
used by the library.

> Generic:
> [!]: Final provides and requires are sane (see attachments).
> [!]: Fully versioned dependency in subpackages if applicable.
>      Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in drawtk-
>      debuginfo
This dependency is not used in case of -debuginfo packages.
(The reason is that those packages tend to be huge, and forcing them
to be kept in sync automatically using dependencies would make updates
much more heavyweight. -debuginfo packages are allowed to get out of sync,
and the user has to either update them manually or use a yum/dnf
plugin to do that automatically.)

> [!]: Package functions as described.
In case of library packages, if they compile, they usually work
fine. Do you have any reason to believe it is broken? The review
should always describe what is broken, "functions as described"
is too generic to be useful.

> [!]: %check is present and all tests pass.

> [!]: Rpmlint is run on all installed packages.
>      Note: Mock build failed
This is a bug in fedora-review (or maybe mock?). In fact the package builds
and install fine in mock. When doing reviews I simply cut out stuff like that
(when fedora-review or rpmlint screws up), because it serves no purpose and
only confuses the author of the package.

(In reply to Dmitry Mikhirev from comment #4)
> There is a public repository on github: https://github.com/nbourdau/drawtk
> I archived sources from alioth repo because it contains exactly the same
> files that were in the latest released tarball that seems lost.
Please put this comment in the spec file. It will be useful to other
people who rebuild your package or look for sources.

> This library is needed only for possible future development.
OK.

> >[!]: License field in the package spec file matches the actual license.
> 
> Thank you, I will fix this.
Please see my comment above; I don't think it is incorrect.

Comment 7 Zbigniew Jędrzejewski-Szmek 2016-01-24 22:00:08 UTC
Yeah, I think everything is fine and you addressed all the relevant comments from Ranjan and me.

Package is APPROVED.

Comment 8 Gwyn Ciesla 2016-01-25 02:31:28 UTC
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/drawtk


Note You need to log in before you can comment on or make changes to this bug.