Bug 1856005 - Review Request: dmtcp - Checkpoint/Restart functionality for Linux processes
Summary: Review Request: dmtcp - Checkpoint/Restart functionality for Linux processes
Keywords:
Status: MODIFIED
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: x86_64
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Orion Poplawski
QA Contact: Fedora Extras Quality Assurance
URL: https://github.com/dmtcp/dmtcp
Whiteboard:
Depends On:
Blocks: FE-NEEDSPONSOR
TreeView+ depends on / blocked
 
Reported: 2020-07-11 18:16 UTC by Paul Grosu
Modified: 2020-10-14 16:39 UTC (History)
5 users (show)

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


Attachments (Terms of Use)

Description Paul Grosu 2020-07-11 18:16:21 UTC
Spec URL: http://www.ccs.neu.edu/~pgrosu/fedora/dmtcp.spec
SRPM URL: http://www.ccs.neu.edu/~pgrosu/fedora/dmtcp-2.6.1~rc1-0.1.fc31.src.rpm  

Review (Dec. 2019): http://www.ccs.neu.edu/~pgrosu/fedora/review.txt
Review Response (Dec. 2019): http://www.ccs.neu.edu/~pgrosu/fedora/review-response-new-v1.txt

Fedora Account System Username: pgrosu or pgrosu@gmail.com

FE-NEEDSPONSOR (Orion Poplawski)

Description: 

We have finished packaging DMTCP, and we would appreciate a review.

DMTCP (Distributed MultiThreaded Checkpointing) is a tool to
transparently checkpointing the state of an arbitrary group of
applications including multi-threaded and distributed computations.
It operates directly on the user binary executable, with no Linux kernel
modules or other kernel mods.

Comment 1 Artur Frenszek-Iwicki 2020-07-22 21:26:43 UTC
>Group:		Applications/System
Not used in Fedora.
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_sections

>make %{?_smp_mflags}
Use %make_build instead.

>make install DESTDIR=%{buildroot}
Use %make_install instead.

