Bug 1673956

Hello, this is not a complete review. I will do additional review tomorrow.
Please read this for your reference.

Summary
=======

1. rpmlint results
2. Koji scratch build succeeded
3. License
   
Details
=======

1. rpmlint results
------------------

One error and one warning on the source rpm and 11 warnings on the binary rpms.
Here are the rpmlint results::
  
  $ rpmlint /home/vagrant/rpmbuild/SRPMS/octave-openems-0.0.35-1.fc29.src.rpm 
  octave-openems.src:15: W: macro-in-comment %{version}
  octave-openems.src: E: specfile-error warning: Macro expanded in comment on line 15: %{version}.tar.xz --exclude-vcs openEMS-Project
  1 packages and 0 specfiles checked; 1 errors, 1 warnings.

  $ rpmlint /home/vagrant/rpmbuild/RPMS/x86_64/octave-ctb-0.0.35-1.fc29.x86_64.rpm
  octave-ctb.x86_64: W: dangerous-command-in-%preun mv
  1 packages and 0 specfiles checked; 0 errors, 1 warnings.

  $ rpmlint /home/vagrant/rpmbuild/RPMS/x86_64/octave-hyp2mat-0.0.35-1.fc29.x86_64.rpm
  octave-hyp2mat.x86_64: W: manual-page-warning /usr/share/man/man1/hyp2mat.1.gz 230: warning: macro `ni' not defined
  octave-hyp2mat.x86_64: W: dangerous-command-in-%preun mv
  1 packages and 0 specfiles checked; 0 errors, 2 warnings.

  $ rpmlint /home/vagrant/rpmbuild/RPMS/x86_64/octave-hyp2mat-debuginfo-0.0.35-1.fc29.x86_64.rpm
  1 packages and 0 specfiles checked; 0 errors, 0 warnings.

  $ rpmlint /home/vagrant/rpmbuild/RPMS/x86_64/octave-openems-0.0.35-1.fc29.x86_64.rpm
  octave-openems.x86_64: W: shared-lib-calls-exit /usr/lib64/libQCSXCAD.so.0.6.2 exit.5
  octave-openems.x86_64: W: shared-lib-calls-exit /usr/lib64/libnf2ff.so.0.1.0 exit.5
  octave-openems.x86_64: W: shared-lib-calls-exit /usr/lib64/libopenEMS.so.0.0.35 exit.5
  octave-openems.x86_64: W: no-manual-page-for-binary AppCSXCAD
  octave-openems.x86_64: W: no-manual-page-for-binary nf2ff
  octave-openems.x86_64: W: no-manual-page-for-binary openEMS
  octave-openems.x86_64: W: dangerous-command-in-%preun mv
  1 packages and 0 specfiles checked; 0 errors, 7 warnings.

  $ rpmlint /home/vagrant/rpmbuild/RPMS/x86_64/octave-openems-debuginfo-0.0.35-1.fc29.x86_64.rpm
  1 packages and 0 specfiles checked; 0 errors, 0 warnings.

  $ rpmlint /home/vagrant/rpmbuild/RPMS/x86_64/octave-openems-debugsource-0.0.35-1.fc29.x86_64.rpm
  1 packages and 0 specfiles checked; 0 errors, 0 warnings.

  $ rpmlint /home/vagrant/rpmbuild/RPMS/x86_64/octave-openems-devel-0.0.35-1.fc29.x86_64.rpm
  octave-openems-devel.x86_64: W: no-documentation
  1 packages and 0 specfiles checked; 0 errors, 1 warnings.

My review on the result above is as followings.

1.1.  octave-openems.src:15: W: macro-in-comment %{version}
You can escape a macro in comment in the specfile by adding another leading %
to suppress this warning. Macros in comments can be a problem because they are
expanded everywhere.::
  $ diff octave-openems.spec.orig octave-openems.spec
  15c15
  < # tar cJvf openems-%{version}.tar.xz --exclude-vcs openEMS-Project
  ---
  > # tar cJvf openems-%%{version}.tar.xz --exclude-vcs openEMS-Project

1.2.  octave-openems.src: E: specfile-error warning: Macro expanded in comment on line 15: %{version}.tar.xz --exclude-vcs openEMS-Project
I think the reason is same with the 1.1's one.

1.3.  octave-ctb.x86_64: W: dangerous-command-in-%preun mv
I think this warning is probably because modifying the file system by root.
Executing "rpmspec -P octave-openems.spec" will show what it is doing.

1.4.  octave-hyp2mat.x86_64: W: manual-page-warning /usr/share/man/man1/hyp2mat.1.gz 230: warning: macro `ni' not defined
The "ni" macro is undefined.

