Bug 1244353 - Review Request: ossim - Open Source Software Image Map
Summary: Review Request: ossim - Open Source Software Image Map
Keywords:
Status: NEW
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2015-07-17 21:48 UTC by Rashad M
Modified: 2017-08-27 20:19 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed:


Attachments (Terms of Use)

Description Rashad M 2015-07-17 21:48:31 UTC
This is reopening of  #478722 to add ossim packages.

I want to submit ossim package to official fedora. currently I had hosted its source rpms here - https://www.orfeo-toolbox.org/srpms/ossim-1.8.18-2.fc20.src.rpm

The package is also built on Fedora Copr for Fedora 20 and Fedora 21
https://www.orfeo-toolbox.org/srpms/ossim-1.8.18-2.fc20.src.rpm

https://copr.fedoraproject.org/coprs/orfeotoolbox/otb/monitor/

Comment 1 Marcin Haba 2015-07-19 10:58:31 UTC
Hello,

It is informal review because I am not Fedora packages maintainer.

Here are a few comments:

1) No Spec URL attached to this bugzilla request. Please past in new comment Spec and SRPM.

2) Source0 is not possible to download:

http://download.osgeo.org/ossim/source/ossim-1.8.18-1/ossim-1.8.18-1.tar.gz

ERROR 404: Not Found.

3) rpmlint on source package shows:

ossim.spec:154: W: mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line 154)

4) In Spec is a couple of commented BuildRequires

$ grep '^#BuildRequires' ossim.spec 
#BuildRequires: ant
#BuildRequires: gdal-devel
#BuildRequires: hdf-devel
#BuildRequires: hdf5-devel
#BuildRequires: java-devel
#BuildRequires: minizip-devel
#BuildRequires: OpenSceneGraph-devel
#BuildRequires: podofo-devel
#BuildRequires: qt4-devel
#BuildRequires: swig

If there not exist any special reason to include these commented requirements I think that better might be not provide these comments because they make the Spec obfuscated.

5) License files should not be handled in %doc macro but in %license macro.
https://fedoraproject.org/wiki/Packaging:Guidelines#Documentation

Comment 2 Rashad M 2015-07-20 08:52:59 UTC
Thanks for your review.


Here is the spec file hosted on our git https://git.orfeo-toolbox.org/otb-devutils.git/blob/HEAD:/Packaging/fedora/SPECS/ossim.spec


I had updated spec as per your suggestions.

Comment 3 Marcin Haba 2015-07-20 16:11:59 UTC
Hello,

Thanks for your changes in Spec.

Could I ask you about provide spec file in plan text form togeter with source rpm in lines like below:

Spec URL: <url-to-spec-in-text-format.spec>
SRPM URL: <url-to-srpm-available-to-download.src.rpm>

I am asking about it because fedora-review tool is able to work with specs in text format, not with HTML git pages. This tool downloads files directly from links pasted here in bugzilla.redhat.com and then performs tests and prepares review check list form.

Thanks in advance.

Comment 4 Rashad M 2015-07-24 22:38:13 UTC
Sorry for the delay. Here is the source and spec.



Spec URL: https://git.orfeo-toolbox.org/otb-devutils.git/blob_plain/HEAD:/Packaging/fedora/SPECS/ossim.spec

SRPM URL: http://www.filedropper.com/ossim-1818-2fc20src

Comment 5 Marcin Haba 2015-07-25 12:38:38 UTC
Hello,

Thanks for provided Spec and SRPM URLs.

Minor note to SRPM URL. It indicates to service which needs to type captcha for download SRPM. For future SRPM URLs nicer could be provide direct link to SRPM. If you do not have any place to put SRPM, you can use https://fedorapeople.org/ service for that.

OK. Comming back to review, I successfully built your packages.

Rpmlint tool is showing a couple of errors and warnings on your packages.