>%{_mandir}/man1/*gz
Do not assume that man pages will be gzipped.
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages

Comment 2 Paul Grosu 2020-07-24 13:02:50 UTC
Spec URL: http://www.ccs.neu.edu/~pgrosu/fedora/dmtcp.spec
SRPM URL: http://www.ccs.neu.edu/~pgrosu/fedora/dmtcp-2.6.1~rc1-0.1.fc30.src.rpm

We have now updated the dmtcp.spec file according to your corrections.  
The new link contains those fixes. 
Thank you.  
If you would like to do a deeper review, we promise to respond rapidly.

Comment 3 Dave Love 2020-07-24 16:08:54 UTC
This either needs to run python3 explicitly or buildrequire %_bindir/python (if that's still allowable).  Currently %check fails completely and it's not immediately obvious, due to ||:.  Can you adjust the source not to try to run tests which are expected to fail, or somehow count the failures, and remove the ||:?  (I remember seeing a number of failures when I've run the tests on el7 which I never investigated, which might be more-or-less specific to el7, and I can believe running in mock could cause some.)  If that isn't possible, at least add a comment about failures in the %check stanza.

The .so objects in the package-specific directory are fine for internal, use by the way, regardless of what the review tool says.

Comment 4 Paul Grosu 2020-07-25 13:56:09 UTC
Spec URL: http://www.ccs.neu.edu/~pgrosu/fedora/dmtcp.spec
SRPM URL: http://www.ccs.neu.edu/~pgrosu/fedora/dmtcp-2.6.1~rc1-0.1.fc30.src.rpm

We've fixed the two issues:
1.  We separated "python3, gcc-c++" into two separate BuildRequires.
2.  We fixed '%check' to remove '|| :' at the end.  There is a small
    danger on a slow test machine that a test will time out and fail.
    It's not likely on modern machines, and we added a comment to
    explain this issue.
3.  In %check, the command to check was changed to:
      AUTOTEST="--retry-once --slow" make check
    This seems to fix the problem with epel7.  We can now use koji
    to build and check binary packages for epel7.

Finally, note also that the dmtcp.spec lists "%ix86 x86_64 aarch64" under
"ExclusiveArch".  The Bugzilla Hardware field allowed only
all architectures or just one.  We chose x86_64, but we believe
the package works on all three architectures.

Thanks for any feedback.

Comment 5 Dave Love 2020-08-03 12:55:48 UTC
The current version still doesn't run the tests because it doesn't find "python" -- see build.log.  I'd suggest modifying the script(s) to call python3 explicitly.  (I haven't looked at the source.)

Comment 6 Orion Poplawski 2020-08-06 02:11:18 UTC
Issues:
=======
- Package does not contain duplicates in %files.
  Note: warning: File listed twice: /usr/share/doc/dmtcp
  See: https://docs.fedoraproject.org/en-US/packaging-
  guidelines/#_duplicate_files

- Sources used to build the package match the upstream source, as provided
  in the spec URL.
  Note: Upstream MD5sum check error, diff is in
  /home/orion/1856005-dmtcp/diff.txt
  See: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/
Hmm - might be a transient network issue.

- as noted above, %check is incomplete:
make[1]: Leaving directory '/builddir/build/BUILD/dmtcp-2.6.1~rc1/test'
/bin/sh: python: command not found
*** No python found in your path.
*** Please add  python to path or build Python 2.x for x >= 3.

you need to explicitly call /usr/bin/python3.

- please fix the license header in dmtcp.h

Otherwise looking good.

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

C/C++:
[x]: Package does not contain kernel modules.
[-]: Package contains no static executables. - exception granted
[x]: Development (unversioned) .so files in -devel subpackage, if present.
     Note: Unversioned so-files in private %_libdir subdirectory (see
     attachment). Verify they are not in ld path.
[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: Checking patched sources after %prep for licenses. Licenses
     found: "Unknown or generated", "Expat License GNU Lesser General
     Public License, Version 3", "GNU Lesser General Public License,
     Version 3", "GNU General Public License v2.0 or later [generated
     file]", "GNU General Public License v3.0 or later", "FSF Unlimited
     License [generated file]", "Expat License [generated file]", "Public
     domain", "GNU Lesser General Public License v3.0 or later", "LaTeX
     Project Public License", "[generated file]", "GNU Lesser General
     Public License v2.1 or later", "Apache License 2.0", "GNU General
     Public License, Version 2", "GNU Lesser General Public License v2.1 or
     later [obsolete FSF postal address (Temple Place)]". 495 files have
     unknown license. Detailed output of licensecheck in
     /home/orion/1856005-dmtcp/licensecheck.txt
[x]: License file installed when any subpackage combination is installed.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag. - exception granted
[x]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 71680 bytes in 4 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: 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.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[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]: 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 is not relocatable.
[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.
[x]: Final provides and requires are sane (see attachments).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
     Note: gpgverify is not used.
[x]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[!]: %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]: Fully versioned dependency in subpackages if applicable.
[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]: Package should compile and build into binary rpms on all supported
     architectures.
[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)
[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: dmtcp-2.6.1~rc1-0.1.fc33.x86_64.rpm
          dmtcp-devel-2.6.1~rc1-0.1.fc33.x86_64.rpm
          dmtcp-debuginfo-2.6.1~rc1-0.1.fc33.x86_64.rpm
          dmtcp-debugsource-2.6.1~rc1-0.1.fc33.x86_64.rpm
          dmtcp-2.6.1~rc1-0.1.fc33.src.rpm
dmtcp.x86_64: W: spelling-error %description -l en_US checkpointing -> check pointing, check-pointing, checkpoint
dmtcp.x86_64: E: statically-linked-binary /usr/bin/mtcp_restart
dmtcp.x86_64: E: shared-lib-without-dependency-information /usr/lib64/dmtcp/libdmtcp_alloc.so
dmtcp.x86_64: E: missing-call-to-setgroups-before-setuid /usr/lib64/dmtcp/libdmtcp_pid.so
dmtcp-devel.x86_64: W: no-documentation
dmtcp.src: W: spelling-error %description -l en_US checkpointing -> check pointing, check-pointing, checkpoint
dmtcp.src:52: W: macro-in-comment %{__python3}
dmtcp.src:52: W: macro-in-comment %{_vpath_srcdir}
dmtcp.src: W: invalid-url Source0: http://downloads.sourceforge.net/dmtcp/2.6.1/dmtcp-2.6.1~rc1.tar.gz <urlopen error [Errno 101] Network is unreachable>
5 packages and 0 specfiles checked; 3 errors, 6 warnings.




Rpmlint (debuginfo)
-------------------
Checking: dmtcp-debuginfo-2.6.1~rc1-0.1.fc33.x86_64.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.





Rpmlint (installed packages)
----------------------------
dmtcp.x86_64: W: spelling-error %description -l en_US checkpointing -> check pointing, check-pointing, checkpoint
dmtcp.x86_64: W: invalid-url URL: http://dmtcp.sourceforge.net <urlopen error [Errno -3] Temporary failure in name resolution>
dmtcp.x86_64: E: statically-linked-binary /usr/bin/mtcp_restart
dmtcp.x86_64: E: shared-lib-without-dependency-information /usr/lib64/dmtcp/libdmtcp_alloc.so
dmtcp.x86_64: E: missing-call-to-setgroups-before-setuid /usr/lib64/dmtcp/libdmtcp_pid.so
dmtcp-debugsource.x86_64: W: invalid-url URL: http://dmtcp.sourceforge.net <urlopen error [Errno -3] Temporary failure in name resolution>
dmtcp-devel.x86_64: W: invalid-url URL: http://dmtcp.sourceforge.net <urlopen error [Errno -3] Temporary failure in name resolution>
dmtcp-devel.x86_64: W: no-documentation
dmtcp-debuginfo.x86_64: W: invalid-url URL: http://dmtcp.sourceforge.net <urlopen error [Errno -3] Temporary failure in name resolution>
4 packages and 0 specfiles checked; 3 errors, 6 warnings.



Unversioned so-files
--------------------
dmtcp: /usr/lib64/dmtcp/libdmtcp.so
dmtcp: /usr/lib64/dmtcp/libdmtcp_alloc.so
dmtcp: /usr/lib64/dmtcp/libdmtcp_batch-queue.so
dmtcp: /usr/lib64/dmtcp/libdmtcp_dl.so
dmtcp: /usr/lib64/dmtcp/libdmtcp_ipc.so
dmtcp: /usr/lib64/dmtcp/libdmtcp_modify-env.so
dmtcp: /usr/lib64/dmtcp/libdmtcp_pathvirt.so
dmtcp: /usr/lib64/dmtcp/libdmtcp_pid.so
dmtcp: /usr/lib64/dmtcp/libdmtcp_svipc.so
dmtcp: /usr/lib64/dmtcp/libdmtcp_timer.so
dmtcp: /usr/lib64/dmtcp/libdmtcp_unique-ckpt.so

Source checksums
----------------
http://downloads.sourceforge.net/dmtcp/2.6.1/dmtcp-2.6.1~rc1.tar.gz :
  CHECKSUM(SHA256) this package     : 06d4af27ebfa25b0beb0de7d0c1eb308b95a23af4867d5686d938b05e76dbce0
  CHECKSUM(SHA256) upstream package : 4fa92d711609c945c3f6364f3824cd9b315f3c9ef3ad877888e8d45597504661
diff -r also reports differences


Requires
--------
dmtcp (rpmlib, GLIBC filtered):
    /usr/bin/bash
    ld-linux-x86-64.so.2()(64bit)
    libc.so.6()(64bit)
    libdl.so.2()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libgcc_s.so.1(GCC_3.3.1)(64bit)
    libm.so.6()(64bit)
    libpthread.so.0()(64bit)
    librt.so.1()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    libstdc++.so.6(CXXABI_1.3.9)(64bit)
    rtld(GNU_HASH)

dmtcp-devel (rpmlib, GLIBC filtered):
    dmtcp(x86-64)

dmtcp-debuginfo (rpmlib, GLIBC filtered):

dmtcp-debugsource (rpmlib, GLIBC filtered):



Provides
--------
dmtcp:
    dmtcp
    dmtcp(x86-64)
    libdmtcp.so()(64bit)
    libdmtcp_alloc.so()(64bit)
    libdmtcp_batch-queue.so()(64bit)
    libdmtcp_dl.so()(64bit)
    libdmtcp_ipc.so()(64bit)
    libdmtcp_modify-env.so()(64bit)
    libdmtcp_pathvirt.so()(64bit)
    libdmtcp_pid.so()(64bit)
    libdmtcp_svipc.so()(64bit)
    libdmtcp_timer.so()(64bit)
    libdmtcp_unique-ckpt.so()(64bit)

dmtcp-devel:
    dmtcp-devel
    dmtcp-devel(x86-64)

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

dmtcp-debugsource:
    dmtcp-debugsource
    dmtcp-debugsource(x86-64)

Comment 7 Paul Grosu 2020-08-07 02:49:47 UTC
Spec URL: http://www.ccs.neu.edu/~pgrosu/fedora/dmtcp.spec
SRPM URL: http://www.ccs.neu.edu/~pgrosu/fedora/dmtcp-2.6.1~rc1-0.1.fc30.src.rpm

Hi Orion,

New the SRPM has 3 new changes from what you saw before:

1) We now include the Apache License 2.0 in dmtcp.h.  Thank you for catching this.
2) The Koji testing seems to have problems with our %ix86 architecture during 'make check'.  We believe it works, but since this architecture is seldom used for our DMTCP, we removed that architecture from the dmtcp.spec
3) We changed the %check field in dmtcp.spec.  Instead of explicitly invoking /usr/bin/python3, it seems simpler to invoke 'make check' with some added options from DMTCP.

Thank you,
- Gene and Paul

Comment 8 Dave Love 2020-08-10 08:38:39 UTC
As I looked at it, and it may save Orion saying the same if I'm not putting words in his mouth:

I saw the tests are still not being run -- see the log.  I think you need to do something, either fixing it up for python3 or dropping %check, which is confusing if it never works.  For some packages fixing up tests might not be so important, but this seems to me like the sort of intricate application for which they would be.  (Fedora can give you an early warning about impending breakage due to changes in linux, libc, etc., though I don't know how sensitive dmtcp is to those.)

Also, rpmlint still complains about the %dir in %files, though I think it's harmless.  You only need %dir if you're installing into a directory which isn't owned by a package that's guaranteed to be installed; pkgconfig or cmake files are common examples.

Comment 9 Orion Poplawski 2020-08-12 02:31:17 UTC
My main concern at this point is the proper setting of the License field.  I'm sure I quite understand having the header file under a different license.  It may be worth pinging legal about it.

Comment 10 Paul Grosu 2020-08-13 07:23:53 UTC
Hi David and Orion,

David: The last time we were able to get on to Koji (last week), we were able to see the %check successfully executed for our latest dmtcp.spec.  But it appears that Koji has been down for about a week.  In either case, we would be able to comment out %check in dmtcp.spec.  Is that allowed?

Orion: Thank you for looking into the license issues.

Thank you,
- Gene and Paul

Comment 11 Dave Love 2020-08-13 13:14:22 UTC
I can't remember what's been off when, but koji hasn't been off for long recently.  Anyway, you can build locally even if you don't ru Fedora.  I run mock in a vagrant VM hosted on a Debian system.

If you look at https://koji.fedoraproject.org/koji/taskinfo?taskID=49201579 you'll see all the tests have been skipped because there's no "python".  I'd have thought you'd want to autoconf python for 2 and 3 these days, but if you don't want to do that or just edit the shebang, you can currently still just require %_bindir/python.

The changes in https://download.copr.fedorainfracloud.org/results/loveshack/testing/fedora-rawhide-x86_64/01607736-dmtcp/ get the tests run, but one fails in mock for some reason that you might have a better idea about; it doesn't outside mock.  It's allowed not to run tests -- they may not exist -- but I still think running them would be useful, given what I've seen break on the bleeding edge of rawhide.

Comment 12 Dave Love 2020-08-13 14:45:06 UTC
On EL7 the script test fails, even outside mock, and kills make check

bash           ckpt:PASSED; rstr:PASSED -> ckpt:PASSED; rstr:PASSED
script         ckpt:PASSED; rstr:PASSED -> ckpt:PASSED; rstr:bash: line 1: 29524 Alarm clock             python3 ./test/autotest.py --retry-once --slow
make: *** [check] Error 142

Comment 13 Paul Grosu 2020-08-17 17:21:25 UTC
Spec URL: http://www.ccs.neu.edu/~pgrosu/fedora/dmtcp.spec
SRPM URL: http://www.ccs.neu.edu/~pgrosu/fedora/dmtcp-2.6.1~rc1-0.1.fc30.src.rpm

Hi David and Orion,

Okay, we tracked down the bug in '%check'.  It was a silly mistake.
You should now be seeing a full '%check', and we tested on epel7 now.

Running '%check' causes the build of the binary rpm to be rather slow.
We can leave it as is, or we can comment out the '%check'.

For now, we've updated:
 * dmtcp.spec
 * dmtcp-2.6.1~rc1-0.1.el7.src.rpm  (using epel7 now for our source rpm)

Thanks again for your help.
- Gene and Paul

Comment 14 Dave Love 2020-08-24 09:28:14 UTC
I don't know about Orion, wouldn't worry about a build time of half an hour with tests.  I'm more worried about not being able to get the tests to pass if I make sure they're not skipped (which they still are in the build).  One or more fail in mock on el7 and f33, as well as with a straight build from the tarball on centos7 and debian 10.  I don't have time to try to debug them.  Can you say what's going wrong and how significant it is?

Koji examples:
https://koji.fedoraproject.org/koji/taskinfo?taskID=49885693 
https://koji.fedoraproject.org/koji/taskinfo?taskID=49893986

Comment 15 Paul Grosu 2020-09-08 04:42:50 UTC
Hi Dave,

Sorry for the delay in responding.  Where it is failing it tends to be for rarely used cases, or for cases where the tests take long or may time out.  Primarily we are interested in having DMTCP 2.6 accepted as a package, and then soon after updating to DMTCP 3.0.  Would it be better if we skipped the tests for now?  Our larger goal is to move our package to DMTCP 3.0, but we would like to see a 2.6 package first as a confidence-building measure.

Best regards,
- Gene and Paul

Comment 16 Paul Grosu 2020-10-14 16:39:56 UTC
Hi Orion,
    When we last heard, you were checking with the lawyers if it was okay for us to have two slightly different open-source copyrights.  For almost all of DMTCP, it's LGPLv3.  But for the single file include/dmtcp.h , it's Apache (or any other completely open license).  The file include/dmtcp.h needs to be added to the end user's own program.  We don't want LGPLv3 there, since it would make claims about the copyright of the larger execution unit in which it is embedded.

Thanks,
- Paul and Gene


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