1.5.  octave-hyp2mat.x86_64: W: dangerous-command-in-%preun mv
I think this warning is probably because modifying the file system by root.
Executing "rpmspec -P octave-openems.spec" will show what it is doing.

1.6.  octave-openems.x86_64: W: shared-lib-calls-exit /usr/lib64/libQCSXCAD.so.0.6.2 exit.5
Functions in this library should return success or error so that calling 
program can handle the result. The library might not return nothing and
call the "exit" function that causes normal process termination.

1.7.  octave-openems.x86_64: W: shared-lib-calls-exit /usr/lib64/libnf2ff.so.0.1.0 exit.5
The reason is same with the 1.6's one.

1.8.  octave-openems.x86_64: W: shared-lib-calls-exit /usr/lib64/libopenEMS.so.0.0.35 exit.5
The reason is same with the 1.6's one.

1.9.  octave-openems.x86_64: W: no-manual-page-for-binary AppCSXCAD
The package should contain the man page for "AppCSXCAD" [1]. You might know that
help2man [2] is a useful tool.

[1] https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages
[2] https://www.gnu.org/software/help2man/

1.10.  octave-openems.x86_64: W: no-manual-page-for-binary nf2ff
The reason is same with the 1.9's one.

1.11.  octave-openems.x86_64: W: no-manual-page-for-binary openEMS
The reason is same with the 1.9's one.

1.12.  octave-openems.x86_64: W: dangerous-command-in-%preun mv
I think this warning is probably because modifying the file system by root.
Executing "rpmspec -P octave-openems.spec" will show what it is doing.

1.13.  octave-openems-devel.x86_64: W: no-documentation
The package should include documentation like README if you have.

2. Koji scratch build succeeded
--------------------------------

Here is the result of "koji build --scratch rawhide octave-openems-0.0.35-1.fc29.src.rpm"
https://koji.fedoraproject.org/koji/taskinfo?taskID=33656976

Here is the reference to run a koji scratch build.
https://fedoraproject.org/wiki/Using_the_Koji_build_system#Scratch_Builds

3. License
-----------

The packaging guidelines say maintainers must make every possible effort to be
accurate when filling the License: field [1]. If QCSXCAD is licensed under
LGPL-3.0, License: field should contain "LGPLv3". See the Fedora Software 
License List [2].

The packaging guidelines say multiple Licensing scenario is highly encouraged to
be avoided whenever reasonably possible [3]. If multiple Licensing scenario 
happens, the package must contain a comment explaining the multiple licensing 
breakdown [3].

[1] https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_field
[2] https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#SoftwareLicenses
[3] https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_multiple_licensing_scenarios


Thanks in advance,
Hirotaka Wakabayashi

Hello, this is an additional review to #1. Please read this for you reference.

Summary
=======

1. Legibility
2. Source URL
3. Bundling multiple GitHub project's code
4. Directory Ownership

Details
=======

1. Legibility
-----------

Each package must consistently use macros. Here are guidelines::
  
  https://docs.fedoraproject.org/en-US/packaging-guidelines/#_macros
  https://docs.fedoraproject.org/en-US/packaging-guidelines/#_using_buildroot_and_optflags_vs_rpm_build_root_and_rpm_opt_flags

