[Bug 1673956] New: Review Request: octave-openems - An electromagnetic field solver for octave

https://bugzilla.redhat.com/show_bug.cgi?id=1673956

Bug ID: 1673956
           Summary: Review Request: octave-openems - An electromagnetic
                    field solver for octave
           Product: Fedora
           Version: rawhide
          Hardware: All
                OS: Linux
            Status: NEW
         Component: Package Review
          Severity: medium
          Priority: medium
          Assignee: [e-mail]
          Reporter: [e-mail]
        QA Contact: [e-mail]
                CC: [e-mail]
  Target Milestone: ---
    Classification: Fedora

Spec URL: https://fedorapeople.org/~sailer/octave-openems.spec
SRPM URL: https://fedorapeople.org/~sailer/octave-openems-0.0.35-1.fc29.src.rpm

Description: 
openEMS is a free and open electromagnetic field solver using the FDTD
method. Octave is used as an easy and flexible scripting interface.

It features:
* fully 3D Cartesian and cylindrical coordinates graded mesh.
* Multi-threading, SIMD (SSE) support for high speed FDTD.

Fedora Account System Username:
sailer

COPR build:
https://copr.fedorainfracloud.org/coprs/sailer/radiofrequency/build/856375/

[Bug 1673956] Review Request: octave-openems - An electromagnetic field solver for octave

https://bugzilla.redhat.com/show_bug.cgi?id=1673956

Hirotaka Wakabayashi  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #1 from Hirotaka Wakabayashi  ---
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@GLIBC_2.2.5
  octave-openems.x86_64: W: shared-lib-calls-exit /usr/lib64/libnf2ff.so.0.1.0
exit@GLIBC_2.2.5
  octave-openems.x86_64: W: shared-lib-calls-exit
/usr/lib64/libopenEMS.so.0.0.35 exit@GLIBC_2.2.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@GLIBC_2.2.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@GLIBC_2.2.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@GLIBC_2.2.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/LicensingGuideline...
[2] https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#SoftwareLicenses
[3]
https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuideline...

Thanks in advance,
Hirotaka Wakabayashi

[Bug 1673956] Review Request: octave-openems - An electromagnetic field solver for octave

https://bugzilla.redhat.com/show_bug.cgi?id=1673956

--- Comment #2 from 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_...

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

needinfo canceled: [Bug 1673956] Review Request: octave-openems - An electromagnetic field solver for octave

Product: Fedora
Version: rawhide
Component: Package Review

Thomas Sailer  has canceled Package Review
's request for Thomas Sailer
's needinfo:
Bug 1673956: Review Request: octave-openems - An electromagnetic field solver
for octave
https://bugzilla.redhat.com/show_bug.cgi?id=1673956

--- Comment #4 from Thomas Sailer  ---
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/

[Bug 1673956] Review Request: octave-openems - An electromagnetic field solver for octave

https://bugzilla.redhat.com/show_bug.cgi?id=1673956

Thomas Sailer  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|needinfo?(t.sailer@alumni.e |
                   |thz.ch)                     |

--- Comment #4 from Thomas Sailer  ---
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/

needinfo canceled: [Bug 1673956] Review Request: octave-openems - An electromagnetic field solver for octave

Product: Fedora
Version: rawhide
Component: Package Review

Package Review  has canceled Package
Review 's request for Thomas Sailer
's needinfo:
Bug 1673956: Review Request: octave-openems - An electromagnetic field solver
for octave
https://bugzilla.redhat.com/show_bug.cgi?id=1673956

--- Comment #6 from Package Review  ---
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.