Bug 1677989 - Review Request: vcglib Visualization and Computer Graphics Library
Summary: Review Request: vcglib Visualization and Computer Graphics Library
Keywords:
Status: POST
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Robert-André Mauchin
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1658153 1687351
TreeView+ depends on / blocked
 
Reported: 2019-02-17 12:22 UTC by J. Scheurich
Modified: 2019-11-19 16:07 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed:
sanjay.ankur: fedora-review+


Attachments (Terms of Use)

Description J. Scheurich 2019-02-17 12:22:50 UTC
Spec URL: ftp://ftp.ourproject.org/pub/wdune/vcglib.spec
SRPM URL: ftp://ftp.ourproject.org/pub/wdune/vcglib-1.0.1-1.fc30.src.rpm
Description: The Visualization and Computer Graphics Library (VCG for short) 
             is a open source portable C++ templated library for manipulation,
             processing and displaying with OpenGL of triangle and tetrahedral
             meshes.
Fedora Account System Username: muftii

Comment 1 Jerry James 2019-02-18 00:21:40 UTC
The License tag is not in the correct format.  See https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Multiple_Licensing_Scenarios.

Remove inappropriate executable bits in %prep.  For example, some of the header files in the img directory are marked (incorrectly) as executable.

This package bundles eigen3.  To unbundle it:
- Add BuildRequires: eigen3-devel
- Add Requires: eigen3-devel
- In %prep, add something like the following (untested):
# Unbundle eigen3
rm -fr eigenlib
for fil in $(grep -Frl eigenlib .); do
  sed -i 's,<eigenlib\(.*\)>,<eigen3\1>,;s,"\.\./\.\./eigenlib\(.*\)",<eigen3\1>,' $fil
done

After unbundling, check whether that changes the license tag.

You are working much too hard in %install.  Just do this:

mkdir -p %{buildroot}%{_includedir}/%{name}
cp -a apps img vcg wrap %{buildroot}%{_includedir}/%{name}

If that copies some files that should not appear in the binary RPM, then remove them.  I think that will be much more concise that what you have now.

Likewise, you are working much too hard in %files.  I think this is all you need:

%files
%license LICENSE.txt
%{_includedir}/vcglib/

The main package should be empty.  You should have a subpackage created with %package devel, so that the final package name is vcglib-devel.  Look at the way the eigen3 package does this and copy it.

It would be a good idea to run doxygen and place the resulting files in a subpackages named docs.  The PowerPoint file in the docs directory could go in that package as well.

There are a number of C++ files with main() functions in them; e.g., apps/plyrefine/main.cpp.  Should those be built into binaries?  If so, the resulting binaries should go in the main package.

Comment 2 J. Scheurich 2019-02-18 09:32:31 UTC
Spec URL: ftp://ftp.ourproject.org/pub/wdune/vcglib.spec
SRPM URL: ftp://ftp.ourproject.org/pub/wdune/vcglib-1.0.1-1.fc30.src.rpm
Description: The Visualization and Computer Graphics Library (VCG for short) 
             is a open source portable C++ templated library for manipulation,
             processing and displaying with OpenGL of triangle and tetrahedral
             meshes.
Fedora Account System Username: muftii