2. Source URL

The "extraversion" macro value, which is "_11_g6a75e98", should be included in
the SOURCE0 comment description. Here is the result of the part of the comment
description::
  
  $ cd openEMS-Project; git describe --tags | sed -e s,-,_,
  v0.0.35_13-g78e7642

I think the value should be same with the "extraversion" macro value, because
the sources used to build the package must match the upstream source. See the
SourceURL guidelines.

https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/

3. Bundling multiple GitHub project's code

The openEMS-Project contains multiple GitHub project's source code as GitHub
submodules. Each submodule the parent module depends on should be a link to
a module globally installed from a package, because I think the Fedora package
guidelines are basically provided that one software, which I think is defined
by its own license and version, should be one package.

4. Directory Ownership

The following directories should be removed after removing relevant packages::
  
  /usr/share/octave/packages/csxcad-0.0.35/private
  /usr/share/octave/packages/openems-0.0.35/private

The following patch solved the problem above in my environment::

  $ diff octave-openems.spec.orig octave-openems.spec
  299a300
  > %dir %{octpkgdir}/private
  306a308
  > %dir %{octpkgdir}/private

See http://ftp.rpm.org/max-rpm/s1-rpm-inside-files-list-directives.html for the
%docdir directive.


Thanks in advance,
Hirotaka Wakabayashi

This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time. We're sorry
it is taking so long. If you're still interested in packaging this software
into Fedora repositories, please respond to this comment clearing the
NEEDINFO flag.

You may want to update the specfile and the src.rpm to the latest version
available and to propose a review swap on Fedora devel mailing list to increase
chances to have your package reviewed. If this is your first package and you
need a sponsor, you may want to post some informal reviews. Read more at
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group.

Without any reply, this request will shortly be considered abandoned
and will be closed.
Thank you for your patience.

Hi Hirotaka,

thanks for the review!

Macro expanded in comment: thanks for spotting this, will fix.

dangerous-command-in-%preun: this is due to a macro defined in package octave-devel; it should be addressed there if at all

shared-lib-calls-exit: somewhat unfortunate, but this is only used in critical error conditions to abort, so I don't think it is a problem

no-manual-page-for-binary: man pages are not mandatory; a man page for a gui program that takes no options whatsoever does not provide a lot of value; the other binaries are not intended to be used directly, but rather are used as helper binaries by the octave packages

no-documentation: this is a shortcoming of rpmlint; the documentation is part of the octave package, it will be shown in octave with for example pkg describe openems

license: I missed the LGPLv3, thanks for catching this; I've added the license breakdown

consistent use of macros: can you please point out where you see inconsistencies?

source url: I don't understand the issue, git describe --tags | sed -e s,-,_, should be the same as v%{version}%{extraversion}

%{octpkgdir}/private: that seems to be an issue with the macros in octave-devel...

https://sailer.fedorapeople.org/octave-openems-0.0.35-1.fc33.src.rpm
https://sailer.fedorapeople.org/octave-openems.spec
https://copr.fedorainfracloud.org/coprs/sailer/radiofrequency/build/1544079/

This is an automatic check from review-stats script.

This review request ticket hasn't been updated for some time. We're sorry
it is taking so long. If you're still interested in packaging this software
into Fedora repositories, please respond to this comment clearing the
NEEDINFO flag.

You may want to update the specfile and the src.rpm to the latest version
available and to propose a review swap on Fedora devel mailing list to increase
chances to have your package reviewed. If this is your first package and you
need a sponsor, you may want to post some informal reviews. Read more at
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group.

Without any reply, this request will shortly be considered abandoned
and will be closed.
Thank you for your patience.

This is an automatic action taken by review-stats script.

The ticket submitter failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we consider this ticket as DEADREVIEW and proceed to close it.