Here are errors:
ossim-devel.x86_64: E: incorrect-fsf-address /usr/include/ossim/vpfutil/values.h
ossim-devel.x86_64: E: non-devel-file-in-devel-package /usr/lib64/libossim.so.1.8.18
ossim-devel.x86_64: E: library-without-ldconfig-postin /usr/lib64/libossim.so.1.8.18
ossim-devel.x86_64: E: library-without-ldconfig-postun /usr/lib64/libossim.so.1.8.18
ossim-debuginfo.x86_64: E: wrong-script-end-of-line-encoding /usr/src/debug/ossim-1.8.18/ossim/src/ossim/base/ossimAdjSolutionAttributes.cpp
ossim-debuginfo.x86_64: E: wrong-script-end-of-line-encoding /usr/src/debug/ossim-1.8.18/ossim/include/ossim/base/ossimGeodeticEvaluator.h
ossim-debuginfo.x86_64: E: wrong-script-end-of-line-encoding /usr/src/debug/ossim-1.8.18/ossim/include/ossim/base/ossimAdjSolutionAttributes.h
ossim-debuginfo.x86_64: E: wrong-script-end-of-line-encoding /usr/src/debug/ossim-1.8.18/ossim/src/ossim/base/ossimAdjSolutionAttributes.cpp
ossim-debuginfo.x86_64: E: wrong-script-end-of-line-encoding /usr/src/debug/ossim-1.8.18/ossim/include/ossim/base/ossimGeodeticEvaluator.h
ossim-debuginfo.x86_64: E: wrong-script-end-of-line-encoding /usr/src/debug/ossim-1.8.18/ossim/include/ossim/base/ossimAdjSolutionAttributes.h
ossim-devel.x86_64: E: incorrect-fsf-address /usr/include/ossim/vpfutil/values.h
ossim-devel.x86_64: E: library-without-ldconfig-postin /usr/lib64/libossim.so.1.8.18
ossim-devel.x86_64: E: library-without-ldconfig-postun /usr/lib64/libossim.so.1.8.18

Here are warnings:
ossim.x86_64: W: shared-lib-calls-exit /usr/lib64/libossim.so.1.8.18 exit@GLIBC_2.2.5
ossim.x86_64: W: no-documentation
ossim-devel.x86_64: W: shared-lib-calls-exit /usr/lib64/libossim.so.1.8.18 exit@GLIBC_2.2.5
ossim-devel.x86_64: W: no-documentation
ossim-data.x86_64: W: no-documentation
ossim-data.x86_64: W: non-conffile-in-etc /etc/profile.d/ossim.sh
ossim-data.x86_64: W: non-conffile-in-etc /etc/profile.d/ossim.csh
ossim.src:66: W: macro-in-comment %setup
ossim.src:141: W: mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line 141)
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/support_data/ossimNitfDataExtensionSegmentV2_1.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/src/ossim/base/ossimAdjSolutionAttributes.cpp
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/base/ossimBinaryDataProperty.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/support_data/ossimNitfImageDataMaskV2_1.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/src/ossim/support_data/ossimNitfDataExtensionSegmentV2_1.cpp
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/base/ossimGeodeticEvaluator.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/base/ossimAdjSolutionAttributes.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/src/ossim/support_data/ossimNitfImageDataMaskV2_1.cpp
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/src/ossim/base/ossimBinaryDataProperty.cpp
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/support_data/ossimNitfDataExtensionSegmentV2_1.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/src/ossim/base/ossimAdjSolutionAttributes.cpp
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/base/ossimBinaryDataProperty.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/support_data/ossimNitfImageDataMaskV2_1.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/src/ossim/support_data/ossimNitfDataExtensionSegmentV2_1.cpp
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/base/ossimGeodeticEvaluator.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/base/ossimAdjSolutionAttributes.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/src/ossim/support_data/ossimNitfImageDataMaskV2_1.cpp
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/src/ossim/base/ossimBinaryDataProperty.cpp
ossim.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libossim.so.1.8.18 /lib64/libgeos_c.so.1
ossim.x86_64: W: shared-lib-calls-exit /usr/lib64/libossim.so.1.8.18 exit@GLIBC_2.2.5
ossim.x86_64: W: no-documentation
ossim-devel.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libossim.so.1.8.18 /lib64/libgeos_c.so.1
ossim-devel.x86_64: W: shared-lib-calls-exit /usr/lib64/libossim.so.1.8.18 exit@GLIBC_2.2.5
ossim-devel.x86_64: W: no-documentation
ossim-data.x86_64: W: no-documentation
ossim-data.x86_64: W: non-conffile-in-etc /etc/profile.d/ossim.sh
ossim-data.x86_64: W: non-conffile-in-etc /etc/profile.d/ossim.csh

For knowing what does every error mean you can use rpmlint command with -I switcher, for example:

$ rpmlint -I library-without-ldconfig-postin
library-without-ldconfig-postin:
This package contains a library and provides no %post scriptlet containing a
call to ldconfig.

Comment 6 Rashad M 2015-08-01 15:01:05 UTC
Hello,
Can you help me with some rpmlint errors such as below

non-conffile-in-etc /etc/profile.d/ossim.sh
ossim-devel.x86_64: E: non-devel-file-in-devel-package 
E: library-without-ldconfig-postin

