Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 1760617 - Review Request: mmc - A GPU mesh-based Monte Carlo photon simulator
Summary: Review Request: mmc - A GPU mesh-based Monte Carlo photon simulator
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Ankur Sinha (FranciscoD)
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: fedora-neuro, NeuroFedora
TreeView+ depends on / blocked
 
Reported: 2019-10-10 23:27 UTC by Qianqian Fang
Modified: 2020-03-16 20:32 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-03-03 21:41:48 UTC
Type: ---
Embargoed:
sanjay.ankur: fedora-review+


Attachments (Terms of Use)
Build log showing compiler warnings (deleted)
2019-10-28 09:27 UTC, Ankur Sinha (FranciscoD)
no flags Details

Description Qianqian Fang 2019-10-10 23:27:55 UTC
Spec URL: https://github.com/fangq/fedorapkg/blob/mmclab/octave-mmclab.spec
SRPM URL: https://kwafoo.coe.neu.edu/~fangq/share/temp/octave-mmclab-1.7.9-1.fc30.src.rpm
Description: 

MMCLAB is the native MEX version of MMC - Mesh-based Monte Carlo - for
MATLAB and GNU Octave. By converting the input and output files into
convenient in-memory variables, MMCLAB is very intuitive to use and
straightforward to be integrated with mesh generation and post-simulation
analyses. Because MMCLAB contains the same computational codes for
OpenCL-based photon simulation as in a MMC binary, running MMCLAB
inside MATLAB is expected to give similar speed as running a standalone
MMC binary using either a CPU or a GPU.

Fedora Account System Username: fangq

Comment 1 Qianqian Fang 2019-10-10 23:33:32 UTC
rpmlint output for the srpm package

----------------------------------------------------------------------------
fangq@localhost SRPMS]$ rpmlint octave-mmclab-1.7.9-1.fc30.src.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.
----------------------------------------------------------------------------

rpmlint output for the 4 output rpm packages (octave-mmclab, mmclab-demos, mmc, mmc-demos)

----------------------------------------------------------------------------
[fangq@localhost RPMS]$ find . -name "*mmc*.rpm" -print -exec rpmlint '{}' \;
----------------------------------------------------------------------------
./noarch/mmc-demos-1.7.9-1.fc30.noarch.rpm
mmc-demos.noarch: W: spelling-error Summary(en_US) datasets -> data sets, data-sets, databases
mmc-demos.noarch: W: spelling-error %description -l en_US datasets -> data sets, data-sets, databases
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/colin27/prop_brain.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/dcs/dcs.inp
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/dcs/dcs_g1_Db.m
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/dcs/dcs_g1_Db_fms.m
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/dcs/dcs_g2_Db.m
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/dcs/dcs_g2_Db_fms.m
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/dcs/prop_dcs.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/meshtest/prop_mesh0.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/meshtest/prop_mesh1.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/meshtest/prop_mesh2.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/onecube/prop_onecube.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/reftest/prop_onecube.dat
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/regression/exitangle/phantom.m
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/replay/prop_replaytest.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/replaywide/prop_replaywide.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/rngtest/Makefile
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/sfdi2layer/README.txt
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/sfdi2layer/createmesh.m
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/sfdi2layer/prop_sfdi.dat
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/sfdi2layer/sfdi.inp
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/sharing/README.txt
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/sharing/prop_sharing.dat
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/sharing/sharing.inp
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/validation/prop_cube.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/validation/prop_cube2.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/widedet/prop_widedet.dat
1 packages and 0 specfiles checked; 0 errors, 28 warnings.
----------------------------------------------------------------------------
./noarch/mmclab-demos-1.7.9-1.fc30.noarch.rpm
----------------------------------------------------------------------------
mmclab-demos.noarch: W: spelling-error Summary(en_US) datasets -> data sets, data-sets, databases
mmclab-demos.noarch: W: spelling-error %description -l en_US datasets -> data sets, data-sets, databases
mmclab-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmclab-demos/example/demo_sfdi_2layer.m
1 packages and 0 specfiles checked; 0 errors, 3 warnings.
----------------------------------------------------------------------------
./x86_64/octave-mmclab-1.7.9-1.fc30.x86_64.rpm
octave-mmclab.x86_64: W: dangerous-command-in-%preun mv
1 packages and 0 specfiles checked; 0 errors, 1 warnings.
----------------------------------------------------------------------------
./x86_64/mmc-1.7.9-1.fc30.x86_64.rpm
mmc.x86_64: W: spelling-error Summary(en_US) datasets -> data sets, data-sets, databases
mmc.x86_64: W: no-manual-page-for-binary mmc
1 packages and 0 specfiles checked; 0 errors, 2 warnings.
----------------------------------------------------------------------------

Comment 2 Qianqian Fang 2019-10-10 23:35:02 UTC
In this package, I also moved the .mex file to /usr/libexec (if this is the right place), and create a link in the toolbox folder. I will do the same for octave-zmat/octave-mcxlab.

Comment 3 Ankur Sinha (FranciscoD) 2019-10-11 09:44:50 UTC
Can you please provide the "raw" link to the spec file please? The fedora-review tool cannot work with the link in the current form since it returns HTML, not the raw spec file.

(In reply to Qianqian Fang from comment #2)
> In this package, I also moved the .mex file to /usr/libexec (if this is the
> right place), and create a link in the toolbox folder. I will do the same
> for octave-zmat/octave-mcxlab.

I don't think mex files need to be moved. You simply put these files (along with other arch specific files) into %{octpkglibdir} which expands to %{_libdir}/octave/packages/.. %{octpkglibdir} should be known to octave too, so nothing else should be needed here.

libexec was a special case because the package bundled binaries.

Comment 4 Ankur Sinha (FranciscoD) 2019-10-11 09:45:47 UTC
PS: please block the "fedora-neuro" tracker bug for NeuroFedora related packages. That'll ensure that the whole team gets notifications automatically.

Comment 5 Qianqian Fang 2019-10-11 13:29:14 UTC
Spec URL: https://raw.githubusercontent.com/fangq/fedorapkg/mmclab/octave-mmclab.spec
SRPM URL: https://kwafoo.coe.neu.edu/~fangq/share/temp/octave-mmclab-1.7.9-1.fc30.src.rpm
Description: 

MMCLAB is the native MEX version of MMC - Mesh-based Monte Carlo - for
MATLAB and GNU Octave. By converting the input and output files into
convenient in-memory variables, MMCLAB is very intuitive to use and
straightforward to be integrated with mesh generation and post-simulation
analyses. Because MMCLAB contains the same computational codes for
OpenCL-based photon simulation as in a MMC binary, running MMCLAB
inside MATLAB is expected to give similar speed as running a standalone
MMC binary using either a CPU or a GPU.

Fedora Account System Username: fangq

Comment 6 Qianqian Fang 2019-10-11 13:36:46 UTC
done. the raw link is posted, and moved .mex back with the .m files.

Comment 7 Qianqian Fang 2019-10-11 19:38:58 UTC
@Ankur, happy to set the block flag, but I can't find the field, can you let me know which item to change?

also, let me know if the updated spec file is acceptable. thanks

Comment 8 Ankur Sinha (FranciscoD) 2019-10-13 17:53:43 UTC
The new bugzilla hides the blocks/depends field under "Advanced fields". If you
click the button, it should show the fields.

Here is the review, but I'll have to do it again once you regenerate the SRPM
from the new spec and upload that:


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

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


Issues:
=======
- Should the main spec/package be simply called mmc and then octave-.. as
  sub-packages instead of the other way round?

- Why does the build require vim-common?

- 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.
  Note: License file COPYING is not marked as %license
  See: https://docs.fedoraproject.org/en-US/packaging-
  guidelines/LicensingGuidelines/#_license_text

COPYING is required for the octave package, so this looks OK.

- Large documentation must go in a -doc subpackage. Large could be size
  (~1MB) or number of files.
  Note: Documentation size is 5703680 bytes in 190 files.
  See: https://docs.fedoraproject.org/en-US/packaging-
  guidelines/#_documentation

Please move the docs to the a sub-package.

- Lots of bundling here too. Is this required?

$ licensecheck -r . | sed '/UNKNOWN/ d'
./LICENSE.txt: GPL (v3 or later)
./mmclab/LICENSE.txt: GPL (v3 or later)
./src/drand48_r_libgw32c.c: GNU Lesser General Public License (v2.1 or later)
./src/mmcdoxy.cfg: *No copyright* GENERATED FILE
./src/nifti1.h: Public domain
./examples/onecube/README.txt: *No copyright* GENERATED FILE
./examples/reftest/README.txt: *No copyright* GENERATED FILE
./mmclab/example/demo_head_atlas.m: *No copyright* GENERATED FILE
./src/SFMT/LICENSE.txt: BSD 3-clause "New" or "Revised" License
./src/cjson/LICENSE: Expat License
./src/cjson/README: Expat License
./src/cjson/cJSON.c: Expat License
./src/cjson/cJSON.h: Expat License
./src/cjson/test.c: Expat License
./src/sse_math/sse_math.h: zlib/libpng license
./src/waitmex/license.txt: BSD 3-clause "New" or "Revised" License


- Build flags aren't used. Example from the build.log file:
Building built/mmc.o
cc -c -Wall -g -DMCX_EMBED_CL -fno-strict-aliasing -m64 -DMMC_USE_SSE -DHAVE_SSE2 -msse -msse2 -msse3 -mssse3 -msse4.1 -O3 -fopenmp  -fPIC -DMCX_CONTAINER -DUSE_OS_TIMER -DUSE_OPENCL -I../src  -o built/mmc.o  mmc.c

Please use %{set_opt_flags} before you run %make_build

- You do not need to use pushd, you can use %make_build -C <directory name>

- There's still the ln -s mex etc. Is this the latest srpm? (Each time you
  update the spec file, you need to regenerate the srpm and upload the latest
  versions of both.)

