Bug 1673956 - Review Request: octave-openems - An electromagnetic field solver for octave
Summary: Review Request: octave-openems - An electromagnetic field solver for octave
Keywords:
Status: NEW
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
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: 2019-02-08 14:48 UTC by Thomas Sailer
Modified: 2020-07-10 08:16 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed:
Type: ---


Attachments (Terms of Use)

Description Thomas Sailer 2019-02-08 14:48:16 UTC
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/

Comment 1 Hirotaka Wakabayashi 2019-03-21 11:54:18 UTC
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/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

Comment 2 Hirotaka Wakabayashi 2019-03-23 11:17:47 UTC
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

Comment 3 Package Review 2020-07-10 00:57:11 UTC
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.

Comment 4 Thomas Sailer 2020-07-10 08:16:39 UTC
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/


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