but I have 
%post -n ossim -p /sbin/ldconfig

%postun -n ossim -p /sbin/ldconfig

in the spec.

regarding execute perm warning. I had update the spec file


Also I dont how to host srpm on fedorapeople. is there anything else I need apart from Fedora account?

Comment 7 Marcin Haba 2015-08-01 18:13:48 UTC
Hello Rashad,

Thanks for the update in this task.

About errors:

1) ossim-data.x86_64: W: non-conffile-in-etc /etc/profile.d/ossim.sh

It looks that you can keep it as is now, because it is profile.d  shell type file, not config file. For be 100% sure, I will ask somebody from Fedora packages maintainers, and then let you know. If is need then I prepare patch for rpmlint and send the patch to rpmlint author.

2) ossim-devel.x86_64: E: non-devel-file-in-devel-package /usr/lib64/libossim.so.1.8.18

Because this dynamically linked library is used by users programs, for example:

$ ldd /usr/bin/ossim-cmm 
	linux-vdso.so.1 (0x00007ffcbdb7a000)
	libossim.so.1 => /lib64/libossim.so.1 (0x00007f2183d00000)        <==== here
	libOpenThreads.so.20 => /lib64/libOpenThreads.so.20 (0x00007f2183af8000)
	libgeos-3.4.2.so => /lib64/libgeos-3.4.2.so (0x00007f2183750000)
	libgeos_c.so.1 => /lib64/libgeos_c.so.1 (0x00007f2183528000)
	libgeotiff.so.2 => /lib64/libgeotiff.so.2 (0x00007f21832f0000)
	libjpeg.so.62 => /lib64/libjpeg.so.62 (0x00007f2183090000)
	libtiff.so.5 => /lib64/libtiff.so.5 (0x00007f2182e18000)
	libdl.so.2 => /lib64/libdl.so.2 (0x00007f2182c10000)
	libz.so.1 => /lib64/libz.so.1 (0x00007f21829f8000)
	libstdc++.so.6 => /lib64/libstdc++.so.6 (0x00007f2182670000)
	libm.so.6 => /lib64/libm.so.6 (0x00007f2182368000)
	libgcc_s.so.1 => /lib64/libgcc_s.so.1 (0x00007f2182150000)
	libc.so.6 => /lib64/libc.so.6 (0x00007f2181d90000)
	libpthread.so.0 => /lib64/libpthread.so.0 (0x00007f2181b70000)
	libproj.so.0 => /lib64/libproj.so.0 (0x00007f2181920000)
	libjbig.so.2.1 => /lib64/libjbig.so.2.1 (0x00007f2181710000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f2184f00000)

I guess that it is not devel type file. The Packaging Guidelines says here:

https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#DevelPackages

that there is possible to include in -devel packages only reasonable selected shared libraries. As you will see in above guidelines, one from general advises is to determine for which purposes is used library: for a developer or for users. If it is used for development then a file needs to be moved to -devel package, if for user applications, then a file needs to be moved to base package.

Also the library file is already included in two packages:

- ossim-1.8.18-2.fc21.x86_64.rpm
- ossim-devel-1.8.18-2.fc21.x86_64.rpm

so it causes second problem - doubled the same library in two different RPM packages.

If you decide that the library is used for users application, not for development, I propose to not include this library in devel package, and then both above issues (rpmlint error and doubled file) become fixed.

3) ossim-devel.x86_64: E: library-without-ldconfig-postin /usr/lib64/libossim.so.1.8.18

I also have no idea why the error occurs, because it is compatible with Guidelines:

https://fedoraproject.org/wiki/Packaging:Guidelines?rd=Packaging/Guidelines#Shared_Libraries

However the error appears only on -devel package, so I guess that it will be fixed when you do not include library in -devel package.


If you want add to this task your SRPM file via FedoraPeople service then you can follow on this link:

https://fedoraproject.org/wiki/Infrastructure/fedorapeople.org#Accessing_Your_fedorapeople.org_Space

Here is described what you need to open FedoraPeople account.

I hope that it helps.

Comment 8 Marcin Haba 2015-08-02 05:58:59 UTC
I asked on fedora-review IRC channel about this warning: "non-conffile-in-etc /etc/profile.d/ossim.sh".

The answer is that in this case the warning is acceptable.