- Quite a few rpmlint issues. Please correct them. Some hints here:
  https://fedoraproject.org/wiki/Common_Rpmlint_issues


===== 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]: If your application is a C or C++ application you must list a
     BuildRequires against gcc, gcc-c++ or clang.
[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: There is no build directory. Running licensecheck on vanilla
     upstream sources. No licenses found. Please check the source files for
     licenses manually.
^
Is this also bundling lots of things? Is this required?

$ licensecheck -r . | sed '/UNKNOWN/ d'
./LICENSE.txt: GPL (v3 or later)
./mmclab/LICENSE.txt: GPL (v3 or later)
./src/drand48_r_libgw32c.c: GNU Lesser General Public License (v2.1 or later)
./src/mmcdoxy.cfg: *No copyright* GENERATED FILE
./src/nifti1.h: Public domain
./examples/onecube/README.txt: *No copyright* GENERATED FILE
./examples/reftest/README.txt: *No copyright* GENERATED FILE
./mmclab/example/demo_head_atlas.m: *No copyright* GENERATED FILE
./src/SFMT/LICENSE.txt: BSD 3-clause "New" or "Revised" License
./src/cjson/LICENSE: Expat License
./src/cjson/README: Expat License
./src/cjson/cJSON.c: Expat License
./src/cjson/cJSON.h: Expat License
./src/cjson/test.c: Expat License
./src/sse_math/sse_math.h: zlib/libpng license
./src/waitmex/license.txt: BSD 3-clause "New" or "Revised" License

[x]: License file installed when any subpackage combination is installed.
[x]: Package does not own files or directories owned by other packages.
[!]: %build honors applicable compiler flags or justifies otherwise.

They do not. Example from the build.log file:
Building built/mmc.o
cc -c -Wall -g -DMCX_EMBED_CL -fno-strict-aliasing -m64 -DMMC_USE_SSE -DHAVE_SSE2 -msse -msse2 -msse3 -mssse3 -msse4.1 -O3 -fopenmp  -fPIC -DMCX_CONTAINER -DUSE_OS_TIMER -DUSE_OPENCL -I../src  -o built/mmc.o  mmc.c

Please use %{set_opt_flags} before you run %make_build

[!]: Package contains no bundled libraries without FPC exception.
cjson, SFMT, sse_math, waitmex and others are bundled.

Either they need to be unbundled, or specified, and license updated
accordingly.

[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[-]: 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
^
Issues noted.

[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 requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[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 must not depend on deprecated() packages.
[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:
[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.
[ ]: Final provides and requires are sane (see attachments).
[!]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in mmclab-
     demos , mmc , mmc-demos
Do the Requires be versioned?
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_explicit_requires

[?]: Package functions as described.
^ Not yet tested

[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
     Note: gpgverify is not used.
[-]: 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.
[x]: 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]: 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:
[!]: Spec file according to URL is the same as in SRPM.
     Note: Spec file as given by url is not the same as in SRPM (see
     attached diff).
     See: (this test has no URL)
^ This confirms that the generated srpm does not match the latest spec. Please
post the newest ones.

[x]: Rpmlint is run on debuginfo package(s).
     Note: No rpmlint messages.
[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.


Rpmlint
-------
Checking: octave-mmclab-1.7.9-1.fc32.x86_64.rpm
          mmclab-demos-1.7.9-1.fc32.noarch.rpm
          mmc-1.7.9-1.fc32.x86_64.rpm
          mmc-demos-1.7.9-1.fc32.noarch.rpm
          octave-mmclab-debuginfo-1.7.9-1.fc32.x86_64.rpm
          octave-mmclab-debugsource-1.7.9-1.fc32.x86_64.rpm
          octave-mmclab-1.7.9-1.fc32.src.rpm
octave-mmclab.x86_64: W: dangerous-command-in-%preun mv
mmclab-demos.noarch: W: spelling-error Summary(en_US) datasets -> data sets, data-sets, databases
mmclab-demos.noarch: W: spelling-error %description -l en_US datasets -> data sets, data-sets, databases
mmclab-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmclab-demos/example/demo_sfdi_2layer.m
mmc.x86_64: W: spelling-error Summary(en_US) datasets -> data sets, data-sets, databases
mmc.x86_64: W: no-manual-page-for-binary mmc
mmc-demos.noarch: W: spelling-error Summary(en_US) datasets -> data sets, data-sets, databases
mmc-demos.noarch: W: spelling-error %description -l en_US datasets -> data sets, data-sets, databases
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/colin27/prop_brain.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/dcs/dcs.inp
^
Please remove the executable perm

mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/dcs/dcs_g1_Db.m
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/dcs/dcs_g1_Db_fms.m
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/dcs/dcs_g2_Db.m
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/dcs/dcs_g2_Db_fms.m
^
Please correct this

mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/dcs/prop_dcs.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/meshtest/prop_mesh0.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/meshtest/prop_mesh1.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/meshtest/prop_mesh2.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/onecube/prop_onecube.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/reftest/prop_onecube.dat
^
Please correct this

mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/regression/exitangle/phantom.m
^
Please correct this

mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/replay/prop_replaytest.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/replaywide/prop_replaywide.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/rngtest/Makefile
^
Please correct this

mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/sfdi2layer/README.txt
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/sfdi2layer/createmesh.m
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/sfdi2layer/prop_sfdi.dat
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/sfdi2layer/sfdi.inp
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/sharing/README.txt
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/sharing/prop_sharing.dat
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/sharing/sharing.inp
^
Please correct this

mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/validation/prop_cube.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/validation/prop_cube2.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/widedet/prop_widedet.dat
^
Please correct this

7 packages and 0 specfiles checked; 0 errors, 34 warnings.




Rpmlint (debuginfo)
-------------------
Checking: octave-mmclab-debuginfo-1.7.9-1.fc32.x86_64.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.





Rpmlint (installed packages)
----------------------------
perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LC_CTYPE = "C.UTF-8",
	LANG = "en_GB.UTF-8"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LC_CTYPE = "C.UTF-8",
	LANG = "en_GB.UTF-8"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
mmc-demos.noarch: W: spelling-error Summary(en_US) datasets -> data sets, data-sets, databases
mmc-demos.noarch: W: spelling-error %description -l en_US datasets -> data sets, data-sets, databases
mmc-demos.noarch: W: invalid-url URL: http://mcx.space/#mmc <urlopen error [Errno -2] Name or service not known>
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/colin27/prop_brain.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/dcs/dcs.inp
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/dcs/dcs_g1_Db.m
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/dcs/dcs_g1_Db_fms.m
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/dcs/dcs_g2_Db.m
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/dcs/dcs_g2_Db_fms.m
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/dcs/prop_dcs.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/meshtest/prop_mesh0.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/meshtest/prop_mesh1.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/meshtest/prop_mesh2.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/onecube/prop_onecube.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/reftest/prop_onecube.dat
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/regression/exitangle/phantom.m
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/replay/prop_replaytest.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/replaywide/prop_replaywide.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/rngtest/Makefile
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/sfdi2layer/README.txt
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/sfdi2layer/createmesh.m
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/sfdi2layer/prop_sfdi.dat
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/sfdi2layer/sfdi.inp
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/sharing/README.txt
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/sharing/prop_sharing.dat
mmc-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmc-demos/examples/sharing/sharing.inp
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/validation/prop_cube.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/validation/prop_cube2.dat
mmc-demos.noarch: W: spurious-executable-perm /usr/share/doc/mmc-demos/examples/widedet/prop_widedet.dat
octave-mmclab-debugsource.x86_64: W: invalid-url URL: http://mcx.space/#mmc <urlopen error [Errno -2] Name or service not known>
octave-mmclab.x86_64: W: invalid-url URL: http://mcx.space/#mmc <urlopen error [Errno -2] Name or service not known>
octave-mmclab.x86_64: W: dangerous-command-in-%preun mv
mmc.x86_64: W: spelling-error Summary(en_US) datasets -> data sets, data-sets, databases
mmc.x86_64: W: invalid-url URL: http://mcx.space/#mmc <urlopen error [Errno -2] Name or service not known>
mmc.x86_64: W: no-manual-page-for-binary mmc
octave-mmclab-debuginfo.x86_64: W: invalid-url URL: http://mcx.space/#mmc <urlopen error [Errno -2] Name or service not known>
mmclab-demos.noarch: W: spelling-error Summary(en_US) datasets -> data sets, data-sets, databases
mmclab-demos.noarch: W: spelling-error %description -l en_US datasets -> data sets, data-sets, databases
mmclab-demos.noarch: W: invalid-url URL: http://mcx.space/#mmc <urlopen error [Errno -2] Name or service not known>
mmclab-demos.noarch: W: wrong-file-end-of-line-encoding /usr/share/doc/mmclab-demos/example/demo_sfdi_2layer.m
6 packages and 0 specfiles checked; 0 errors, 40 warnings.



Source checksums
----------------
https://github.com/fangq/mmc/archive/v1.7.9/mmc-1.7.9.tar.gz :
  CHECKSUM(SHA256) this package     : b5533a639a0efe7de2785358c6b2ef1ba2228f00fffd35faf9fd0b42f8040e64
  CHECKSUM(SHA256) upstream package : b5533a639a0efe7de2785358c6b2ef1ba2228f00fffd35faf9fd0b42f8040e64


Requires
--------
octave-mmclab (rpmlib, GLIBC filtered):
    /bin/sh
    libOpenCL.so.1()(64bit)
    libOpenCL.so.1(OPENCL_1.0)(64bit)
    libOpenCL.so.1(OPENCL_1.2)(64bit)
    libc.so.6()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libgomp.so.1()(64bit)
    libgomp.so.1(GOMP_1.0)(64bit)
    libgomp.so.1(GOMP_4.0)(64bit)
    libgomp.so.1(OMP_1.0)(64bit)
    libm.so.6()(64bit)
    libpthread.so.0()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    octave
    octave-iso2mesh
    opencl-filesystem
    rtld(GNU_HASH)

mmclab-demos (rpmlib, GLIBC filtered):
    octave
    octave-iso2mesh
    octave-mmclab

mmc (rpmlib, GLIBC filtered):
    libOpenCL.so.1()(64bit)
    libOpenCL.so.1(OPENCL_1.0)(64bit)
    libOpenCL.so.1(OPENCL_1.2)(64bit)
    libc.so.6()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libgomp.so.1()(64bit)
    libgomp.so.1(GOMP_1.0)(64bit)
    libgomp.so.1(GOMP_4.0)(64bit)
    libgomp.so.1(OMP_1.0)(64bit)
    libm.so.6()(64bit)
    libpthread.so.0()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    octave
    octave-iso2mesh
    opencl-filesystem
    rtld(GNU_HASH)

mmc-demos (rpmlib, GLIBC filtered):
    octave
    octave-iso2mesh

octave-mmclab-debuginfo (rpmlib, GLIBC filtered):

octave-mmclab-debugsource (rpmlib, GLIBC filtered):



Provides
--------
octave-mmclab:
    octave-mmclab
    octave-mmclab(x86-64)

mmclab-demos:
    mmclab-demos

mmc:
    mmc
    mmc(x86-64)

mmc-demos:
    mmc-demos

octave-mmclab-debuginfo:
    debuginfo(build-id)
    octave-mmclab-debuginfo
    octave-mmclab-debuginfo(x86-64)

octave-mmclab-debugsource:
    octave-mmclab-debugsource
    octave-mmclab-debugsource(x86-64)



Diff spec file in url and in SRPM
---------------------------------
--- /home/asinha/dump/fedora-review/1760617-octave-mmclab/srpm/octave-mmclab.spec	2019-10-13 18:11:35.017723858 +0100
+++ /home/asinha/dump/fedora-review/1760617-octave-mmclab/srpm-unpacked/octave-mmclab.spec	2019-10-11 00:04:24.000000000 +0100
@@ -6,9 +6,9 @@
 Version:        1.7.9
 Release:        1%{?dist}
-Summary:        A GPU mesh-based Monte Carlo photon simulator for MATLAB/Octave
+Summary:        A GPU Mesh-based Monte Carlo photon simulator for MATLAB/Octave
 License:        GPLv3+
 URL:            http://mcx.space/#mmc
 Source0:        https://github.com/fangq/%{project}/archive/v%{version}/%{project}-%{version}.tar.gz
-BuildRequires:  octave-devel gcc-c++ vim-common opencl-headers ocl-icd-devel
+BuildRequires:  octave-devel gcc-c++  vim-common opencl-headers ocl-icd-devel
 
 Requires:       octave opencl-filesystem octave-iso2mesh
@@ -28,5 +28,5 @@
 
 %package -n %{octpkg}-demos
-Summary:        Example datasets and scripts for MMCLAB toolbox
+Summary:        Example datasets and scripts for the MMCLAB toolbox
 BuildArch:      noarch
 Requires:       octave octave-%{octpkg} octave-iso2mesh
@@ -36,5 +36,5 @@
 
 %package -n %{project}
-Summary:        Example datasets and scripts for mesh-based Monte Carlo (MMC)
+Summary:        Example datasets and scripts for the MMCLAB toolbox
 BuildRequires:  octave-devel gcc-c++  vim-common opencl-headers ocl-icd-devel
 Requires:       octave opencl-filesystem octave-iso2mesh
@@ -146,6 +146,8 @@
 rm %{octpkg}/*.txt
 mv %{octpkg}/example .
+mv %{octpkg}/*.mex .
 mv %{octpkg} inst
 mv src/Makefile .
+ln -s %{_libexecdir}/%{octpkg}/%{branch}.mex inst/%{branch}.mex
 %octave_pkg_build
 
@@ -165,6 +167,9 @@
 %octave_pkg_install
 
+install -m 0755 -vd  %{buildroot}%{_libexecdir}/%{octpkg}
+install -m 0755 -vp  %{branch}.mex %{buildroot}%{_libexecdir}/%{octpkg}/
+
 install -m 0755 -vd %{buildroot}%{_bindir}
-install -m 0755 -vt %{buildroot}%{_bindir} bin/%{project}
+install -m 0755 -t %{buildroot}%{_bindir} bin/%{project}
 
 %post
@@ -180,4 +185,6 @@
 %license LICENSE.txt
 %doc README.txt AUTHORS.txt
+%dir %{_libexecdir}/%{octpkg}
+%{_libexecdir}/%{octpkg}/*.mex
 %dir %{octpkgdir}
 %{octpkgdir}/*.m


Generated by fedora-review 0.7.3 (44b83c7) last change: 2019-09-18
Command line :/usr/bin/fedora-review -b 1760617
Buildroot used: fedora-rawhide-{{ target_arch }}
Active plugins: C/C++, Shell-api, Generic
Disabled plugins: Java, Python, Perl, fonts, PHP, R, SugarActivity, Ocaml, Haskell
Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH

Comment 9 Qianqian Fang 2019-10-14 16:32:59 UTC
Spec URL: https://raw.githubusercontent.com/fangq/fedorapkg/mmclab/mmc.spec
SRPM URL: https://kwafoo.coe.neu.edu/~fangq/share/temp/mmc-1.7.9-1.fc30.src.rpm
Description: 

MMCLAB is the native MEX version of MMC - Mesh-based Monte Carlo - for
MATLAB and GNU Octave. By converting the input and output files into
convenient in-memory variables, MMCLAB is very intuitive to use and
straightforward to be integrated with mesh generation and post-simulation
analyses. Because MMCLAB contains the same computational codes for
OpenCL-based photon simulation as in a MMC binary, running MMCLAB
inside MATLAB is expected to give similar speed as running a standalone
MMC binary using either a CPU or a GPU.

Fedora Account System Username: fangq

Comment 10 Qianqian Fang 2019-10-14 16:53:49 UTC
@Ankur

thanks for the feedback, my updated spec file and srpm file are updated in the above post

here are my replies to some of the top issues you spotted, let me know what you think.


-------------------------------------------------------------------------
> Should the main spec/package be simply called mmc and then octave-.. as
> sub-packages instead of the other way round?

done, the main package is now mmc, and octave-mmclab etc are now subpkgs

-------------------------------------------------------------------------
> This confirms that the generated srpm does not match the latest spec. Please
> post the newest ones.

sorry, I must have forgotten to update the srpm previously. Now, the new srpm can be found int he above post.

-------------------------------------------------------------------------
> Please remove the executable perm

all the permission and end-of-line issues are now corrected in the upstream, and I made a new upstream package to create the rpms

https://github.com/fangq/mmc/commit/2139721339907362f269562540b2d63ef88f0561

I confirm that these issues are fixed in the new srpm

-------------------------------------------------------------------------
> Lots of bundling here too. Is this required?


this case is not bundling - the licenses found by the licensecheck are in the source-code files. They are all compatible with the final binary's license (GPLv3+) according to

https://www.gnu.org/licenses/license-list.en.html#GPLCompatibleLicenses

as required by GPL license, once all these source units are compiled and linked into a binary, the final generated software shall be licensed under GPL

https://www.gnu.org/licenses/gpl-faq.en.html#MereAggregation

so, as long as the licenses of the source units are compatible with GPL, we should label the final executable/package as GPL. So I believe what I did here is correct.

-------------------------------------------------------------------------
here are some of the minor comments:

> Why does the build require vim-common?

I use the xxd utility from vim-common to convert the OpenCL kernel file *.cl to .h file in order to compile it in the binary; this is similar to the case in mcxlab (Bug 1758622).
-------------------------------------------------------------------------
> Please move the docs to the a sub-package.

I've already put all mmc/examples into mmc-demos and mmc/mmclab/example to mmclab-demos. 
-------------------------------------------------------------------------
> - Build flags aren't used. Example from the build.log file:
> Please use %{set_opt_flags} before you run %make_build

done
-------------------------------------------------------------------------
> You do not need to use pushd, you can use %make_build -C <directory name>

done
-------------------------------------------------------------------------
> There's still the ln -s mex etc. Is this the latest srpm? (Each time you
> update the spec file, you need to regenerate the srpm and upload the latest
> versions of both.)

I believe this was a result of obsolete srpm files. please run it again with the new srpm.

-------------------------------------------------------------------------
> [!]: Fully versioned dependency in subpackages if applicable.
>     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in mmclab-
>     demos , mmc , mmc-demos
> Do the Requires be versioned?

I think it is fine. 
this is the initial package, all the features in the demos are supported by this version. In the future, if some demos requires newer versions, I will add version in Requires.
-------------------------------------------------------------------------

Comment 11 Orion Poplawski 2019-10-16 01:42:13 UTC
Please drop %{project}, just use %{name}.

Comment 12 Qianqian Fang 2019-10-16 18:58:33 UTC
@Orion - agree, the redundant macro is now removed

https://github.com/fangq/fedorapkg/commit/873fba99c50592bd1da840bab3fb489ccc2fbd7b

@Ankur - I also manually added the build flags, as my Makefile does not use the standard flags, see

https://github.com/fangq/fedorapkg/commit/873fba99c50592bd1da840bab3fb489ccc2fbd7b#diff-22ce4fe606fb9f0c2db4bcb72337bc66R147

both the spec and srpm files are updated.

let me know if this can be approved. thanks

Comment 13 Qianqian Fang 2019-10-18 15:05:12 UTC
@Ankur, let me know if you have any further feedback regarding the spec file. thanks

Comment 14 Ankur Sinha (FranciscoD) 2019-10-21 12:04:20 UTC
Running it through Fedora review again now.

(In reply to Qianqian Fang from comment #10)
> -------------------------------------------------------------------------
> > Lots of bundling here too. Is this required?
> 
> 
> this case is not bundling - the licenses found by the licensecheck are in
> the source-code files. They are all compatible with the final binary's
> license (GPLv3+) according to
> 
> https://www.gnu.org/licenses/license-list.en.html#GPLCompatibleLicenses
> 
> as required by GPL license, once all these source units are compiled and
> linked into a binary, the final generated software shall be licensed under
> GPL
> 
> https://www.gnu.org/licenses/gpl-faq.en.html#MereAggregation
> 
> so, as long as the licenses of the source units are compatible with GPL, we
> should label the final executable/package as GPL. So I believe what I did
> here is correct.
> 


No, you misunderstand. Bundling is not only about the compatibility of licenses these tools are provided under. That's a different question. 

The question w.r.t bundling here is: why are we carrying copies of these external libraries in the sources and building individual copies of them especially for this package? SFMT, for example is available here: http://www.math.sci.hiroshima-u.ac.jp/~m-mat/MT/SFMT/  So, what we should try to do is package SFMT separately, and use it as a BuildRequires in all packages that use it. This has also come up in other reviews. Bundling is not forbidden any more, but reasonable effort should be made to avoid it. We really do not want different packages each carrying their own copies of libraries---it greatly increases the maintenance burden, and of course, if all packages carry their own private copies of libraries, there is a high likelihood that they'll deviate from upstream's code (and the currently supported version). As package maintainers, we try to work with upstream (here, that is you :)) to help them use the latest versions of general libraries (which are generally available in Fedora).

This applies to: SFMT, cjson, and waitmex. They appear to be general purpose libraries/tools. Is there some reason they cannot be packaged as separate packages and used as BuildRequires?

The following documents are relevant:

- https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling -> if at all possible, we use unbundled versions, otherwise we clearly indicate what versions are bundled
- https://fedoraproject.org/wiki/Staying_close_to_upstream_projects

Cheers,

Comment 15 Qianqian Fang 2019-10-21 19:23:24 UTC
@Ankur

I agree with you that it makes software more modular and scalable if all dependencies can be built as separate packages. However, I think the scenario here is slightly different.

the 4 external components that mmc depends on, namely, sse_math, waitmex, cjson and SFMT, are extremely lightweight units, and are designed primarily for embedded use. For example

- sse_math is a header-only single-unit header file
- waitmex and cJSON are both single-unit (waitmex.c/.h, cJSON.c/.h) libraries
- SFMT is also a single unit (SFMT.c with multiple header files) library 

if you search on github, the majority of the appearance of SFMT are for embedded use, similar to mmc, 

https://github.com/search?q=SFMT.h&type=Code

same for cJSON

https://github.com/search?q=cjson&type=Code

in fact, the embedded use of these utilities are seen as an advantage by the authors of these tools, and they intentionally designed the unit as portable and lightweight as possible to enable such use, and also to ensure such by using a more permissive license (BSD, MIT, ZLIB). 

In a way, I am permitted, by the respective licenses, to convert each of the cited units into GPL and place a uniform license disclaimers in each of the units, so that it looks more like an integrated/single-licensed source-code tree, but I prefer not to do that because I have to make such edits again if I want to sync some of the units to new releases. I also thought it would give upstream authors more visibility by keeping their original disclaimers in the source codes; but I did not expect this visibility to turn into a restriction for me to distribute the final software in the license that I desire (which is known to be compliant with the upstream licenses).


again, I am not against packaging these lightweight libraries separately (I am happy to help), but it is not as beneficial in the case of mmc. Because these libraries are generally not available even in open-source distributions, not mention about windows/mac where the majority of mmc users use, to make mmc dependent to such libraries makes the code hard to compile for a large portion of my existing users. I will also have to deal with future changes of the API interfaces and data structures that may happen independent to my codes. So, even we can create separate packages for these utilities, I still prefer my code to embed these lightweight units for portability - perhaps in the future if these libraries become widely available like libz, libblas or libOpenCL, then, I may consider switching to dynamic linking mode.


let me know if this makes sense to you.

Comment 16 Qianqian Fang 2019-10-21 23:41:25 UTC
to add another clarification - mmc uses these libraries internally for file IO of specific inputs and computations purposes, and has no public interface to call these libraries externally and allow to use them individually. You can treat them as private/internal functions (compiled in a single GPL-licensed binary), not a way to encapsulate/ship the libraries per se to allow reuse - from this perspective, I felt that calling it "bundling" is not exactly accurate.

Comment 17 Ankur Sinha (FranciscoD) 2019-10-22 09:41:01 UTC
(In reply to Qianqian Fang from comment #16)
> to add another clarification - mmc uses these libraries internally for file
> IO of specific inputs and computations purposes, and has no public interface
> to call these libraries externally and allow to use them individually. You
> can treat them as private/internal functions (compiled in a single
> GPL-licensed binary), not a way to encapsulate/ship the libraries per se to
> allow reuse - from this perspective, I felt that calling it "bundling" is
> not exactly accurate.

Replying to this first.

No, bundling simply refers to "not using system versions of libraries". How you use them, or whether or not they are made available by your package is not a factor here. The point is simply that you are using a private version of these libraries. The guidelines should clarify this:

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

Now on to the second part:

(In reply to Qianqian Fang from comment #15)
> @Ankur
> 
> I agree with you that it makes software more modular and scalable if all
> dependencies can be built as separate packages. However, I think the
> scenario here is slightly different.

No, again---this is not why bundling is avoided. Avoiding bundling simply results in software being more modular. The main issue we are trying to avoid is explained better with the help of an example, mmc here:

- mmc is bundling SFMT
- The current release of SFMT is 5.1 from 2017

Now, since mmc is bundling this SFMT version, as SFMT moves forward with bugfixes/optimisations/enhancements, mmc does not receive any of these. In fact, since we don't know the version of SFMT in use in mmc (I can't find any version information in the code somehow), we cannot say for sure if the SFMT developers are still supporting it. So, if a user finds a bug in the bundled version of SFMT, who then looks into it? If mmc modifies SFMT, then you have effectively forked an older version of SFMT and now responsible for maintaining it---separate from SFMT upstream.


> 
> the 4 external components that mmc depends on, namely, sse_math, waitmex,
> cjson and SFMT, are extremely lightweight units, and are designed primarily
> for embedded use. For example
> 
> - sse_math is a header-only single-unit header file
> - waitmex and cJSON are both single-unit (waitmex.c/.h, cJSON.c/.h) libraries
> - SFMT is also a single unit (SFMT.c with multiple header files) library 
> 
> if you search on github, the majority of the appearance of SFMT are for
> embedded use, similar to mmc, 
> 
> https://github.com/search?q=SFMT.h&type=Code

That does not make it the right practice from a software development point of view---and the point of the review process is to ensure that we follow software development best practices while adding software to Fedora. (A goal of the NeuroFedora group is also to try and push these software development standards upstream to academics who write software but may not necessarily be trained in software development.)

> 
> same for cJSON
> 
> https://github.com/search?q=cjson&type=Code
> 
> in fact, the embedded use of these utilities are seen as an advantage by the
> authors of these tools, and they intentionally designed the unit as portable
> and lightweight as possible to enable such use, and also to ensure such by
> using a more permissive license (BSD, MIT, ZLIB). 

If that's the case, then these developers are also not following suggested software development best practices. It is unfortunate. Embedding libraries has more disadvantages than advantages.

> 
> In a way, I am permitted, by the respective licenses, to convert each of the
> cited units into GPL and place a uniform license disclaimers in each of the
> units, so that it looks more like an integrated/single-licensed source-code
> tree, but I prefer not to do that because I have to make such edits again if
> I want to sync some of the units to new releases. I also thought it would
> give upstream authors more visibility by keeping their original disclaimers
> in the source codes; but I did not expect this visibility to turn into a
> restriction for me to distribute the final software in the license that I
> desire (which is known to be compliant with the upstream licenses).

The licensing is fine.

> 
> 
> again, I am not against packaging these lightweight libraries separately (I
> am happy to help), but it is not as beneficial in the case of mmc. Because
> these libraries are generally not available even in open-source
> distributions, not mention about windows/mac where the majority of mmc users
> use, to make mmc dependent to such libraries makes the code hard to compile
> for a large portion of my existing users. I will also have to deal with
> future changes of the API interfaces and data structures that may happen
> independent to my codes. 

But isn't this a good thing? As the developer, don't you want to use the latest versions of these libraries? What is the advantage of limiting your tool to older versions, (apart from it being convenient)? If the concern is that SFMT etc are not available on most installations, and bundling helps here, can we at least:

- version the bundled libraries so we know what snapshot of these in time we're depending on, and 
- maybe try to always bundle the latest versions?

> So, even we can create separate packages for these
> utilities, I still prefer my code to embed these lightweight units for
> portability

This is fine if you want to distribute pre-built binaries directly from Github, but isn't quite relevant when it comes to packaging for distributions. The common way upstreams go about it is by providing an easy compilation option where one can choose between the bundled version of the library or a system version.

>  - perhaps in the future if these libraries become widely
> available like libz, libblas or libOpenCL, then, I may consider switching to
> dynamic linking mode.

This is a catch-22. If everyone keeps bundling them, there will be no incentive to make them available in distributions XD

 
> let me know if this makes sense to you.

Sure, I understand where you are coming from. As I've said, bundling is not forbidden anymore---it used to be. It is up to you as developer + package maintainer to decide if bundling is the way to go. At the same time, though, it is our role as reviewers to have this discussion to see if bundling can be avoided.


If we're going ahead with bundling, please follow the guidelines and include the required Provides: .. statements in the spec.:
https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling

Comment 18 Qianqian Fang 2019-10-22 16:27:25 UTC
@Ankur

your points are taken - I have no objection to the current policy regarding bundling: if a system library already exists and it makes sense to dynamically link to it to avoid duplication, but embedding open-source codes/units in a larger project is also a very common practice and there are even dedicated tools to facilitate such use (such as git submodule, or header-only libraries).

I understand your standing point of pushing reusable libraries in the context of building a healthy, modular and dynamic distribution/software environment, but as a software developer and distributor, I also have to pay extensive attention to 1) portability - i.e. to ensure smooth/easy software compilation not only on a specific distribution, but other platforms including Windows/Mac, 2) stability - including both forward and backward compatibility that are independent to external library interface changes, 3) performance - some of the libraries, such as sse_math and SFMT, are primarily used for their "inline" functions that are important for the software performance/speed, unless the library was specifically coded to ensure inlined functions, moving towards dynamic linking will likely result in speed loss.

again, your points were loud and clear, but my assessment, as an upstream author and packger, is that the suggestion for decoupling the embedded libraries for separate packages and dynamic link with those will require a lot more work but no clear benefit (or even detrimental in terms of speed) for the software itself. So, I prefer to move forward in the current package/source code settings.

Regarding the suggested "Provides: bundled(...)" line, I just want to make sure - this package neither compiles nor installs private copies of separate library files for sse_math/sfmt/json; it only statically compiles/links with them in our mmc/mmc.mex binaries. No one could directly call the functions in these embedded libraries from the generated binaries (mmc/mmc.mex). 

In this case, is a "Provide" directive still appropriate?

Comment 19 Orion Poplawski 2019-10-23 01:58:50 UTC
I don't see how using packaged versions of static/header libraries could be detrimental.

The roles of the "Provides: bundled()" idiom is to determine who makes use of a library in any way.  The primary use is in case of a security issue with a library - how would you determine what packages would need to get fixed?  For static libraries you would look at the BuildRequires.  But if a package uses a bundled version, how would you tell?  By examining the Provides: bundled() items.

Comment 20 Qianqian Fang 2019-10-23 03:51:22 UTC
ok, I added the "Provides:bundled()" statements, see

https://github.com/fangq/fedorapkg/commit/b86488daa0f88931ce15011de13ac898580592d1

but I want to mention that neither "ssemath" nor "cjson" currently exist in Fedora, is this a problem? rpmlint only complained about missing versions in provides.

I confirm that SFMT is currently not compiled in this package - it will only be included when I use "%make_build -f makefile_sfmt", which is not the current building option.

please let me know if you see any other places that I need to update. both the .spec and srpm files were updated with URL unchanged.

Comment 21 Ankur Sinha (FranciscoD) 2019-10-28 09:25:59 UTC
Looks pretty good. A few issues are noted below. Once clarified, this can be
approved.


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

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


Issues:
=======
- Package installs properly.
  Note: Installation errors (see attachment)
  See: https://docs.fedoraproject.org/en-US/packaging-guidelines/
^
Please check this---fedora review failed to install the packages.

- Large documentation must go in a -doc subpackage. Large could be size
  (~1MB) or number of files.
  Note: Documentation size is 5703680 bytes in 190 files.
  See: https://docs.fedoraproject.org/en-US/packaging-
  guidelines/#_documentation

^
This is in a demos sub-package so that's OK.


- octave-mmc has a mex file, so it should not be noarch, should it. Please
  check. The files should go to %octpkglibdir instead of %octpkgdir too.

- Why is vim-common a BuildRequires?

- Do we know what versions of libraries are being bundled here? If so, please
  version the Provides commands for them.

- if SFMT is not being used, please delete it's source directory in %prep. This
  is done to ensure that it does not get enabled by mistake during a rebuild
  etc.

- Cosmetic: you don't need to repeat BuildRequires in the sub-package
  definitions.

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

C/C++:
[x]: 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]: If your application is a C or C++ application you must list a
     BuildRequires against gcc, gcc-c++ or clang.
[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. No licenses found. Please check the source files for
     licenses manually.
[x]: License file installed when any subpackage combination is installed.
[x]: Package does not own files or directories owned by other packages.
[x]: %build honors applicable compiler flags or justifies otherwise.
^
The build log has lots of compiler warnings, though. Please take a look.

[-]: 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.
[-]: 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.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[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 must not depend on deprecated() packages.
[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.
[?]: Package functions as described.
^
Not tested

[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Scriptlets must be sane, if used.
[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
     Note: gpgverify is not used.
[-]: 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.
[x]: 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]: 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:
[!]: Rpmlint is run on all installed packages.
     Note: Mock build failed
     See: https://docs.fedoraproject.org/en-US/packaging-
     guidelines/#_use_rpmlint
[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.


Installation errors
-------------------
INFO: mock.py version 1.4.20 starting (python version = 3.7.5)...
Start: init plugins
INFO: selinux enabled
Finish: init plugins
INFO: Signal handler active
Start: run
Start: chroot init
INFO: calling preinit hooks
INFO: enabled root cache
INFO: enabled dnf cache
Start: cleaning dnf metadata
Finish: cleaning dnf metadata
INFO: enabled HW Info plugin
Mock Version: 1.4.20
INFO: Mock Version: 1.4.20
Finish: chroot init
INFO: installing package(s): /home/asinha/dump/fedora-review/1760617-mmc/results/mmc-debugsource-1.7.9-1.fc32.x86_64.rpm /home/asinha/dump/fedora-review/1760617-mmc/results/mmc-debuginfo-1.7.9-1.fc32.x86_64.rpm /home/asinha/dump/fedora-review/1760617-mmc/results/octave-mmclab-1.7.9-1.fc32.x86_64.rpm /home/asinha/dump/fedora-review/1760617-mmc/results/mmc-1.7.9-1.fc32.x86_64.rpm /home/asinha/dump/fedora-review/1760617-mmc/results/mmclab-demos-1.7.9-1.fc32.noarch.rpm /home/asinha/dump/fedora-review/1760617-mmc/results/mmc-demos-1.7.9-1.fc32.noarch.rpm
ERROR: Command failed: 
 # /usr/bin/dnf --installroot /var/lib/mock/fedora-rawhide-x86_64/root/ --releasever 32 --setopt=deltarpm=False --allowerasing --disableplugin=local --disableplugin=spacewalk install /home/asinha/dump/fedora-review/1760617-mmc/results/mmc-debugsource-1.7.9-1.fc32.x86_64.rpm /home/asinha/dump/fedora-review/1760617-mmc/results/mmc-debuginfo-1.7.9-1.fc32.x86_64.rpm /home/asinha/dump/fedora-review/1760617-mmc/results/octave-mmclab-1.7.9-1.fc32.x86_64.rpm /home/asinha/dump/fedora-review/1760617-mmc/results/mmc-1.7.9-1.fc32.x86_64.rpm /home/asinha/dump/fedora-review/1760617-mmc/results/mmclab-demos-1.7.9-1.fc32.noarch.rpm /home/asinha/dump/fedora-review/1760617-mmc/results/mmc-demos-1.7.9-1.fc32.noarch.rpm --setopt=tsflags=nocontexts



Rpmlint
-------
Checking: mmc-1.7.9-1.fc32.x86_64.rpm
          octave-mmclab-1.7.9-1.fc32.x86_64.rpm
          mmclab-demos-1.7.9-1.fc32.noarch.rpm
          mmc-demos-1.7.9-1.fc32.noarch.rpm
          mmc-debuginfo-1.7.9-1.fc32.x86_64.rpm
          mmc-debugsource-1.7.9-1.fc32.x86_64.rpm
          mmc-1.7.9-1.fc32.src.rpm
mmc.x86_64: W: no-manual-page-for-binary mmc
octave-mmclab.x86_64: E: arch-dependent-file-in-usr-share /usr/share/octave/packages/mmclab-1.7.9/mmcl.mex
octave-mmclab.x86_64: W: dangerous-command-in-%preun mv
mmclab-demos.noarch: W: spelling-error Summary(en_US) datasets -> data sets, data-sets, databases
mmclab-demos.noarch: W: spelling-error %description -l en_US datasets -> data sets, data-sets, databases
mmc-demos.noarch: W: spelling-error Summary(en_US) datasets -> data sets, data-sets, databases
mmc-demos.noarch: W: spelling-error %description -l en_US datasets -> data sets, data-sets, databases
mmc.src:15: W: unversioned-explicit-provides bundled(ssemath)
mmc.src:16: W: unversioned-explicit-provides bundled(cjson)
7 packages and 0 specfiles checked; 1 errors, 8 warnings.




Source checksums
----------------
https://github.com/fangq/mmc/archive/v1.7.9/mmc-1.7.9.tar.gz :
  CHECKSUM(SHA256) this package     : 5c30e86d60cae5d9233165ed50684ae7bb37b5a503266c3aceacd07fca3c85fd
  CHECKSUM(SHA256) upstream package : 5c30e86d60cae5d9233165ed50684ae7bb37b5a503266c3aceacd07fca3c85fd


Requires
--------
mmc (rpmlib, GLIBC filtered):
    libOpenCL.so.1()(64bit)
    libOpenCL.so.1(OPENCL_1.0)(64bit)
    libOpenCL.so.1(OPENCL_1.2)(64bit)
    libc.so.6()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libgcc_s.so.1(GCC_3.3.1)(64bit)
    libgomp.so.1()(64bit)
    libgomp.so.1(GOMP_1.0)(64bit)
    libgomp.so.1(GOMP_4.0)(64bit)
    libgomp.so.1(OMP_1.0)(64bit)
    libm.so.6()(64bit)
    libpthread.so.0()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    octave
    octave-iso2mesh
    opencl-filesystem
    rtld(GNU_HASH)

octave-mmclab (rpmlib, GLIBC filtered):
    /bin/sh
    libOpenCL.so.1()(64bit)
    libOpenCL.so.1(OPENCL_1.0)(64bit)
    libOpenCL.so.1(OPENCL_1.2)(64bit)
    libc.so.6()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libgcc_s.so.1(GCC_3.3.1)(64bit)
    libgomp.so.1()(64bit)
    libgomp.so.1(GOMP_1.0)(64bit)
    libgomp.so.1(GOMP_4.0)(64bit)
    libgomp.so.1(OMP_1.0)(64bit)
    libm.so.6()(64bit)
    libpthread.so.0()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    octave
    octave-iso2mesh
    opencl-filesystem
    rtld(GNU_HASH)

mmclab-demos (rpmlib, GLIBC filtered):
    octave
    octave-iso2mesh
    octave-mmclab

mmc-demos (rpmlib, GLIBC filtered):
    octave
    octave-iso2mesh

mmc-debuginfo (rpmlib, GLIBC filtered):

mmc-debugsource (rpmlib, GLIBC filtered):



Provides
--------
mmc:
    bundled(cjson)
    bundled(ssemath)
    mmc
    mmc(x86-64)

octave-mmclab:
    octave-mmclab
    octave-mmclab(x86-64)

mmclab-demos:
    mmclab-demos

mmc-demos:
    mmc-demos

mmc-debuginfo:
    debuginfo(build-id)
    mmc-debuginfo
    mmc-debuginfo(x86-64)

mmc-debugsource:
    mmc-debugsource
    mmc-debugsource(x86-64)



Generated by fedora-review 0.7.3 (44b83c7) last change: 2019-09-18
Command line :/usr/bin/fedora-review -b 1760617
Buildroot used: fedora-rawhide-{{ target_arch }}
Active plugins: C/C++, Generic, Shell-api
Disabled plugins: Perl, R, SugarActivity, PHP, Java, Python, Haskell, Ocaml, fonts
Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH

Comment 22 Ankur Sinha (FranciscoD) 2019-10-28 09:27:02 UTC
Created attachment 1629741 [details]
Build log showing compiler warnings

Build log is attached for your information. While these warnings do not need to be fixed for this review, it's worth looking into them.

Comment 23 Qianqian Fang 2019-10-28 17:14:49 UTC
thanks for the review.

the updated spec file can be found at

https://raw.githubusercontent.com/fangq/fedorapkg/mmclab/mmc.spec

but it does not build any more. the issue is related to how to "%octpkglibdir". I would like to know how to fix this.


> Please check this---fedora review failed to install the packages.

I managed to reproduce this issue using mock, see the mock log attached at the bottom.

basically, the issue is that mmc/octave-mmclab has a dependency to octave-iso2mesh, but it complains libmpfr.so.4 could not be installed. I found that this .so file is part of mpfr package.

I already listed mpfr-devel in octave-iso2mesh's spec file, and from the mpfr's spec, it the devel package depends on the main package, so assuming that both mpfr and mpfr-devel should have been installed for octave-iso2mesh. but somehow, they were not.

can you let me know if this is a problem for iso2mesh? or I did not use mock properly?


> octave-mmc has a mex file, so it should not be noarch, should it. Please
> check. The files should go to %octpkglibdir instead of %octpkgdir too.

I did not specify BuildArch for octave-mmclab. From the generated rpms, it seems is not noarch. the only two noarch packages are mmc-demos and mmclab-demos, can you verify if this is the case?

I tried to follow the arch-specific octave template 

https://fedoraproject.org/wiki/Packaging:Octave#Arch_specific_Octave_spec_template

and made the follow change, but rpmbuild failed, complaining that directory not found. is this the right way to add %{octpkglibdir}? I also removed %dir as the template showed, then it complains both file missing and dir missing.

error: Directory not found: /home/fangq/rpmbuild/BUILDROOT/mmc-1.7.9-1.fc30.x86_64/usr/lib64/octave/packages/mmclab-1.7.9

-------------------------------------
diff --git a/mmc.spec b/mmc.spec
index ca632bd..5a7c194 100644
--- a/mmc.spec
+++ b/mmc.spec
@@ -196,7 +196,7 @@ install -m 0755 -pt %{buildroot}%{_bindir} bin/%{name}
 %doc README.txt AUTHORS.txt
 %dir %{octpkgdir}
 %{octpkgdir}/*.m
-%{octpkgdir}/*.mex
+%dir %{octpkglibdir}
 %doc %{octpkgdir}/doc-cache
 %{octpkgdir}/packinfo
----------------------------------------

> Why is vim-common a BuildRequires?

see my earlier replies. I need the "xxd" command in vim-common to preprocess the OpenCL kernel. so, building mmc requires this utility.

> Do we know what versions of libraries are being bundled here? If so, please
> version the Provides commands for them.

the cjson I have is from 2015-12-20, which is older than v0.0.0 in the github, so I listed 0.0 in the version; the ssemath's website has a version updated in 2008, which is the one I used. Because it is not version controlled, I also listed 0.0.

https://github.com/fangq/fedorapkg/commit/26960e9fc798068a49bb54e0ecdc55f6f7685083

> if SFMT is not being used, please delete it's source directory in %prep. This
> is done to ensure that it does not get enabled by mistake during a rebuild
> etc.


done
https://github.com/fangq/fedorapkg/commit/d32719a7245e3f49f445d7eaca059fff47018b06


> Cosmetic: you don't need to repeat BuildRequires in the sub-package
> definitions.

done
https://github.com/fangq/fedorapkg/commit/c3e71ef59d37714af0a526f6e27d92807f91b117


-------------------------------------------------
fangq@localhost:/var/lib/mock/fedora-rawhide-x86_64/result$ mock -r fedora-32-x86_64 --install octave-mmclab-debuginfo-1.7.9-1.fc32.x86_64.rpm mmc-debuginfo-1.7.9-1.fc32.x86_64.rpm mmc-debugsource-1.7.9-1.fc32.x86_64.rpm mmc-demos-1.7.9-1.fc32.noarch.rpm mmclab-demos-1.7.9-1.fc32.noarch.rpm octave-mmclab-1.7.9-1.fc32.x86_64.rpm mmc-1.7.9-1.fc32.x86_64.rpm
INFO: mock.py version 1.4.20 starting (python version = 3.7.5)...
Start: init plugins
INFO: selinux enabled
Finish: init plugins
INFO: Signal handler active
Start: run
Start: chroot init
INFO: calling preinit hooks
INFO: enabled root cache
INFO: enabled dnf cache
Start: cleaning dnf metadata
Finish: cleaning dnf metadata
INFO: enabled HW Info plugin
Mock Version: 1.4.20
INFO: Mock Version: 1.4.20
Finish: chroot init
INFO: installing package(s): octave-mmclab-debuginfo-1.7.9-1.fc32.x86_64.rpm mmc-debuginfo-1.7.9-1.fc32.x86_64.rpm mmc-debugsource-1.7.9-1.fc32.x86_64.rpm mmc-demos-1.7.9-1.fc32.noarch.rpm mmclab-demos-1.7.9-1.fc32.noarch.rpm octave-mmclab-1.7.9-1.fc32.x86_64.rpm mmc-1.7.9-1.fc32.x86_64.rpm
No matches found for the following disable plugin patterns: local, spacewalk
fedora                                                                                 29 kB/s |  12 kB     00:00    
Error: 
 Problem 1: package mmc-1.7.9-1.fc32.x86_64 requires octave-iso2mesh, but none of the providers can be installed
  - conflicting requests
  - nothing provides libmpfr.so.4()(64bit) needed by octave-iso2mesh-1.9.1-3.fc32.x86_64
 Problem 2: package octave-mmclab-1.7.9-1.fc32.x86_64 requires octave-iso2mesh, but none of the providers can be installed
  - conflicting requests
  - nothing provides libmpfr.so.4()(64bit) needed by octave-iso2mesh-1.9.1-3.fc32.x86_64
 Problem 3: package mmclab-demos-1.7.9-1.fc32.noarch requires octave-iso2mesh, but none of the providers can be installed
  - conflicting requests
  - nothing provides libmpfr.so.4()(64bit) needed by octave-iso2mesh-1.9.1-3.fc32.x86_64
 Problem 4: package mmc-demos-1.7.9-1.fc32.noarch requires octave-iso2mesh, but none of the providers can be installed
  - conflicting requests
  - nothing provides libmpfr.so.4()(64bit) needed by octave-iso2mesh-1.9.1-3.fc32.x86_64
(try to add '--skip-broken' to skip uninstallable packages or '--nobest' to use not only best candidate packages)
ERROR: Command failed: 
 # /usr/bin/dnf --installroot /var/lib/mock/fedora-rawhide-x86_64/root/ --releasever 32 --setopt=deltarpm=False --allowerasing --disableplugin=local --disableplugin=spacewalk install octave-mmclab-debuginfo-1.7.9-1.fc32.x86_64.rpm mmc-debuginfo-1.7.9-1.fc32.x86_64.rpm mmc-debugsource-1.7.9-1.fc32.x86_64.rpm mmc-demos-1.7.9-1.fc32.noarch.rpm mmclab-demos-1.7.9-1.fc32.noarch.rpm octave-mmclab-1.7.9-1.fc32.x86_64.rpm mmc-1.7.9-1.fc32.x86_64.rpm --setopt=tsflags=nocontexts

Comment 24 Qianqian Fang 2020-01-27 20:00:40 UTC
@ankur, I'd like to reviving this thread, and fix the issue you suggested - can you take a look at my last post and let me know 1) what's the correct way to set octpkglibdir and 2) are other changes sufficient? thanks

Comment 25 Ankur Sinha (FranciscoD) 2020-01-28 08:26:57 UTC
I just realised this was still WIP! Sorry about that! I'll report back in a day or two. It's back on my list now. :)

Comment 26 Ankur Sinha (FranciscoD) 2020-02-03 14:29:43 UTC
(In reply to Qianqian Fang from comment #23)
> thanks for the review.
> 
> the updated spec file can be found at
> 
> https://raw.githubusercontent.com/fangq/fedorapkg/mmclab/mmc.spec
> 
> but it does not build any more. the issue is related to how to
> "%octpkglibdir". I would like to know how to fix this.
> 
> 
> > Please check this---fedora review failed to install the packages.
> 
> I managed to reproduce this issue using mock, see the mock log attached at
> the bottom.
> 
> basically, the issue is that mmc/octave-mmclab has a dependency to
> octave-iso2mesh, but it complains libmpfr.so.4 could not be installed. I
> found that this .so file is part of mpfr package.
> 
> I already listed mpfr-devel in octave-iso2mesh's spec file, and from the
> mpfr's spec, it the devel package depends on the main package, so assuming
> that both mpfr and mpfr-devel should have been installed for
> octave-iso2mesh. but somehow, they were not.
> 
> can you let me know if this is a problem for iso2mesh? or I did not use mock
> properly?

I'm not seeing the dep error anymore. So I'm not sure what was happening here. I don't think iso2mesh should Require: mpfr-devel, but we can discuss that in a different bug (or on the ML).


> > > octave-mmc has a mex file, so it should not be noarch, should it. Please
> > check. The files should go to %octpkglibdir instead of %octpkgdir too.
> 
> I did not specify BuildArch for octave-mmclab. From the generated rpms, it
> seems is not noarch. the only two noarch packages are mmc-demos and
> mmclab-demos, can you verify if this is the case?
> 
> I tried to follow the arch-specific octave template 
> 
> https://fedoraproject.org/wiki/Packaging:
> Octave#Arch_specific_Octave_spec_template
> 
> and made the follow change, but rpmbuild failed, complaining that directory
> not found. is this the right way to add %{octpkglibdir}? I also removed %dir
> as the template showed, then it complains both file missing and dir missing.
>

I've opened a PR with a tweak that just puts the mex file in the right place. The octave macros don't seem to do it---but I will email the ML to confirm.

If you can have a look at it and submit the updated spec/srpm links, I'll re-run fedora-review on everything. I think we're quite close to completion here. :)

Comment 27 Qianqian Fang 2020-02-17 22:20:46 UTC
@Ankur, thanks. I finally got my virtualbox working again. 

I did a few minor changes (re-tagging upstream release, sync source file, and then drop the branch name), now I was able to build mmc/octave-mmclab/mmc-demos/mmclab-demos packages without any error.

Spec URL: https://github.com/fangq/fedorapkg/blob/mmclab/mmc.spec
SRPM URL: https://kwafoo.coe.neu.edu/~fangq/share/temp/mmc-1.7.9-1.fc30.src.rpm

I tested the binary package (mmc) and it works fine. But when testing the octave-mmclab package, for some reason, octave fails to find the mex file.

The .mex file is now located in /usr/lib64/octave/packages/mmclab-1.7.9/mmc.mex after installation, however, when I type "pkg mmclab load" in octave, I could only load the noarch .m files in the package (they are stored in /usr/share/octave/packages/mmclab-1.7.9/mmclab.m), but not the .mex file.

did I miss any step in order to use the .mex file?

thanks

Comment 28 Ankur Sinha (FranciscoD) 2020-02-18 11:38:16 UTC
Can you give me the complete steps on how to check this please? I built it for f31, and it loads OK. What should one do next to verify if the mex file can be used?

(ins)>> pkg list
Package Name  | Version | Installation directory
--------------+---------+-----------------------
    iso2mesh  |   1.9.1 | /usr/share/octave/packages/iso2mesh-1.9.1
      jnifti  |     0.5 | /usr/share/octave/packages/jnifti-0.5
     jsonlab  |   1.9.8 | /usr/share/octave/packages/jsonlab-1.9.8
      mmclab  |   1.7.9 | /usr/share/octave/packages/mmclab-1.7.9
        zmat  |     0.9 | /usr/share/octave/packages/zmat-0.9
(ins)>> pkg load mmclab
(ins)>> pkg mmclab load
(ins)>> pkg list
Package Name  | Version | Installation directory
--------------+---------+-----------------------
    iso2mesh *|   1.9.1 | /usr/share/octave/packages/iso2mesh-1.9.1
      jnifti *|     0.5 | /usr/share/octave/packages/jnifti-0.5
     jsonlab *|   1.9.8 | /usr/share/octave/packages/jsonlab-1.9.8
      mmclab *|   1.7.9 | /usr/share/octave/packages/mmclab-1.7.9
        zmat *|     0.9 | /usr/share/octave/packages/zmat-0.9
(ins)>>


/usr/share/octave/octave_packages correctly lists the package directories, so that looks OK too:



# name: dir
# type: sq_string
# elements: 1
# length: 39
/usr/share/octave/packages/mmclab-1.7.9


# name: archprefix
# type: sq_string
# elements: 1
# length: 39
/usr/lib64/octave/packages/mmclab-1.7.9


I do not, however, see this in the output of path():

(ins)>> path()

Octave's search path contains the following directories:

.
/usr/share/octave/packages/zmat-0.9
/usr/share/octave/packages/jsonlab-1.9.8
/usr/share/octave/packages/jnifti-0.5
/usr/share/octave/packages/iso2mesh-1.9.1
/usr/share/octave/packages/mmclab-1.7.9
/usr/lib64/octave/5.1.0/site/oct/x86_64-redhat-linux-gnu
/usr/lib64/octave/site/oct/api-v53/x86_64-redhat-linux-gnu
/usr/lib64/octave/site/oct/x86_64-redhat-linux-gnu
/usr/share/octave/5.1.0/site/m
/usr/share/octave/site/api-v53/m
/usr/share/octave/site/m
/usr/share/octave/site/m/startup
/usr/lib64/octave/5.1.0/oct/x86_64-redhat-linux-gnu
/usr/share/octave/5.1.0/m
/usr/share/octave/5.1.0/m/audio
/usr/share/octave/5.1.0/m/deprecated
/usr/share/octave/5.1.0/m/elfun
/usr/share/octave/5.1.0/m/general
/usr/share/octave/5.1.0/m/geometry
/usr/share/octave/5.1.0/m/gui
/usr/share/octave/5.1.0/m/help
/usr/share/octave/5.1.0/m/image
/usr/share/octave/5.1.0/m/io
/usr/share/octave/5.1.0/m/java
/usr/share/octave/5.1.0/m/legacy
/usr/share/octave/5.1.0/m/linear-algebra
/usr/share/octave/5.1.0/m/miscellaneous
/usr/share/octave/5.1.0/m/ode
/usr/share/octave/5.1.0/m/optimization
/usr/share/octave/5.1.0/m/path
/usr/share/octave/5.1.0/m/pkg
/usr/share/octave/5.1.0/m/plot
/usr/share/octave/5.1.0/m/plot/appearance
/usr/share/octave/5.1.0/m/plot/draw
/usr/share/octave/5.1.0/m/plot/util
/usr/share/octave/5.1.0/m/polynomial
/usr/share/octave/5.1.0/m/prefs
/usr/share/octave/5.1.0/m/profiler
/usr/share/octave/5.1.0/m/set
/usr/share/octave/5.1.0/m/signal
/usr/share/octave/5.1.0/m/sparse
/usr/share/octave/5.1.0/m/specfun
/usr/share/octave/5.1.0/m/special-matrix
/usr/share/octave/5.1.0/m/startup
/usr/share/octave/5.1.0/m/statistics
/usr/share/octave/5.1.0/m/strings
/usr/share/octave/5.1.0/m/testfun
/usr/share/octave/5.1.0/m/time
/usr/share/octave/5.1.0/data

Comment 29 Qianqian Fang 2020-02-18 13:23:07 UTC
to verify the toolbox, simply type "mmc" and enter in Octave, you should see a short help info.

also, run mmclab('gpuinfo'), this should call mmc.mex and list all supported OpenCL devices.

because 'which mmc' currently does not return anything, so the above two command will not succeed.

Comment 30 Ankur Sinha (FranciscoD) 2020-02-19 10:18:08 UTC
Orion answered the query on the mailing list. I've opened another PR that seems to do the trick. Please have a look. Cheers

```
(ins)>> pkg list
Package Name  | Version | Installation directory
--------------+---------+-----------------------
    iso2mesh  |   1.9.1 | /usr/share/octave/packages/iso2mesh-1.9.1
      jnifti  |     0.5 | /usr/share/octave/packages/jnifti-0.5
     jsonlab  |   1.9.8 | /usr/share/octave/packages/jsonlab-1.9.8
      mmclab  |   1.7.9 | /usr/share/octave/packages/mmclab-1.7.9
        zmat  |     0.9 | /usr/share/octave/packages/zmat-0.9
(ins)>> pkg load mmclab
(ins)>> mmc
Usage:
    [flux,detphoton]=mmclab(cfg);

Please run 'help mmclab' for more details.
(ins)>> mmclab('gpuinfo')
Platform [0] Name Clover
error: mmc: no active GPU device found
error: called from
    mmclab at line 265 column 17
(ins)>> which mmc
'mmc' is a function from the file /usr/lib64/octave/packages/mmclab-1.7.9/x86_64-redhat-linux-gnu-api-v53/mmc.mex
(ins)>>
```

Comment 31 Ankur Sinha (FranciscoD) 2020-02-19 10:26:51 UTC
I forgot to say. Please run a scratch build on koji to test if it builds on all architectures. When I tried it here, it didn't compile on them all.

koji build rawhide <path to srpm> --scratch

Comment 32 Qianqian Fang 2020-02-20 00:18:11 UTC
@Ankur, the error was caused by the lack of support of SSE instructions on ARM, here is part of the log


```
Building built/xorshift128p_rand.o
cc -c -Wall -g -DMCX_EMBED_CL -fno-strict-aliasing -DMMC_USE_SSE -DHAVE_SSE2 -msse -msse2 -msse3 -mssse3 -msse4.1 -O3 -fopenmp  -fPIC -DMCX_CONTAINER -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -march=armv7-a -mfpu=vfpv3-d16 -mtune=generic-armv7-a -mabi=aapcs-linux -mfloat-abi=hard -DUSE_OS_TIMER -DUSE_OPENCL -I../src  -o built/xorshift128p_rand.o  xorshift128p_rand.c
cc1: error: unrecognized command-line option '-msse'
cc1: error: unrecognized command-line option '-msse2'
cc1: error: unrecognized command-line option '-msse3'
cc1: error: unrecognized command-line option '-mssse3'
cc1: error: unrecognized command-line option '-msse4.1'
make: *** [../commons/Makefile_common.mk:188: built/xorshift128p_rand.o] Error 1
```

I can use a conditional flag to let it compile without SSE, but I don't think it is much useful, and requires some additional tweaks of my makefile, so I think the best way moving forward is to exclude arm in this spec for now, and support it for future updates.

I am wondering what is the right way to exclude arch? I added

ExcludeArch:   armv7h1

but does not seem to skip arm at in my koji --scratch build.

Comment 33 Ankur Sinha (FranciscoD) 2020-02-20 12:05:13 UTC
That sounds OK. Please take a look at the guidelines, they explain how such cases should be handled:

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

ExcludeArch: %{arm}

would probably do the trick. As the guidelines say, one must file bugs to document that the package does not support ARM. Additionally, no packages that require this one can be added to ARM either.

Cheers,
Ankur

Comment 34 Qianqian Fang 2020-02-20 17:19:59 UTC
thanks. it was not just arm, but all platforms other than i686 and x86_64. see 

https://koji.fedoraproject.org/koji/taskinfo?taskID=41672272


so I did the following:

1. tweaked my Makefile so that it can compile on non ix86/x86_64 platforms

2. recreated a release, and updated the source file

3. updated the spec file using %ifarch

after this update, I was able to compile on all platforms, see

https://koji.fedoraproject.org/koji/taskinfo?taskID=41698953


However I want to mention that the simulation results on other platforms are not quite correct. I will leave this for further source code updates. For packaging, I think this should be sufficient.

Please let me know what you think. both spec and srpm files are updated.

Spec URL: https://github.com/fangq/fedorapkg/blob/mmclab/mmc.spec
SRPM URL: https://kwafoo.coe.neu.edu/~fangq/share/temp/mmc-1.7.9-1.fc30.src.rpm

Comment 35 Ankur Sinha (FranciscoD) 2020-02-22 20:34:54 UTC
(In reply to Qianqian Fang from comment #34)
> thanks. it was not just arm, but all platforms other than i686 and x86_64.
> see 
> 
> https://koji.fedoraproject.org/koji/taskinfo?taskID=41672272
> 
> 
> so I did the following:
> 
> 1. tweaked my Makefile so that it can compile on non ix86/x86_64 platforms
> 
> 2. recreated a release, and updated the source file
> 
> 3. updated the spec file using %ifarch
> 
> after this update, I was able to compile on all platforms, see
> 
> https://koji.fedoraproject.org/koji/taskinfo?taskID=41698953

That looks good.


 
> However I want to mention that the simulation results on other platforms are
> not quite correct. I will leave this for further source code updates. For
> packaging, I think this should be sufficient.

If the simulation results aren't correct, do we want to provide mmc for these arches (I worry about the scenario where someone does use the tool on these arches and comes up with wrong scientific results)? Maybe excluding them would be the correct thing to do? That's perfectly OK as long as we follow the system and file bugs to record it.

Comment 36 Qianqian Fang 2020-02-22 22:28:19 UTC
My preference is to get the package out first. I will create a bug ticket in my mmc github repo, and track this. there is a special combination of flags can make the results correct, but I will have to educate my users in the documentation.

let me know if you can approve this package. thanks for all the help.

Comment 37 Ankur Sinha (FranciscoD) 2020-02-22 22:46:54 UTC
(In reply to Qianqian Fang from comment #36)
> My preference is to get the package out first. 

Sure, but what I'm saying is that you do *not* have to build the package for all arches for the package to be approved. If upstream (you) do not support all arches, they can be excluded as per the guidelines:

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

"If a Fedora package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch needs to have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture..."

So, all you have to do is file the tracker bugs.


Isn't that better than providing the package for all arches, but it working incorrectly on a few? It's totally your call. If this weren't a scientific tool where correctness is paramount, I wouldn't care so much either :)

Comment 38 Ankur Sinha (FranciscoD) 2020-03-02 10:44:16 UTC
From an rpm perspective, the package looks good. XXX APPROVED XXX

I'll let you decide what arches etc. you want to support @fangq, since you are upstream here anyway.


Cheers,

Comment 39 Qianqian Fang 2020-03-03 18:02:05 UTC
thanks Ankur for approving. I was able to fix the incorrect results with this patch

https://github.com/fangq/mmc/commit/99312c3c1c8b9cfb932930a932c94f7a11aab976

now I free comfortable to move forward to create the package.

Comment 40 Gwyn Ciesla 2020-03-03 19:27:20 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/mmc

Comment 41 Fedora Update System 2020-03-03 22:15:52 UTC
FEDORA-2020-402060f3b7 has been submitted as an update to Fedora 30. https://bodhi.fedoraproject.org/updates/FEDORA-2020-402060f3b7

Comment 42 Fedora Update System 2020-03-03 22:30:08 UTC
FEDORA-2020-626c7c163e has been submitted as an update to Fedora 32. https://bodhi.fedoraproject.org/updates/FEDORA-2020-626c7c163e

Comment 43 Fedora Update System 2020-03-04 20:14:37 UTC
mmc-1.7.9-1.fc32 has been pushed to the Fedora 32 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2020-626c7c163e

Comment 44 Fedora Update System 2020-03-04 21:48:55 UTC
mmc-1.7.9-1.fc31 has been pushed to the Fedora 31 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2020-e50e3bb623

Comment 45 Fedora Update System 2020-03-04 22:26:37 UTC
mmc-1.7.9-1.fc30 has been pushed to the Fedora 30 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2020-402060f3b7

Comment 46 Fedora Update System 2020-03-12 21:56:12 UTC
mmc-1.7.9-1.fc31 has been pushed to the Fedora 31 stable repository. If problems still persist, please make note of it in this bug report.

Comment 47 Fedora Update System 2020-03-13 02:30:08 UTC
mmc-1.7.9-1.fc30 has been pushed to the Fedora 30 stable repository. If problems still persist, please make note of it in this bug report.

Comment 48 Fedora Update System 2020-03-16 20:21:24 UTC
mmc-1.7.9-1.fc32 has been pushed to the Fedora 32 stable repository. If problems still persist, please make note of it in this bug report.

Comment 49 Fedora Update System 2020-03-16 20:32:42 UTC
mmc-1.7.9-1.fc32 has been pushed to the Fedora 32 stable repository. If problems still persist, please make note of it in this bug report.


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