Update
-specfile shortemd
- i can not unbulde eigenlib. cause fedoras eigenlib is version 3.3.7, while vcglib's copy of eigenlib is 3.2.5
  i can not say if eigen3.3.7 is compatible with eigenlib 3.2.5, i have only one testcase 8-(

Comment 3 Petr Menšík 2019-03-06 18:25:12 UTC
Whatever test case you have, try to make it working with latest eigen3 headers provided by Fedora. That is requirement, it cannot be accepted without it.

I would expect from the library to not prefix includes to its own libraries. Just remove eigenlib/ prefix from includes. Change #include <eigenlib/Eigen/Core> to #include <Eigen/Core> etc. Rely on compiler include paths configured when your library is used. It would be nice if you provided pkg-config file that depends on eigen3 library, but it is not required. Please do not use relative include paths like "../../eigenlib/Eigen/LU".

Users of this package should use CPPFLAGS=`pkg-config --cflags eigen3` when including your headers. If they do, includes will work as expected and will reuse eigen3 headers from system.
It has to live with its dependencies. If some structures were changed, adapt your code to match latest.

Can you provide any example how you want users to use your package?

Comment 4 J. Scheurich 2019-03-07 10:16:29 UTC
The only test example in vcglib that uses a older Eigen3 version to not compile with 
the fedora 30 Eigen version:

 $ pwd
/xxx/vcglib/apps/sample/trimesh_indexing
$ make
make: *** No rule to make target '../../../eigenlib/Eigen/src/Core/Functors.h', needed by 'trimesh_indexing.o'.  Stop.
$ find /usr/include/eigen3 -name Functors.h
$ 

What to do ?

wdune/white_dune uses vcglib as build requirement, but do not use Eigen3...

What to do ?
Should i simply remove the Eigen directory from vcglib ? 
The Eigen test example is not included in the fedora vcglib distribution.

Comment 5 Petr Menšík 2019-03-07 17:18:34 UTC
Removed eigenlib prefix with this command:
pushd vcg
sed -e '/^#include/ s/<eigenlib\//</' vcg/space/fitting3.h -i vcg/{space,math,complex/algorithms,complex/algorithms/parametrization,complex/algorithms,complex/algorithms/update/}/*.h
sed -e '/^#include/ s,"../../eigenlib/\(.*\)",<\1>,' math/eigen.h
popd

However, libary itself does not provide any way to configure or build examples, does it? No Makefile is provided. Can it be tested just from wdune (bug #1658153)

Comment 6 Petr Menšík 2019-03-07 17:19:13 UTC
second sed command is missing -i, should be
sed -e '/^#include/ s,"../../eigenlib/\(.*\)",<\1>,' -i math/eigen.h

Comment 7 Petr Menšík 2019-03-07 17:28:07 UTC
Releases should be the same for any distribution, you should not make different releases for Fedora only. Just include all software related to vcglib inside, it is always possible to not install it. Examples are useful however, I think they should be included in doc files.

Comment 8 Petr Menšík 2019-03-07 17:55:21 UTC
It would be very good if library had its own configure, which could prepare also makefiles for all examples. Examples could be installed into some place. But if they can verify library actually works, they should be definitely buildable from library sources itself. Jerry mentioned them already in comment #1. Examples subpackage be installed in separate subpackage.

Comment 9 Petr Menšík 2019-03-08 10:46:41 UTC
comment #1 states main package vcglib should be empty. It does not mean you have to remove examples from tarball. I think on contrary, it should not be deleted from source archive. We expect you would release one source archive for all distributions, always the same. It can be chosen in spec file, which part would be packaged and which should not be packaged. If you have to delete some parts, delete them in build process only. Deleted from sources should be just parts with incompatible license, nothing else.

I am not sure about that. Main package usually contains only compiled library, check [1] for information. However, this library does not compile any shared library that can be reused. That might be fixed later, I think it does not work as library now but should. Instead, examples could have subpackage examples, with more runtime dependencies on them, especially is they are visually atractive and not simple to compile yourself. Ok, proposal is this. Includes would not contain eigenlib, it has its own project. 

Because vcglib as its own project on github [2], it definitely requires its own package and must NOT be part of wdune package.

Ok, examples are not ready to use system-wide eigen3. But here we can reuse pkgconfig as well [3]. Just append to all used .pro files:

cat > fedora.unix << QMAKE
unix {
    CONFIG += link_pkgconfig
    PKGCONFIG += eigen3
}
QMAKE
find -name '*.pro' | while read P; do cat fedora.unix >> "$P"; done

Then tools can be built. It seems they are not only samples, but also useful tools. They should be packed definitely. utils subpackage could be used for them, with all dependencies required to install them. I think it is strange there is no script to build all those tools in single command, it should be proposed to the project. It can be solved in spec file until that happens. Generated makefiles have no working install target unfortunately.

I think such modifications should be recommended to the project in general, not just for Fedora.

1. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_libraries_and_applications
2. https://github.com/cnr-isti-vclab/vcglib
3. http://qt.shoutwiki.com/wiki/Using_pkg-config_with_qmake

Comment 10 Petr Menšík 2019-03-08 13:00:52 UTC
Found by coincidence, that meshlab project [1] already bundles this library. That is a another reason to produce separate package that can be reused also by it. Is seems to be high-quality project without separate compilation support.

Maintainers of meshlab, are you willing to help to prepare vcglib to reuse from meshlab?

1. https://src.fedoraproject.org/rpms/meshlab

Comment 11 J. Scheurich 2019-03-08 23:36:56 UTC
Spec URL: ftp://ftp.ourproject.org/pub/wdune/vcglib.spec
SRPM URL: ftp://ftp.ourproject.org/pub/wdune/vcglib-1.0.1-1.fc30.src.rpm
Description: The Visualization and Computer Graphics Library (VCG for short) 
             is a open source portable C++ templated library for manipulation,
             processing and displaying with OpenGL of triangle and tetrahedral
             meshes.
Fedora Account System Username: mufti11

Update:
Removed Eigen3 from library (incompatible with fedoras more recent Eigen3 lib)
Added  /usr/share/pkgconfig/vcglib.pc

Comment 12 Petr Menšík 2019-03-13 10:26:20 UTC
pkgconfig file is great, but please move creation of it into %prep section. Just use cat > vcglib.pc ... in %prep, then in %install just:
install vcglib.pc %{buildroot}%{_datarootdir}/pkgconfig/vcglib.pc

Please add "Requires: eigen3" to vcglib.pc

You do not have to manually install README file. Just use in %files:
%doc README.md
It would install all such files from source directory for you.

I think if you want to correct mode of source files, that should be done also in %prep.
Might help:
find -name '*.h' -exec chmod a-x '{}' ';'
find -name '*.cpp' -exec chmod a-x '{}' ';'

Then in install section, just copy vcg subdirectory:
cp -a vcg %{buildroot}%{_includedir}/%{name}

If it requires just vcg subdirectory indeed, maybe include path might be omitted and install it just directly into /usr/include/vcg. Then pkgconfig would not be required for building, it would work automatically. I think that would be best option here.

Upstream should be asked to correct it in next release, please fill issue on their github.

Also, since this library is header-only, it has to provide vcglib-static in devel subpackage [1].

In devel subpackage, only headers required to build application should be used. It seems to me vcg subdirectory is the only one belonging there, maybe also img. Please do NOT install eigenlib at all.

Apps should be packaged in separate utils subpackage. If you insist they are not to be compiled and packaged, just do not include their headers and other files. Removing just *.cpp files is not enough. Either compile them and install compiled binaries or do not install it at all. They would be already part of source package, there is no point installing all sources as part of normal package. Headers in devel package used by other packages are exception to this. Binary rpm should contain prebuilt executables and scripts that do not require compilation, or data.

1. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_packaging_header_only_libraries
I think at least some of apps should be compiled and installed, but would accept if you want this for wdune dependency only.

Comment 13 Petr Menšík 2019-03-13 12:39:00 UTC
One more thing, devel subpackage does not contain license. That is ok, but it must depend on package that contains it.
%package devel must include:
Requires: %{name}%{?_isa} = %{name}-%{version}

Comment 14 J. Scheurich 2019-03-14 10:28:12 UTC
Spec URL: ftp://ftp.ourproject.org/pub/wdune/vcglib.spec
SRPM URL: ftp://ftp.ourproject.org/pub/wdune/vcglib-1.0.1-1.src.rpm
Description: The Visualization and Computer Graphics Library (VCG for short) 
             is a open source portable C++ templated library for manipulation,
             processing and displaying with OpenGL of triangle and tetrahedral
             meshes.
Fedora Account System Username: muftii

Comment 15 Petr Menšík 2019-03-15 13:55:05 UTC
1) 
Requires: %{name} is not enough. It has to be versioned, must depend on same. And depend also on the same architecture. Like I mentioned in comment #13.

2)
eigenlib prefix from includes has to be removed in installed headers. Place something like this into %prep:
And add BuildRequires: sed

find vcg img wrap -name '*.h' -exec sed -i '{}' \
        -e '/^#include/ s/<eigenlib\//</' \
        -e '/^#include/ s,"../../eigenlib/\(.*\)",<\1>,' \
        ';'

3)
Ok, unfortunately wrap subdirectory is required, cannot be omitted. Contains headers used by other vcg headers.
What is worse, wrap contains code in cpp files that should be compiled into shared library. But no single project file is prepared for real library.

I think until full featured package is prepared, we can copy just that part of wrap, that does not require compilation. Wdune seems to link with it just with fine.
I think these parts are required:

mkdir -p %{buildroot}%{_includedir}/%{name}/wrap
cp -a wrap/*.h wrap/{gcache,gl,glw,igl,io_edgemesh,io_tetramesh,io_trimesh,math,minpack,mt,opensg,system} %{buildroot}%{_includedir}/%{name}/wrap

Mention why that was required in a comment

Comment 16 J. Scheurich 2019-03-15 23:14:50 UTC
Spec URL: ftp://ftp.ourproject.org/pub/wdune/vcglib.spec
SRPM URL: ftp://ftp.ourproject.org/pub/wdune/vcglib-1.0.1-1.src.rpm
Description: The Visualization and Computer Graphics Library (VCG for short) 
             is a open source portable C++ templated library for manipulation,
             processing and displaying with OpenGL of triangle and tetrahedral
             meshes.
Fedora Account System Username: muftii

> 2)
> eigenlib prefix from includes has to be removed in installed headers. Place
> something like this into %prep:
> And add BuildRequires: sed
> 
> find vcg img wrap -name '*.h' -exec sed -i '{}' \
>         -e '/^#include/ s/<eigenlib\//</' \
>         -e '/^#include/ s,"../../eigenlib/\(.*\)",<\1>,' \
>         ';'

To use Eigen3 this headers needs to use "eigen3" instead of "eigenlib"

Comment 17 Petr Menšík 2019-03-18 09:21:44 UTC
No! No prefix of such kind is necessary here, on the contrary.

If you check what flags are supplied by pkg-config vcglib, which we have now installed:

$ pkg-config --cflags vcglib
-I/usr/include/vcglib -I/usr/include/eigen3

It configures flags of compiler to look into vcglib and eigen3 subdirectories in /usr/include. That means you can just use #include <Eigen/Eigen>, and it would find header file located in /usr/include/eigen3/Eigen/Eigen. I think that is how authors of Eigen library want it to be used [1]. Similar way with #include <vcg/complex/complex.h>. Just tell it where to look for headers if they are not directly under /usr/include. If someone wanted to still use bundled eigenlib, he has to only add -Ieigenlib to compiler flags, no other change of source files would be required.  I am sure we do not want to prefix includes in header files with unnecessary modifications, especially if they are not required with pkg-config usage. I think CPPFLAGS="-I/usr/include/eigen3" are user-friendly enough even without pkg-config.

Please modify headers in %prep, maybe in %build. %install should only install already produced files from previous phases into target directories of the system.
"fedpkg --release master prep" or "fedpkg --release master compile" commands should prepare ready-to-use local compilation of package, that can be used in source directory.

It is possible to use such directory for --with-vcglib=... from wdune this way. It is handy for testing any modifications before you have to install produced package.

1. https://eigen.tuxfamily.org/dox-devel/GettingStarted.html

Comment 18 J. Scheurich 2019-03-18 13:19:56 UTC
Spec URL: ftp://ftp.ourproject.org/pub/wdune/vcglib.spec
SRPM URL: ftp://ftp.ourproject.org/pub/wdune/vcglib-1.0.1-1.src.rpm
Description: The Visualization and Computer Graphics Library (VCG for short) 
             is a open source portable C++ templated library for manipulation,
             processing and displaying with OpenGL of triangle and tetrahedral
             meshes.
Fedora Account System Username: muftii

Comment 19 Petr Menšík 2019-03-18 17:37:13 UTC
Almost there. License tag is wrong, most headers have GPLv2 or later, while some files have no license tags. Since there is conflict between main license file and file headers, both must be declared in License tag.

License: GPLv2+ and GPLv3+

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

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


Issues:
=======
- All build dependencies are listed in BuildRequires, except for any that
  are listed in the exceptions section of Packaging Guidelines.
  Note: These BR are not needed: sed
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2


===== 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]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

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)", "Unknown or generated", "GPL (v2 or later)
     GNU Lesser General Public License", "BSD 3-clause "New" or "Revised"
     License", "GPL (v3 or later) GNU Lesser General Public License (v3 or
     later)", "GNU Lesser General Public License (v2.1 or later)", "Boehm
     GC License Mozilla Public License (v2.0)", "*No copyright* Mozilla
     Public License (v2.0)", "*No copyright* GNU General Public License",
     "BSD 2-clause "Simplified" License", "GPL (v2 or later) (with
     incorrect FSF address)", "GNU Lesser General Public License (v2.1)",
     "Mozilla Public License (v2.0)", "GNU Lesser General Public License
     (modified-code-notice clause) GNU Lesser General Public License (v2.1
     or later)", "GPL (v3 or later)". 455 files have unknown license.
     Detailed output of licensecheck in /home/reviewer/fedora/rawhide
     /review-vcglib/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: %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
[x]: 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.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[-]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 10240 bytes in 1 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[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:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in vcglib-
     devel
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: 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.
[ ]: Packages should try to preserve timestamps of original installed
     files.
[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]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

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

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[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.


Rpmlint
-------
Checking: vcglib-1.0.1-1.fc30.x86_64.rpm
          vcglib-devel-1.0.1-1.fc30.x86_64.rpm
          vcglib-1.0.1-1.fc30.src.rpm
vcglib.x86_64: W: spelling-error %description -l en_US templated -> templates, template, template d
vcglib.x86_64: W: incoherent-version-in-changelog 1.0.1 ['1.0.1-1.fc30', '1.0.1-1']
vcglib.x86_64: E: no-binary
vcglib-devel.x86_64: W: no-documentation
vcglib-devel.x86_64: E: incorrect-fsf-address /usr/include/vcglib/wrap/system/getopt.cpp
vcglib-devel.x86_64: E: incorrect-fsf-address /usr/include/vcglib/wrap/system/getopt.h
vcglib.src: W: spelling-error %description -l en_US templated -> templates, template, template d
3 packages and 0 specfiles checked; 3 errors, 4 warnings.

Requires
--------
vcglib-devel (rpmlib, GLIBC filtered):
    /usr/bin/pkg-config
    pkgconfig(eigen3)
    vcglib(x86-64)

vcglib (rpmlib, GLIBC filtered):
    /usr/bin/pkg-config
    eigen3



Provides
--------
vcglib-devel:
    pkgconfig(vcglib)
    vcglib-devel
    vcglib-devel(x86-64)
    vcglib-static

vcglib:
    vcglib
    vcglib(x86-64)



Source checksums
----------------
https://github.com/cnr-isti-vclab/vcglib/archive/v1.0.1.tar.gz :
  CHECKSUM(SHA256) this package     : 406e570637d3820810bf85fea7d8138ab8c70dfcb1be9941a0994ab3a793d1d0
  CHECKSUM(SHA256) upstream package : 406e570637d3820810bf85fea7d8138ab8c70dfcb1be9941a0994ab3a793d1d0


Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02

Comment 20 J. Scheurich 2019-03-20 02:05:30 UTC
(In reply to Petr Menšík from comment #19)
> Rpmlint
> -------
...
> vcglib-devel.x86_64: E: incorrect-fsf-address
> /usr/include/vcglib/wrap/system/getopt.cpp
> vcglib-devel.x86_64: E: incorrect-fsf-address
> /usr/include/vcglib/wrap/system/getopt.h
> vcglib.src: W: spelling-error %description -l en_US templated -> templates,
> template, template d
> 3 packages and 0 specfiles checked; 3 errors, 4 warnings.

Is there soemthing is could do with the getopt problem ?
It looks like vcglib shios its own version of getopt 8-(

Comment 21 Petr Menšík 2019-03-24 19:24:21 UTC
(In reply to J. Scheurich from comment #20)
> (In reply to Petr Menšík from comment #19)
> Is there soemthing is could do with the getopt problem ?
> It looks like vcglib shios its own version of getopt 8-(

Oh, did not check what getopt it is. Thought it would be some local extension. If it is just normal getopt, do not install its header or cpp file. Any cpp file should be part of devel package anyway.
I think it is not used anywhere. If it is used, should be replaced with system version provided by glibc-headers.

Check:
dnf repoquery --whatprovides /usr/include/getopt.h

Comment 22 Petr Menšík 2019-03-24 20:29:57 UTC
Any cpp file should NOT be part of devel package anyway, I had on mind.

If getopt is nowhere used, just delete wrap/system/getopt.* wrap/system/qgetopt.* in prep or build.

I think if you add GPLv2+ to License for main package and sub package, it is ready to be accepted.

Comment 23 Petr Menšík 2019-03-24 21:00:55 UTC
Oh, noticed just now. Thank you for moving to build phase. But %{build} is not defined in such phase, nor is %{buildroot}. It should just work relative to current directory.

find "%{build}%{_includedir}/%{name}/"$i -name '*.cpp' -> find $i -name '*.cpp'
etc.

But cp -a usually has to be done in %install after that, when %{buildroot} is already defined. Some mkdirs have to be moved there also.
It does not complain, but creates unexpanded '%{build}' directory

Comment 24 J. Scheurich 2019-03-26 03:50:27 UTC
pec URL: ftp://ftp.ourproject.org/pub/wdune/vcglib.spec
SRPM URL: ftp://ftp.ourproject.org/pub/wdune/vcglib-1.0.1-1.src.rpm
Description: The Visualization and Computer Graphics Library (VCG for short) 
             is a open source portable C++ templated library for manipulation,
             processing and displaying with OpenGL of triangle and tetrahedral
             meshes.
Fedora Account System Username: muftii

Added GPLv2+ License

Comment 25 Petr Menšík 2019-03-31 18:53:52 UTC
Please remove %{build}%{_includedir} and %{build}%{_datarootdir} from both build and install sections.

rpm -E '%{build}' is not defined and expands just to the same text. That is wrong.

Use just pkgconfig/vcglib.pc. And use %{buildroot} in %install, where that variable is already defined. %build is not defined in any section.

Comment 26 J. Scheurich 2019-04-04 04:49:18 UTC
Spec URL: ftp://ftp.ourproject.org/pub/wdune/vcglib.spec
SRPM URL: ftp://ftp.ourproject.org/pub/wdune/vcglib-1.0.1-1.src.rpm
Description: The Visualization and Computer Graphics Library (VCG for short) 
             is a open source portable C++ templated library for manipulation,
             processing and displaying with OpenGL of triangle and tetrahedral
             meshes.
Fedora Account System Username: muftii

Comment 27 J. Scheurich 2019-04-04 07:46:38 UTC
pec URL: ftp://ftp.ourproject.org/pub/wdune/vcglib.spec
SRPM URL: ftp://ftp.ourproject.org/pub/wdune/vcglib-1.0.1-1.src.rpm
Description: The Visualization and Computer Graphics Library (VCG for short) 
             is a open source portable C++ templated library for manipulation,
             processing and displaying with OpenGL of triangle and tetrahedral
             meshes.
Fedora Account System Username: muftii

Updated specfile

Comment 28 Petr Menšík 2019-04-08 09:47:07 UTC
(In reply to J. Scheurich from comment #27)
> 
> Updated specfile

Please!

in %build section remove lines:
mkdir -p %{buildroot}%{_includedir}/%{name}/wrap
cp -r vcg img %{buildroot}%{_includedir}/%{name}

mkdir -p %{buildroot}%{_includedir}/%{name}/wrap
# wrap contains code in cpp files that should be compiled into a shared library
cp -a wrap/*.h wrap/{gcache,gl,glw,igl,io_edgemesh,io_tetramesh,io_trimesh,math,minpack,mt,opensg,system} \
   %{buildroot}%{_includedir}/%{name}/wrap


in %build section change this:
rm %{buildroot}%{_includedir}/%{name}/wrap/system/getopt.h

for i in vcg img ; do
   find %{buildroot}%{_includedir}/%{name} -name '*.cpp' | xargs -r rm
done
just to:
find vcg img -name '*.cpp' | xargs -r rm


Add into beginning of %install section:
mkdir -p %{buildroot}%{_includedir}/%{name}/wrap
cp -r vcg img %{buildroot}%{_includedir}/%{name}

cp -a wrap/*.h wrap/{gcache,gl,glw,igl,io_edgemesh,io_tetramesh,io_trimesh,math,minpack,mt,opensg,system} \
   %{buildroot}%{_includedir}/%{name}/wrap
rm %{buildroot}%{_includedir}/%{name}/wrap/system/getopt.*

Comment 29 J. Scheurich 2019-04-08 13:12:40 UTC
Spec URL: ftp://ftp.ourproject.org/pub/wdune/vcglib.spec
SRPM URL: ftp://ftp.ourproject.org/pub/wdune/vcglib-1.0.1-1.fc29.src.rpm
Description: The Visualization and Computer Graphics Library (VCG for short) 
             is a open source portable C++ templated library for manipulation,
             processing and displaying with OpenGL of triangle and tetrahedral
             meshes.
Fedora Account System Username: muftii

Comment 30 Petr Menšík 2019-04-09 06:22:32 UTC
Thanks. However, there is still copy of all wrap into target directory, copying there all cpp files not required.

Please remove these lines on line 44-45:
mkdir -p %{buildroot}%{_includedir}/%{name}
cp -a  vcg img wrap %{buildroot}%{_includedir}/%{name}

and change:
rm %{buildroot}%{_includedir}/%{name}/wrap/system/getopt.*
to:
rm %{buildroot}%{_includedir}/%{name}/wrap/system/{getopt,qgetopt}.*

You can test it yourself:
$ fedpkg --release f29 local
$ rpm -qpl x86_64/vcglib-devel*.rpm | grep '\.cpp' | wc -l

Should return 0 in devel package.

To simplify future upgrades, Source URL should contain %{version} instead of version in url:
Source: https://github.com/cnr-isti-vclab/vcglib/archive/v%{version}.tar.gz

That should be all required for this package.

Comment 31 J. Scheurich 2019-04-10 14:16:54 UTC
Spec URL: ftp://ftp.ourproject.org/pub/wdune/vcglib.spec
SRPM URL: ftp://ftp.ourproject.org/pub/wdune/vcglib-1.0.1-1.fc29.src.rpm
Description: The Visualization and Computer Graphics Library (VCG for short) 
             is a open source portable C++ templated library for manipulation,
             processing and displaying with OpenGL of triangle and tetrahedral
             meshes.
Fedora Account System Username: muftii

Comment 32 Petr Menšík 2019-04-10 21:02:49 UTC
Thank you! I think this package is ready.

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

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


Issues:
=======
- Sources used to build the package match the upstream source, as provided
  in the spec URL.
  Note: Upstream MD5sum check error, diff is in
  /home/reviewer/reviews/1677989-vcglib.progress/srpm/review-
  vcglib/diff.txt
  See: http://fedoraproject.org/wiki/Packaging/SourceURL
- All build dependencies are listed in BuildRequires, except for any that
  are listed in the exceptions section of Packaging Guidelines.
  Note: These BR are not needed: sed
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2


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

C/C++:
[-]: Provides: bundled(gnulib) in place as required.
     Note: Sources not installed
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Header files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

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.
[x]: License field in the package spec file matches the actual license.
     Note: There is no build directory. Running licensecheck on vanilla
     upstream sources. Licenses found: "GPL (v2 or later)", "Unknown or
     generated", "GPL (v2 or later) GNU Lesser General Public License",
     "BSD 3-clause "New" or "Revised" License", "GPL (v3 or later) GNU
     Lesser General Public License (v3 or later)", "GNU Lesser General
     Public License (v2.1 or later)", "Boehm GC License Mozilla Public
     License (v2.0)", "*No copyright* Mozilla Public License (v2.0)", "*No
     copyright* GNU General Public License", "BSD 2-clause "Simplified"
     License", "GPL (v2 or later) (with incorrect FSF address)", "GNU
     Lesser General Public License (v2.1)", "Mozilla Public License
     (v2.0)", "GNU Lesser General Public License (modified-code-notice
     clause) GNU Lesser General Public License (v2.1 or later)", "GPL (v3
     or later)". 455 files have unknown license. Detailed output of
     licensecheck in /home/reviewer/reviews/1677989-vcglib.progress/srpm
     /review-vcglib/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.
[x]: %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.
[x]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: 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.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[-]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 10240 bytes in 1 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[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]: 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:
[x]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[x]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in vcglib-
     devel
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: 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.
[?]: Packages should try to preserve timestamps of original installed
     files.
[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]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

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

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[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.


Rpmlint
-------
Checking: vcglib-1.0.1-1.fc29.x86_64.rpm
          vcglib-devel-1.0.1-1.fc29.x86_64.rpm
          vcglib-1.0.1-1.fc29.src.rpm
vcglib.x86_64: E: devel-dependency glibc-headers
vcglib.x86_64: W: spelling-error %description -l en_US templated -> templates, template, template d
vcglib.x86_64: W: incoherent-version-in-changelog 1.0.1 ['1.0.1-1.fc29', '1.0.1-1']
vcglib.x86_64: E: no-binary
vcglib-devel.x86_64: W: no-documentation
vcglib.src: W: spelling-error %description -l en_US templated -> templates, template, template d
3 packages and 0 specfiles checked; 2 errors, 4 warnings.




Rpmlint (installed packages)
----------------------------
sh: /usr/bin/python: No such file or directory
vcglib-devel.x86_64: W: invalid-url URL: http://vcg.isti.cnr.it/vcglib/ <urlopen error [Errno -2] Name or service not known>
vcglib-devel.x86_64: W: no-documentation
vcglib.x86_64: E: devel-dependency glibc-headers
vcglib.x86_64: W: spelling-error %description -l en_US templated -> templates, template, template d
vcglib.x86_64: W: incoherent-version-in-changelog 1.0.1 ['1.0.1-1.fc29', '1.0.1-1']
vcglib.x86_64: W: invalid-url URL: http://vcg.isti.cnr.it/vcglib/ <urlopen error [Errno -2] Name or service not known>
vcglib.x86_64: E: no-binary
2 packages and 0 specfiles checked; 2 errors, 5 warnings.



Requires
--------
vcglib-devel (rpmlib, GLIBC filtered):
    /usr/bin/pkg-config
    pkgconfig(eigen3)
    vcglib(x86-64)

vcglib (rpmlib, GLIBC filtered):
    /usr/bin/pkg-config
    eigen3
    glibc-headers



Provides
--------
vcglib-devel:
    pkgconfig(vcglib)
    vcglib-devel
    vcglib-devel(x86-64)
    vcglib-static

vcglib:
    vcglib
    vcglib(x86-64)



Source checksums
----------------
https://github.com/cnr-isti-vclab/vcglib/archive/v1.0.1.tar.gz :
  CHECKSUM(SHA256) this package     : d32aaad715e7c9e9647310b32431a91dbfe0fd3e21e2e35cbd88d2893c91dec6
  CHECKSUM(SHA256) upstream package : 406e570637d3820810bf85fea7d8138ab8c70dfcb1be9941a0994ab3a793d1d0
diff -r also reports differences


Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02
Command line :/usr/bin/fedora-review -n vcglib
Buildroot used: fedora-29-x86_64
Active plugins: Generic, Shell-api, C/C++
Disabled plugins: Java, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP
Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6

Comment 33 J. Scheurich 2019-04-13 09:33:44 UTC
Spec URL: ftp://ftp.ourproject.org/pub/wdune/vcglib.spec
SRPM URL: ftp://ftp.ourproject.org/pub/wdune/vcglib-1.0.1-1.fc29.src.rpm
Description: The Visualization and Computer Graphics Library (VCG for short) 
             is a open source portable C++ templated library for manipulation,
             processing and displaying with OpenGL of triangle and tetrahedral
             meshes.
Fedora Account System Username: muftii


The following has been lost from %build:

for i in `find vcg img wrap -type f`; do
  sed -i 's,eigenlib\/,eigen3\/,' $i
done

Now it is included

Comment 34 J. Scheurich 2019-10-26 14:20:06 UTC
Fedora Account System Username: muftii

Sorry, typo:

Fedora Account System Username: mufti11

Comment 35 Ankur Sinha (FranciscoD) 2019-11-09 18:18:12 UTC
Resetting flags

Comment 36 Robert-André Mauchin 2019-11-19 16:07:43 UTC
Revalidating.


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