Comment 9 Eduardo Mayorga 2015-08-02 06:18:02 UTC
(In reply to Marcin Haba from comment #8)
> I asked on fedora-review IRC channel about this warning:
> "non-conffile-in-etc /etc/profile.d/ossim.sh".
> 
> The answer is that in this case the warning is acceptable.

You need to mark the file as a config file by using the %config macro.
See: https://fedoraproject.org/wiki/Common_Rpmlint_issues#non-conffile-in-etc

Comment 10 Marcin Haba 2015-08-02 06:39:09 UTC
Hello Eduardo,

Thanks for your advise.

My question on fedora-review channel was:

"I am doing informal review (bugzilla 1244353). Rpmlint returns: W: non-conffile-in-etc /etc/profile.d/ossim.sh. Because it is profile.d/ file, it is obvious that ossim.sh is not configuration file but a script. Is the warning acceptable in this case, or ossim.sh should be added with %config macro or there exists another way to solve this issue"

I am not sure about %config macro, because ossim.sh in shell script (as usual in profile.d/), not configuration file.

Because I received two different answers for my question, I will send this subject to fedora-devel mailing list.

Comment 11 Marcin Haba 2015-08-02 07:52:34 UTC
Hello,

@Rashad:
In case this warning:

ossim-data.x86_64: W: non-conffile-in-etc /etc/profile.d/ossim.sh

please follow on answers written on this thread:

https://lists.fedoraproject.org/pipermail/devel/2015-August/213027.html

Thanks.

Comment 12 Marcin Haba 2015-08-02 08:06:25 UTC
@Eduardo,

It looks that you are right. Thanks for indicate me this guidelines note about the warning.

Comment 13 Michael Schwendt 2015-08-02 11:20:20 UTC
> 3) ossim-devel.x86_64: E: library-without-ldconfig-postin /usr/lib64/libossim.so.1.8.18
> 
> I also have no idea why the error occurs,

It's an error, because the files are misplaced. Let's take a look:

| %files
| %{_libdir}/libossim.so.*
| 
| %files devel
| %{_includedir}/ossim*
| %{_libdir}/*.so*

First of all, the wildcards "libossim.so.*" and "*.so*" overlap. The latter also includes the former, i.e. "*.so*" also matches files matched by "*.so.*" and hence "libossim.so.*", too.

Since you duplicate shared libs in two packages, you probably don't see any symptoms at runtime, but have you examined the package dependencies yet? Run "rpm -qp --whatrequires …" and "rpm -qp --whatprovides …" on the built binary packages. That's important for understanding RPM package dependencies.

Secondly, the important thing about placement of .so files is to decide when they are needed. At runtime? Or at buildtime only? Or at runtime _and_ at buildtime?

libossim.so.1.8.18 is a runtime library to be put into the base package. Same for libossim.so.1 and further versions that result in _automatic_ dependencies in your packages.

On the contrary, an unversioned symlink libossim.so is what makes linking at buildtime work. Without libossim.so the compiler would not find the library when given the -lossim linker option.

[Finally, the third case is rare. A program doing a dlopen() call (or something similar) to open an unversioned shared library at runtime. Such a case can be nasty, because there are no automatic RPM dependencies you can rely on, and if the unversioned .so file is shipped in a -devel package, this can get ugly if the library is needed at runtime always.]


| %post -n ossim -p /sbin/ldconfig
| 
| %postun -n ossim -p /sbin/ldconfig

Now back to the error. You execute ldconfig for the base "ossim" package only, but rpmlint complains about the -devel subpackage (!) where you include runtime libraries, too. The fix would be to not include any runtime libs in the -devel package, and then you don't need to run ldconfig for it either (which would be the normal case, btw).

The explicit "-n ossim" is not needed, btw. The base package name is the default for scriptlet sections lacking an -n option.

[...]

The package needs quite a bit of more work. These are just some drive-by comments.


Try pointing the fedora-review tool at this ticket:

  fedora-review -b 1244353

It will download the latest spec file and src.rpm from the "Spec URL:" and "SRPM URL:" lines you include in your comments and perform many helpful checks.


There is the helpful %{_fixperms} macro for correcting permissions with the source tree:

  $ rpm -E %_fixperms
  /usr/bin/chmod -Rf a+rX,u+w,g-w,o-w


> %package 	apps
> Summary:        %{sname} applications
> Group:          System Environment/Libraries
> Requires:       %{name}%{?_isa} = %{version}-%{release}

An unusual Group tag for files in %_bindir. The tag is optional nowadays, so you may want to remove it everywhere from the spec file.


> %package	    doc
> Summary:        Documentation for %{sname}
> Group:          Documentation
> Requires:       %{name}%{?_isa} = %{version}-%{release}

A -doc subpackage for two files? You cannot be happy with that either:

 | %files doc
 | %doc ossim/README.txt
 |
 | %license ossim/LICENSE.txt

And clearly such a -doc package does not strictly need to depend on the base package and all its dependencies. That's a lot of overhead for anyone, who would only like to peruse the documentation (e.g. when evaluating/considering whether to use the software).

Btw, there are specific guidelines on where to include the license text:
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text


> %package	    data
> Summary:        Additional data files for %{sname}
> Group:          Documentation
> Requires:       %{name}%{?_isa} = %{version}-%{release}

A strange Group tag here, too. And files in /usr/share are supposed to be arch-independent, so the -data subpackage should be made "BuildArch: noarch".

The summary says "additional data files". Where are the other data files? There are none. These are the only packaged data files. So, %summary and %description should tell what these are for and why/when you may want to install this optional package. A hint about the included profile.d files and what they do would be added value, too.


The spec files included in the source tarball are very different.

Comment 14 Rashad M 2015-08-02 11:24:58 UTC
Hello Marcin,

Thanks for quick response. It was very helpful for me.

libossim.so is used by applications and development. So the only option to keep it in non-devel package as the -devel package has always a dependency to non-devel.

regarding profile.d files. All it does is set an environment variable. Is there any option in Fedora packaging to set an env var when a package is installed?


If there isn't any one option to move in the application excutables to location that cannot be found by system path such as /usr/lib64/ossim-apps/ossim-info

and have a shell script in /usr/bin/ossim-info

the script /usr/bin/ossim-info will set the env var and launch the application in /usr/lib64/ossim-apps/ossim-info

I can do that in spec file, but not sure it is acceptable in packaging.

Comment 15 Rashad M 2015-08-02 11:50:42 UTC
(In reply to Michael Schwendt (Fedora Packager Sponsors Group) from comment #13)
> > 3) ossim-devel.x86_64: E: library-without-ldconfig-postin /usr/lib64/libossim.so.1.8.18
> > 
> > I also have no idea why the error occurs,
> 
> It's an error, because the files are misplaced. Let's take a look:
> 
> | %files
> | %{_libdir}/libossim.so.*
> | 
> | %files devel
> | %{_includedir}/ossim*
> | %{_libdir}/*.so*
> 
> First of all, the wildcards "libossim.so.*" and "*.so*" overlap. The latter
> also includes the former, i.e. "*.so*" also matches files matched by
> "*.so.*" and hence "libossim.so.*", too.
> 
> Since you duplicate shared libs in two packages, you probably don't see any
> symptoms at runtime, but have you examined the package dependencies yet? Run
> "rpm -qp --whatrequires …" and "rpm -qp --whatprovides …" on the built
> binary packages. That's important for understanding RPM package dependencies.
> 
> Secondly, the important thing about placement of .so files is to decide when
> they are needed. At runtime? Or at buildtime only? Or at runtime _and_ at
> buildtime?
> 
> libossim.so.1.8.18 is a runtime library to be put into the base package.
> Same for libossim.so.1 and further versions that result in _automatic_
> dependencies in your packages.
> 
> On the contrary, an unversioned symlink libossim.so is what makes linking at
> buildtime work. Without libossim.so the compiler would not find the library
> when given the -lossim linker option.
> 
> [Finally, the third case is rare. A program doing a dlopen() call (or
> something similar) to open an unversioned shared library at runtime. Such a
> case can be nasty, because there are no automatic RPM dependencies you can
> rely on, and if the unversioned .so file is shipped in a -devel package,
> this can get ugly if the library is needed at runtime always.]
> 
> 
> | %post -n ossim -p /sbin/ldconfig
> | 
> | %postun -n ossim -p /sbin/ldconfig
> 
> Now back to the error. You execute ldconfig for the base "ossim" package
> only, but rpmlint complains about the -devel subpackage (!) where you
> include runtime libraries, too. The fix would be to not include any runtime
> libs in the -devel package, and then you don't need to run ldconfig for it
> either (which would be the normal case, btw).
> 
> The explicit "-n ossim" is not needed, btw. The base package name is the
> default for scriptlet sections lacking an -n option.
> 
> [...]
> 
> The package needs quite a bit of more work. These are just some drive-by
> comments.
> 
> 
> Try pointing the fedora-review tool at this ticket:
> 
>   fedora-review -b 1244353
> 
> It will download the latest spec file and src.rpm from the "Spec URL:" and
> "SRPM URL:" lines you include in your comments and perform many helpful
> checks.
> 
> 
> There is the helpful %{_fixperms} macro for correcting permissions with the
> source tree:
> 
>   $ rpm -E %_fixperms
>   /usr/bin/chmod -Rf a+rX,u+w,g-w,o-w
> 
do you have an example usage? When I googled, it was executed after setup macro?

so should I run it on buildroot?
> 
> > %package 	apps
> > Summary:        %{sname} applications
> > Group:          System Environment/Libraries
> > Requires:       %{name}%{?_isa} = %{version}-%{release}
> 
> An unusual Group tag for files in %_bindir. The tag is optional nowadays, so
> you may want to remove it everywhere from the spec file.
> 
> 
> > %package	    doc
> > Summary:        Documentation for %{sname}
> > Group:          Documentation
> > Requires:       %{name}%{?_isa} = %{version}-%{release}
> 
> A -doc subpackage for two files? You cannot be happy with that either:
> 
>  | %files doc
>  | %doc ossim/README.txt
>  |
>  | %license ossim/LICENSE.txt

Can I move the readme in the base package and remove -doc package?

> 
> And clearly such a -doc package does not strictly need to depend on the base
> package and all its dependencies. That's a lot of overhead for anyone, who
> would only like to peruse the documentation (e.g. when
> evaluating/considering whether to use the software).
> 
> Btw, there are specific guidelines on where to include the license text:
> https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text
> 
> 
> > %package	    data
> > Summary:        Additional data files for %{sname}
> > Group:          Documentation
> > Requires:       %{name}%{?_isa} = %{version}-%{release}
> 
> A strange Group tag here, too. And files in /usr/share are supposed to be
> arch-independent, so the -data subpackage should be made "BuildArch: noarch".
> 
removed all Group tag and also set BuildArch:noarch for data package

> The summary says "additional data files". Where are the other data files?
> There are none. These are the only packaged data files. So, %summary and
> %description should tell what these are for and why/when you may want to
> install this optional package. A hint about the included profile.d files and
> what they do would be added value, too.
> 

There are some csv files in ossim/share/projection
and templates of keywordlists. I will update the description.

> 
> The spec files included in the source tarball are very different.

I updated the spec but couldn't host the srpm for the new version.

Comment 17 Marcin Haba 2015-08-03 21:01:19 UTC
Hello Rashad,

Thanks for preparing your new pair Spec and SRPM. URLs look also well. Thanks for it too.

I will try to perform informal review tomorrow afternoon. Then I back here and let you know.

Comment 18 Rashad M 2015-08-04 06:51:36 UTC
wait. I have some more updates. will update those same url in 1hr

Comment 19 Rashad M 2015-08-04 07:24:22 UTC
updated

Spec URL: https://git.orfeo-toolbox.org/otb-devutils.git/blob_plain/HEAD:/Packaging/fedora/SPECS/ossim.spec

SRPM URL: https://www.orfeo-toolbox.org//srpms/ossim-1.8.18-2.fc20.src.rpm

rpmlint shows below message:


ossim.x86_64: W: spelling-error %description -l en_US geospatial -> spatial
ossim.x86_64: W: shared-lib-calls-exit /usr/lib64/libossim.so.1.8.18 exit@GLIBC_2.2.5
ossim.x86_64: W: no-documentation
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/support_data/ossimNitfDataExtensionSegmentV2_1.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/src/ossim/base/ossimAdjSolutionAttributes.cpp
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/base/ossimBinaryDataProperty.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/support_data/ossimNitfImageDataMaskV2_1.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/src/ossim/support_data/ossimNitfDataExtensionSegmentV2_1.cpp
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/base/ossimGeodeticEvaluator.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/base/ossimAdjSolutionAttributes.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/src/ossim/support_data/ossimNitfImageDataMaskV2_1.cpp
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/src/ossim/base/ossimBinaryDataProperty.cpp
ossim-devel.x86_64: W: only-non-binary-in-usr-lib
ossim-devel.x86_64: W: no-documentation
ossim-devel.x86_64: E: incorrect-fsf-address /usr/include/ossim/vpfutil/values.h
4 packages and 0 specfiles checked; 1 errors, 14 warnings.

Comment 20 Rashad M 2015-08-04 07:58:31 UTC
just to let you know that, rpmlint error incorrect-fsf-address has been pushed upstream.

he executable permission for sources has been fixed in the spec but they are still showing on rpmlint. I dont know why.

Comment 21 Marcin Haba 2015-08-04 22:21:50 UTC
Hello Rashad,

I prepared informal review for your proposed package.

Results are below:


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

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



===== 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:
[!]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[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.
[!]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "MIT/X11 (BSD like)", "LGPL (v2.1 or later) (with incorrect FSF
     address)", "LGPL (v2 or later) (with incorrect FSF address)", "Unknown
     or generated". 1843 files have unknown license. Detailed output of
     licensecheck in /home/gani/1244353-ossim/licensecheck.txt
[!]: License file installed when any subpackage combination is installed.
[x]: Package requires other packages for directories it uses.
     Note: No known owner of /usr/lib64/cmake/ossim, /usr/lib64/ossim-apps
[x]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/lib64/ossim-apps,
     /usr/lib64/cmake/ossim, /usr/lib64/cmake
[ ]: %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.
[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]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any
     that are listed in the exceptions section of Packaging Guidelines.
[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]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 0 bytes in 0 files.
[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).
[!]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in ossim-
     doc , ossim-data
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise
     justified.
[!]: Scriptlets must be sane, if used.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[ ]: 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.
[!]: Spec use %global instead of %define unless justified.
     Note: %define requiring justification: %define sname OSSIM
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[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]: Uses parallel make %{?_smp_mflags} macro.
[x]: SourceX is a working URL.

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

Generic:
[x]: Rpmlint is run on debuginfo package(s).
     Note: There are rpmlint messages (see attachment).
[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: ossim-1.8.18-2.fc21.x86_64.rpm
          ossim-devel-1.8.18-2.fc21.x86_64.rpm
          ossim-apps-1.8.18-2.fc21.x86_64.rpm
          ossim-doc-1.8.18-2.fc21.noarch.rpm
          ossim-data-1.8.18-2.fc21.noarch.rpm
          ossim-1.8.18-2.fc21.src.rpm
ossim.x86_64: W: spelling-error %description -l en_US geospatial -> spatial
ossim.x86_64: W: shared-lib-calls-exit /usr/lib64/libossim.so.1.8.18 exit@GLIBC_2.2.5
ossim.x86_64: W: no-documentation
ossim-devel.x86_64: W: only-non-binary-in-usr-lib
ossim-devel.x86_64: W: no-documentation
ossim-devel.x86_64: E: incorrect-fsf-address /usr/include/ossim/vpfutil/values.h
ossim-data.noarch: W: spelling-error %description -l en_US kwl -> kw, kl, awl
ossim-data.noarch: W: spelling-error %description -l en_US csv -> cs, cs v, CST
ossim-data.noarch: W: no-documentation
ossim.src: W: spelling-error %description -l en_US geospatial -> spatial
6 packages and 0 specfiles checked; 1 errors, 9 warnings.




Rpmlint (debuginfo)
-------------------
Checking: ossim-debuginfo-1.8.18-2.fc21.x86_64.rpm
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/support_data/ossimNitfDataExtensionSegmentV2_1.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/src/ossim/base/ossimAdjSolutionAttributes.cpp
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/base/ossimBinaryDataProperty.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/support_data/ossimNitfImageDataMaskV2_1.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/src/ossim/support_data/ossimNitfDataExtensionSegmentV2_1.cpp
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/base/ossimGeodeticEvaluator.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/base/ossimAdjSolutionAttributes.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/src/ossim/support_data/ossimNitfImageDataMaskV2_1.cpp
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/src/ossim/base/ossimBinaryDataProperty.cpp
1 packages and 0 specfiles checked; 0 errors, 9 warnings.





Rpmlint (installed packages)
----------------------------
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/support_data/ossimNitfDataExtensionSegmentV2_1.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/src/ossim/base/ossimAdjSolutionAttributes.cpp
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/base/ossimBinaryDataProperty.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/support_data/ossimNitfImageDataMaskV2_1.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/src/ossim/support_data/ossimNitfDataExtensionSegmentV2_1.cpp
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/base/ossimGeodeticEvaluator.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/include/ossim/base/ossimAdjSolutionAttributes.h
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/src/ossim/support_data/ossimNitfImageDataMaskV2_1.cpp
ossim-debuginfo.x86_64: W: spurious-executable-perm /usr/src/debug/ossim-1.8.18/ossim/src/ossim/base/ossimBinaryDataProperty.cpp
ossim.x86_64: W: spelling-error %description -l en_US geospatial -> spatial
ossim.x86_64: W: unused-direct-shlib-dependency /usr/lib64/libossim.so.1.8.18 /lib64/libgeos_c.so.1
ossim.x86_64: W: shared-lib-calls-exit /usr/lib64/libossim.so.1.8.18 exit@GLIBC_2.2.5
ossim.x86_64: W: no-documentation
ossim-devel.x86_64: W: only-non-binary-in-usr-lib
ossim-devel.x86_64: W: no-documentation
ossim-devel.x86_64: E: incorrect-fsf-address /usr/include/ossim/vpfutil/values.h
ossim-data.noarch: W: spelling-error %description -l en_US kwl -> kw, kl, awl
ossim-data.noarch: W: spelling-error %description -l en_US csv -> cs, cs v, CST
ossim-data.noarch: W: no-documentation
6 packages and 0 specfiles checked; 1 errors, 18 warnings.


My notes
-----------
1)
[!]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.

I marked this point as fail because from that what I noticed OSSIM basis on GeoTrans application code.
I found Terms of use this GeoTrans code here:
http://earth-info.nga.mil/GandG/geotrans/docs/MSP_GeoTrans_Terms_of_Use.pdf

I have not found GeoTrans License in allowed licenses in Fedora:

https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#SoftwareLicenses

If you have some additional information about this 'terms of use', please write here. If you want or you have some doubts for it, you can also send e-mail to fedora-legal mailing list here: https://admin.fedoraproject.org/mailman/listinfo/legal

2)
[!]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "MIT/X11 (BSD like)", "LGPL (v2.1 or later) (with incorrect FSF
     address)", "LGPL (v2 or later) (with incorrect FSF address)", "Unknown
     or generated". 1843 files have unknown license. Detailed output of
     licensecheck in /home/gani/1244353-ossim/licensecheck.txt

I marked it as failed due to GeoTrans license is not mentioned in Spec.

3) 

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

This point I marked as fail because I noticed that License file is installed only when is installed ossim-doc subpackage. I think that you can move license file to main ossim package and mark all subpackages by "Require: ossim" tag.

4)

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

I have not marked this point because I am not familar with compiler flags. So, better is leave empty to verify, than make a mistake.

5) 

[ ]: Package complies to the Packaging Guidelines

I have not marked this point because I am not able to determine if packages compiles to the Packaging Guidelines. I am still learning these guidelines.

6)

[!]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in ossim-
     doc , ossim-data

If you add "Requires: ossim" to subpackages, then this point should be OK.

7)

[!]: Scriptlets must be sane, if used.

I have not found in Fedora scriptlets %_fixperms that you used in spec.
https://fedoraproject.org/wiki/Packaging:ScriptletSnippets

8) 

[ ]: Package should compile and build into binary rpms on all supported
     architectures.

I have no possibility to check compilation on all supported architectures.

Also, you did not define which architectures are they. I guess that all supported.

9)

[!]: Spec use %global instead of %define unless justified.
     Note: %define requiring justification: %define sname OSSIM

Probably better in this point is use %global.
https://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define


10)

Programs used in /usr/bin/ossim-* do not work. All they are scripts that have defined the same content, for example:

cat /usr/bin/ossim-btoa 
#!/bin/bash
export OSSIM_PREFS_FILE=/usr/share/ossim/ossim_preferences

$ cat /usr/bin/ossim-orthoigen 
#!/bin/bash
export OSSIM_PREFS_FILE=/usr/share/ossim/ossim_preferences

...etc.

Valid programs are placed in invalid place (/usr/lib64) here:

/usr/lib64/ossim-apps/ossim-*

11)

In build.log I found this lines:

cpio: ossim-1.8.18/build/src/ossim/lex.ossimEquTokenizer.cc: Cannot stat: No such file or directory
cpio: ossim-1.8.18/build/src/ossim/ossimEquTokenizer.l: Cannot stat: No such file or directory


12)

Please look on Michael's comments. They are useful too.

13)

Thanks for your info about rpmlint warnings. In my opinion this point needs a little work either.

Summary
==========
@Rashad, thank you for your effort in preparing ossim package. From my point of view points 2-13) are possible to do by work on preparing ossim to Fedora. Point 1) can require write on fedora-legal mailing list and maybe contact with GeoTrans institution. Anyway, reliable info about it you can take from fedora-legal.

Thanks for review process.

Comment 22 Rashad M 2015-08-27 09:59:51 UTC
@Marcin,


coming back on this page..

updated spec and srpms

Spec URL: https://git.orfeo-toolbox.org/otb-devutils.git/blob_plain/HEAD:/Packaging/fedora/SPECS/ossim.spec

SRPM URL: https://www.orfeo-toolbox.org//srpms/ossim-1.8.18-2.fc20.src.rpm

Comment 23 Rashad M 2015-09-07 19:04:20 UTC
I had asked upstream about the usage of geotrans source inside ossim. No response yet. 

One of my colleagues mentioned there is a geotranz package in debian. This could be used instead of internal one in ossim with a patch to fix issue. 

Any thoughts?


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