Bug 1690046 - Review Request: charliecloud - unprivileged container runtime
Summary: Review Request: charliecloud - unprivileged container runtime
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Dave Love
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2019-03-18 16:27 UTC by jogas
Modified: 2019-09-06 13:26 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2019-09-06 13:26:15 UTC
Type: ---
Embargoed:
dave.love: fedora-review+


Attachments (Terms of Use)
make test-run log (9.85 KB, text/plain)
2019-07-12 19:48 UTC, Dave Love
no flags Details

Description jogas 2019-03-18 16:27:49 UTC
Spec URL:
https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/epel-7-x86_64/00868926-charliecloud/charliecloud.spec

SRPM URL:
https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/epel-7-x86_64/00868926-charliecloud/charliecloud-0.9.8-1.el7.src.rpm

Description:
Greetings, I am opening this review request with hopes of getting this package into Fedora Extras.

Charliecloud is a 2018 R&D award winning container runtime. It leverages Linux user namespaces to run containers with no privileged operations, no setuid helpers, no daemons, and minimal (if any) configuration changes on center resources. Container images can be built using Docker or anything else that can generate a standard Linux filesystem tree.

Fedora Account System Username:
Jogas

rpmlint for charliecloud.spec:
$ rpmlint ~/rpmbuild/SPECS/charliecloud.spec
/home/jogas/rpmbuild/SPECS/charliecloud.spec: W: invalid-url Source0: https://github.com/hpc/charliecloud/releases/download/v0.9.8/charliecloud-0.9.8.tar.gz HTTP Error 403: Forbidden
0 packages and 1 specfiles checked; 0 errors, 1 warnings.

The source URL points to an asset tarball with pre-built man pages. You can download it just fine with `wget`, but the `HEAD` request that rpmlint sends github is met with a 403 forbidden error. See: https://github.com/rpm-software-management/rpmlint/issues/71

rpmlint log for charliecloud package:
$ rpmlint charliecloud
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

Rpmlint log for charliecloud-doc subpackage:
$ rpmlint charliecloud-doc
charliecloud-doc.x86_64: E: no-ldconfig-symlink /usr/libexec/charliecloud/test/sotest/lib/libsotest.so.1.0
charliecloud-doc.x86_64: W: devel-file-in-non-devel-package /usr/libexec/charliecloud/examples/mpi/mpihello/hello.c
charliecloud-doc.x86_64: E: library-without-ldconfig-postin /usr/libexec/charliecloud/test/sotest/lib/libsotest.so.1.0
charliecloud-doc.x86_64: E: library-without-ldconfig-postun /usr/libexec/charliecloud/test/sotest/lib/libsotest.so.1.0
charliecloud-doc.x86_64: W: devel-file-in-non-devel-package /usr/libexec/charliecloud/test/chtest/chroot-escape.c
charliecloud-doc.x86_64: W: dangling-relative-symlink /usr/libexec/charliecloud/test/bin ../../../bin
charliecloud-doc.x86_64: W: devel-file-in-non-devel-package /usr/libexec/charliecloud/test/sotest/sotest.c
charliecloud-doc.x86_64: W: devel-file-in-non-devel-package /usr/libexec/charliecloud/test/chtest/setgroups.c
charliecloud-doc.x86_64: W: devel-file-in-non-devel-package /usr/libexec/charliecloud/examples/syscalls/userns.c
charliecloud-doc.x86_64: W: devel-file-in-non-devel-package /usr/libexec/charliecloud/test/chtest/mknods.c
charliecloud-doc.x86_64: W: devel-file-in-non-devel-package /usr/libexec/charliecloud/examples/syscalls/pivot_root.c
charliecloud-doc.x86_64: W: devel-file-in-non-devel-package /usr/libexec/charliecloud/test/chtest/setuid.c
1 packages and 0 specfiles checked; 3 errors, 9 warnings.

charliecloud-doc rpmlint comments:

1. E: no-ldconfig-symlink /usr/libexec/charliecloud/test/sotest/lib/libsotest.so.1.0

Charliecloud is a container runtime. These shared objects are never used in the host environment; rather, they are compiled by the test suite (both running and examination of which serve as end-user documentation) and injected into the container (guest) via utility script 'ch-fromhost'. The ldconfig links are generated inside the container runtime environment. For more information, see the test file: https://github.com/hpc/charliecloud/blob/master/test/run/ch-fromhost.bats (line 108).

2. W: devel-file-in-non-devel-package /usr/libexec/charliecloud/examples/mpi/mpihello/hello.c

The test suite has a few C files, e.g. userns.c, pivot_root.c, chroot-escape.c, sotest.c, setgroups.c, mknods.c, setuid.c, .etc, that document — line-by-line in many cases — various components of the open source runtime. These C files are part of our documentation to show end users how containers work.

3. E: library-without-ldconfig-postin /usr/libexec/charliecloud/test/sotest/lib/libsotest.so.1.0

See response to #1.

4.: library-without-ldconfig-postun /usr/libexec/charliecloud/test/sotest/lib/libsotest.so.1.0

See response to #1.

5. devel-file-in-non-devel-package /usr/libexec/charliecloud/test/chtest/chroot-escape.c

See response to #2.

6. dangling-relative-symlink /usr/libexec/charliecloud/test/bin ../../../bin

This is a false positive. The symlink is to /usr/bin, which is not in the package.

7..12.
W: devel-file-in-non-devel-package /usr/libexec/charliecloud/test/sotest/sotest.c
W: devel-file-in-non-devel-package /usr/libexec/charliecloud/test/chtest/setgroups.c
W: devel-file-in-non-devel-package /usr/libexec/charliecloud/examples/syscalls/userns.c
W: devel-file-in-non-devel-package /usr/libexec/charliecloud/test/chtest/mknods.c
W: devel-file-in-non-devel-package /usr/libexec/charliecloud/examples/syscalls/pivot_root.c
W: devel-file-in-non-devel-package /usr/libexec/charliecloud/test/chtest/setuid.c

See response to #2.

Comment 1 Robert-André Mauchin 🐧 2019-03-18 20:54:02 UTC
You'll also need to find a sponsor to get into the Packager group.
See https://fedoraproject.org/wiki/Join_the_package_collection_maintainers?rd=PackageMaintainers/Join and https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

Comment 2 Dave Love 2019-03-19 16:05:25 UTC
I didn't realize this would need sponsorship as well as review.

Anyway, it needs work.  The first requirement is to build, which it doesn't on
rawhide, at least.  You should do scratch builds, taking into account
the differences between el7 and rawhide.  There are various other
things that need fixing which I haven't got time for immediately, but
have a look at
https://copr-dist-git.fedorainfracloud.org/cgit/loveshack/charliecloud/charliecloud.git/tree/
in comparison.  I'm not sure of the merits of shipping the test files,
but they should be in a separate package -- they're not documentation
-- and all you need to avoid the issue of compiling with the wrong
python cleanly is to define the right one, and you need versioned
python now.

Comment 3 jogas 2019-03-19 22:55:36 UTC
(In reply to Dave Love from comment #2)
> I didn't realize this would need sponsorship as well as review.
> 
> Anyway, it needs work.  The first requirement is to build, which it doesn't
> on
> rawhide, at least.  You should do scratch builds, taking into account
> the differences between el7 and rawhide.  There are various other
> things that need fixing which I haven't got time for immediately, but
> have a look at
> https://copr-dist-git.fedorainfracloud.org/cgit/loveshack/charliecloud/
> charliecloud.git/tree/
> in comparison.  I'm not sure of the merits of shipping the test files,
> but they should be in a separate package -- they're not documentation
> -- and all you need to avoid the issue of compiling with the wrong
> python cleanly is to define the right one, and you need versioned
> python now.

(In reply to Dave Love from comment #2)
> I didn't realize this would need sponsorship as well as review.
> 
> Anyway, it needs work.  The first requirement is to build, which it doesn't
> on
> rawhide, at least.  You should do scratch builds, taking into account
> the differences between el7 and rawhide.  There are various other
> things that need fixing which I haven't got time for immediately, but
> have a look at
> https://copr-dist-git.fedorainfracloud.org/cgit/loveshack/charliecloud/
> charliecloud.git/tree/
> in comparison.  I'm not sure of the merits of shipping the test files,
> but they should be in a separate package -- they're not documentation
> -- and all you need to avoid the issue of compiling with the wrong
> python cleanly is to define the right one, and you need versioned
> python now.


Dave, 

I appreciate you taking the time to review and point out issues that need resolving in such a timely manner. Thank you. We recently updated the charliecloud build procedure in an effort to reduce packaging complexity. I am working on incorporating some of the technique and best practices I see in your spec file. How would you like to be credited?

Best,
Jordan

Comment 4 jogas 2019-03-21 21:43:33 UTC
Below are the updated spec and srpm files. The resulting package successfully builds on epel-7-ppc, epel-7, fedora-28, fedora-29, fedora-30, fedora-rawhide, opensuse-leap15, and rhelbeta-8. It also defines a python version (3 by default unless built with `--with python2`), and renames the doc subpackage to test.

Spec https://copr-dist-git.fedorainfracloud.org/cgit/jogas/charliecloud/charliecloud.git/tree/charliecloud.spec?id=fa6f5bfe817503f56af3bc205b5664560d5f5cc0
SRPM: https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/srpm-builds/00871778/charliecloud-0.9.8-1.el7.src.rpm

The rpmlint remains largely unchanged (see notes).

$ rpmlint charliecloud
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

$ rpmlint charliecloud-test
charliecloud-test.x86_64: E: no-ldconfig-symlink /usr/libexec/charliecloud/test/sotest/lib/libsotest.so.1.0
charliecloud-test.x86_64: W: devel-file-in-non-devel-package /usr/libexec/charliecloud/examples/mpi/mpihello/hello.c
charliecloud-test.x86_64: E: library-without-ldconfig-postin /usr/libexec/charliecloud/test/sotest/lib/libsotest.so.1.0
charliecloud-test.x86_64: E: library-without-ldconfig-postun /usr/libexec/charliecloud/test/sotest/lib/libsotest.so.1.0
charliecloud-test.x86_64: W: devel-file-in-non-devel-package /usr/libexec/charliecloud/test/chtest/chroot-escape.c
charliecloud-test.x86_64: W: dangling-relative-symlink /usr/libexec/charliecloud/test/bin ../../../bin
charliecloud-test.x86_64: W: devel-file-in-non-devel-package /usr/libexec/charliecloud/test/sotest/sotest.c
charliecloud-test.x86_64: W: devel-file-in-non-devel-package /usr/libexec/charliecloud/test/chtest/setgroups.c
charliecloud-test.x86_64: W: devel-file-in-non-devel-package /usr/libexec/charliecloud/examples/syscalls/userns.c
charliecloud-test.x86_64: W: devel-file-in-non-devel-package /usr/libexec/charliecloud/test/chtest/mknods.c
charliecloud-test.x86_64: W: devel-file-in-non-devel-package /usr/libexec/charliecloud/examples/syscalls/pivot_root.c
charliecloud-test.x86_64: W: devel-file-in-non-devel-package /usr/libexec/charliecloud/test/chtest/setuid.c
1 packages and 0 specfiles checked; 3 errors, 10 warnings.

rpmlint still complains that the test package has development files. These C files, in addition to their compiled products, are an integral part of the test suite. They are not for downstream developers as would be appropriate in a -devel package. They also illustrate to users of the test suite how things are working.

Comment 5 Dave Love 2019-03-24 14:39:36 UTC
For future reference, the spec URL should be to the raw version for use with
fedora-review though that's currently rather broken.  (You should probably
increment the release tag with changes, at least if it's in copr.)

The rpmlint messages aren't important -- it frequently gives false positives
-- but you need to obey the packaging guide, specifically about the doc files,
as below.

Conditionals for SUSE aren't allowed (unfortunately IMHO), though that
redefinition looks wrong anyhow.

I guess it's harmless, but why the python2 option?

Don't buildrequire make, which is in the build root.  The version constraints
on the BRs aren't necessary since they're always satisfied, and listing them
seems confusing.

Why isn't there a doc package, as in Debian?  The doc source has items which
aren't redundant with the man pages -- including about testing -- and I think
examples should be doc, though I don't think there are rules about that.

LICENSE shouldn't be duplicated in %doc.  Documentation should be in
%_pkgdocdir, not %_datadir/doc, and relative paths (README.EL7) shouldn't be
mixed with absolute ones.  The condition for README.EL7 is wrong (spurious
"_").  I don't know if it has any effect, but don't use %doc for man pages.

It looks as if the test package should require docker (or maybe podman).  I
don't have VMs with enough space to run the tests; I assume they run OK in
EPEL and recent Fedora.

I assume you're trying to get sponsored, but I can't see a self-introduction
on the -devel list.  I'm afraid I can't do it.

Comment 6 jogas 2019-04-02 22:39:07 UTC
(In reply to Dave Love from comment #5)

Once again thank you for your timely feedback. Below are answers to questions
that were not addressed in the updated spec file.

Spec file: https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/fedora-rawhide-x86_64/00876701-charliecloud/charliecloud.spec
SRPM: https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/fedora-rawhide-x86_64/00876701-charliecloud/charliecloud-0.9.8-2.fc31.src.rpm

> Why isn't there a doc package, as in Debian?  The doc source has items which
> aren't redundant with the man pages -- including about testing ...

We plan to provide pre-built html files in the source tarball in an upstream
release.

> I think examples should be doc, though I don't think there are rules about
> that.

Both the example and test subdirectories are integral components of the test
suite. It seems awkward to split the test suite into two packages, e.g., -doc
and -test. That said, we do think the ‘examples’ directory name is somewhat
misleading, especially considering how tightly coupled it is to the test suite.
We plan to address this upstream in the near future.

> It looks as if the test package should require docker (or maybe podman).

The test suite runs and passes without Docker, the scope is just smaller. This
is a situation we feel would benefit from debian-style package relationships,
e.g., `recommends` or `suggests`. While we have do have tests that revolve
around an image builder like Docker, Buildah, or Skopeo, the test suite does
include tests that do not.

> I assume you're trying to get sponsored, but I can't see a self-introduction
> on the -devel list.  I'm afraid I can't do it.

I have not yet introduced myself to the -devel list. My focus has been to work
with you to ensure we end up with a suitable package. That said, the goal is to
become a fedora package manager, do you recommend I search for a sponsor in
parallel?

Comment 7 Dave Love 2019-04-08 16:32:35 UTC
As far as doc files are concerned, I think you should consider the version you're packaging, rather than a future one, and the source built for me, so there doesn't seem to be a reason not to include it.  (If you mean you won't ship the doc source eventually, that would make it non-free by Debian standards.)

I'm not sure what the problem is with shipping small example files as part of the documentation, which is what's normally done, though I don't think there's policy on it.  If tests run without docker, can you run them in %check?  (I don't know if the builders have the namespace support turned on, and should probably check; if not, perhaps it could be turned on if it helps this sort of package.)

Creating the READMEs should happen under %install, not %check.  I'll check more closely later.

You can't get the package into Fedora without being a maintainer, and you probably need to do some informal reviews before someone will sponsor you, so yes, you should be working on that.  See the relevant links under  https://fedoraproject.org/wiki/Package_Review_Process
(It would probably have been better for me to submit packaging and make you co-maintainer initially, but we'll have to work from here.)

Hope that helps for the moment.

Comment 8 jogas 2019-04-10 19:52:00 UTC
(In reply to Dave Love from comment #7)
> ... and the source built for me, so there doesn't seem to be a reason not
> to include it.
 
The html files build correctly with versions of sphinx-build greater than
1.4.9, which as far as I can tell, are not available in el7.
 
We could build the documentation with broken list-tables on el7. This could
be done by forcing sphinx-build to ignore warnings, instead of error.
 
The thought here was that it may be better to provide pre-built html files via
source tarball while we wait for a newer sphinx to become available in el7.
 
Thus the approach in mind is as follows:

* 0.9.8 (this version) provide man pages and a readme with a link to the html,
documentation online.

* 0.9.9 package pre-built html files via source tarball.

When a newer version of sphinx is available in epel, we build them in the spec
file.

Thoughts?


> If tests run without docker, can you run them in %check?  (I don't know if
> the builders have the namespace support turned on, and should probably
> check; if not, perhaps it could be turned on if it helps this sort of
> package.)
 
The tests do run without docker, however, when I attempt to run them in the
%check segment during rpmbuild, one of the tests that uses namespaces hangs.
I am not sure why, as this behavior does not occur outside of rpmbuild.
 
Perhaps we could add a sanity check like `ch-run --version`?
 
> Hope that helps for the moment.
 
It does, thank you. Your feedback is greatly appreciated.

Comment 9 jogas 2019-04-16 20:50:54 UTC
Charliecloud 0.9.9 released and I have updated the spec file accordingly. Note:
pre-built html files did not make it into this version, this is currently planned
for 0.9.10.

Spec file: https://copr-dist-git.fedorainfracloud.org/cgit/jogas/charliecloud/charliecloud.git/tree/charliecloud.spec?id=6109d2193f3ab035cb221b13cd23293861c3e7ca

Comment 10 Dave Love 2019-04-23 16:01:48 UTC
Apologies for the delay;  I'll get back to this a.s.a.p. (but it won't go anywhere without a sponsor anyway).

Comment 11 jogas 2019-04-29 17:03:26 UTC
(In reply to Dave Love from comment #10)
> Apologies for the delay;  I'll get back to this a.s.a.p. (but it won't go
> anywhere without a sponsor anyway).

No worries. Please refer to:

Most recent COPR build: https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/fedora-rawhide-x86_64/00896310-charliecloud/
SPEC: https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/fedora-rawhide-x86_64/00896310-charliecloud/charliecloud.spec
SRPM: https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/fedora-rawhide-x86_64/00896310-charliecloud/charliecloud-test-debuginfo-0.9.9-3.fc31.x86_64.rpm

A few changes are noted in the change log. FWIIW, 0.9.10 is on the horizon and I
have started scouting the lab for a Fedora Maintainer whom I can pester for
sponsorship.

Comment 12 jogas 2019-05-15 15:55:26 UTC
Dave,

0.9.10 has been released and I have updated the spec file accordingly:

SPEC: https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/fedora-rawhide-x86_64/00909410-charliecloud/charliecloud.spec
SRPM: https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/fedora-rawhide-x86_64/00909410-charliecloud/charliecloud-0.9.10-1.fc31.src.rpm


The html documents are now packaged. The spec file has also been updated to install the test suite at PREFIX/libexecdir/<name>-<version>.

Best,

Jordan

Comment 13 Orion Poplawski 2019-06-12 01:14:38 UTC
Dave - Do you still have time to do the review?  I can sponsor Jordan.

Comment 14 Dave Love 2019-06-13 15:35:53 UTC
(In reply to Orion Poplawski from comment #13)
> Dave - Do you still have time to do the review?  I can sponsor Jordan.

Sorry, I thought I was up-to-date on this, and it was just waiting on sponsorship.
I'll try to sort it out tomorrow if I can reconstruct a rawhide VM.

Comment 15 Dave Love 2019-06-14 09:40:43 UTC
The spec and srpm are 404.  Could you post what I should be looking at,
to avoid rooting around in copr and perhaps picking the wrong thing.

Comment 16 jogas 2019-06-14 16:26:47 UTC
(In reply to Dave Love from comment #15)
> The spec and srpm are 404.  Could you post what I should be looking at,
> to avoid rooting around in copr and perhaps picking the wrong thing.

Certainly.

SPEC: https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/fedora-rawhide-x86_64/00935869-charliecloud/charliecloud.spec
SRPM: https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/fedora-rawhide-x86_64/00935869-charliecloud/charliecloud-0.9.10-1.fc31.src.rpm

Thanks Dave.

Comment 17 Dave Love 2019-06-17 10:00:26 UTC
The fedora-review tool is currently more broken than usual, so I can't
do this properly.  However, here's a fairly quick semi-manual go at it.

The Release tag says 1, but the %changelog says 2.

I don't know what the _libexecdir definition is here, but it should be
removed.

build_cflags and build_ldflags are always defined now, so the
conditionals for those can be removed.

It might be better to change this to use mv and avoid the %exclude in
%files:

  cp -r %{buildroot}/%{_libexecdir}/%{name} %{buildroot}/%{_libexecdir}/%{name}-%{version}

Is there some particular reason for doing it that way?

The major issue now is the documentation, I think.  That's 11MB, which
at least means it should be in a sub-package.  However, the bulk of
that seems to be bundled fonts.  They appear (although I'm not sure)
all to be packaged, so the packaged versions should be used.  I don't
know about possibly unbundling various JavaScript bits, but at least
jquery seems to be packaged.  I'm not sure generally how that needs to
be, or should be handled, e.g. by linking from %_jsdir.  Also the
currently shipped files have licences other than the ASL 2.0 that's
declared by the rpm, which I'd missed in my original packaging.
Multiple licences need to be listed in the spec.

I haven't yet figured out how to run the tests from the installed
package.  I think that deserves a README.  Is it sensible for the test
package to require any packages it might use?  Probably it should use
some sort of weak dependencies (conditionally, as they're not
supported in el7) on things it can use from Fedora.

This looks a lot simpler to package than it is on closer inspection!
Orion may have more insight to share.

Comment 18 jogas 2019-06-24 14:11:08 UTC
> The Release tag says 1, but the %changelog says 2.

Oops. Fixed locally.

> I don't know what the _libexecdir definition is here, but it should be
> removed.

The libexecdir macro expands to /usr/lib instead of /usr/libexec on SUSE.
The _libexecdir macro was defined to avoid a SUSE conditional. Is there a
better solution I am missing?

> build_cflags and build_ldflags are always defined now, so the
> conditionals for those can be removed.

Great!

> It might be better to change this to use mv and avoid the %exclude in
> %files:
>
>  cp -r %{buildroot}/%{_libexecdir}/%{name} %{buildroot}/%{_libexecdir}/%{name}-%{version}

Good point, resolved locally.

Re. Documentation:

To recap: the primary issue is that we cannot build the documentation without
sphinx-build version 1.4.9 or greater. AFAICT, only Fedora29, Fedora30, and
Rawhide can satisfy this dependency. Epel provides version 1.1.3.

Originally, on epel, your spec file forced the documentation to build by
ignoring warnings via:

$ make -C doc-src -k || :

I initially thought this method built the html files with broken tables.
However, upon closer inspection I've learned that the html files are skipped
entirely. At this point, I am convinced this the best way to proceed. We build
the documentation and on epel, in place of the html files, we can add a readme
that directs them to online html documentation. In short we'd have:

    Fedora flavors: man pages and html docs
    epel: man pages and readme with a link to online html docs

I'd like your blessing before moving forward. If Orion has any thoughts or
advice regarding this situation it would be warmly received.

> I haven't yet figured out how to run the tests from the installed
> package. I think that deserves a README.  [...]

The test suite location and its usage is unclear. We are addressing this
upstream by adding a ch-test wrapper to bin (https://github.com/hpc/charliecloud/pull/447).
For now, I will add a readme.

As always, I appreciate your time and effort. Thanks Dave.

Best,
Jordan

Comment 19 Orion Poplawski 2019-06-24 16:04:15 UTC
(In reply to jogas from comment #18)
> > I don't know what the _libexecdir definition is here, but it should be
> > removed.
> 
> The libexecdir macro expands to /usr/lib instead of /usr/libexec on SUSE.
> The _libexecdir macro was defined to avoid a SUSE conditional. Is there a
> better solution I am missing?

If SUSE expands _libexecdir to /usr/lib, perhaps they really want you to put your "libexec" files there?  If you want to keep this, put a comment explaining why it is being defined, and a preferable definition would be:

%{!?_libexecdir:%define _libexecdir %{_exec_prefix}/libexec}

unless SUSE does not define %{_exec_prefix}.

> Re. Documentation:
> 
> To recap: the primary issue is that we cannot build the documentation without
> sphinx-build version 1.4.9 or greater. AFAICT, only Fedora29, Fedora30, and
> Rawhide can satisfy this dependency. Epel provides version 1.1.3.
> 
> Originally, on epel, your spec file forced the documentation to build by
> ignoring warnings via:
> 
> $ make -C doc-src -k || :
> 
> I initially thought this method built the html files with broken tables.
> However, upon closer inspection I've learned that the html files are skipped
> entirely. At this point, I am convinced this the best way to proceed. We
> build
> the documentation and on epel, in place of the html files, we can add a
> readme
> that directs them to online html documentation. In short we'd have:
> 
>     Fedora flavors: man pages and html docs
>     epel: man pages and readme with a link to online html docs
> 
> I'd like your blessing before moving forward. If Orion has any thoughts or
> advice regarding this situation it would be warmly received.

That's fine by me.  Another option is to capture a build of the docs from Fedora and install that on EPEL.


FYI - I've sponsored Jordan, and will be leaving tomorrow for an extended vacation.

Comment 20 jogas 2019-07-02 19:59:00 UTC
Another iteration!

SPEC: https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/fedora-rawhide-x86_64/00956000-charliecloud/charliecloud.spec
SRPM: https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/fedora-rawhide-x86_64/00956000-charliecloud/charliecloud-0.9.10-3.fc31.x86_64.rpm

Notes:

1. Fedora COPR build scope narrowed to target Fedora 28, 29, 30, Rawhide,
   and EPEL (x86_64 only). Our goal is to get this package into Fedora/EPEL
   and the spec file was being modified to accommodate environments out of
   scope, e.g., suse.
2. libexecdir definition removed (see .1).
3. Source URL changed back to upstream archives; no longer using the pre-
   packaged html files.
4. html documentation is now built, except on epel, where the README.EL7
   plugs the online html documentation.
5. A README.TEST now shows users where and how to interact with the test suite.

Best,
Jordan

Comment 21 Dave Love 2019-07-02 22:27:56 UTC
I was just about to look at this after being busy/away.  I don't understand the restriction to x86_64, but without a buildarch in the spec it will get built on everything anyhow.  I don't see any reason to exclude the other arches, so you can just leave it at that.

Yes, you can't rely on maintaining a spec for both Fedora and SUSE, and I was surprised Orion thought that was OK.  The spec can get edited by others in Fedora for various reasons.

You can build the doc on x86_64 el7, at least, with a trivial edit, which seems to keep the formatting <https://copr-be.cloud.fedoraproject.org/results/loveshack/charliecloud/epel-7-x86_64/00884378-charliecloud/charliecloud.spec>.  (I don't understand the missing bits which break it on ppc64le.)

Thanks for the test README.  I'll try to find time to run it tomorrow to check, as I should. I expect it's in reasonable shape, but you might like to consider the doc change, assuming it still works with the current version.

Comment 23 Dave Love 2019-07-08 15:23:45 UTC
Apologies that events intervened to stop me getting back to this.

The posted rpm is actually not the source one.  In the absence of a working fedora-review, I've fetched https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/epel-7-x86_64/00956559-charliecloud/charliecloud-0.9.10-4.el7.src.rpm -- I hope that's the right version -- and tried it under el7.

The instructions in the -test package README don't work.  make doesn't run in /usr/libexec/charliecloud-0.9.10 for lack of a Makefile there.  Looking at the files under test and examples, they expect (an unversioned) /usr/libexec/charliecloud, which is also what I'd expect to get installed.

I expect fixing that is the only thing left, but I should eyeball the spec again.

Comment 24 jogas 2019-07-10 17:44:17 UTC
Dave,

Good catch on the test suite install. To address that, I've removed the mv command in the install section; it will no longer attempt to move the test and examples directories to _libexecidr/<name>-<version>. The test suite now works as intended.

I've also updated the test readme.


SPEC: https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/fedora-rawhide-x86_64/00967221-charliecloud/charliecloud.spec
SRPM: https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/fedora-rawhide-x86_64/00967221-charliecloud/charliecloud-0.9.10-5.fc31.src.rpm

Best,
Jordan

Comment 25 Dave Love 2019-07-12 09:59:27 UTC
I modified the spec as I was expecting to build the docs on el7 (which seem to come out OK), list the licences for what's bundled there, and use unbundled bits, apart from fonts which aren't available in el7.  (I expect other things using sphinx should be doing the same, but having spotted it, I thought I should address it.)

More fundamentally, I tried the tests according to the instructions (in a centos7.1810 x86_64 vm).
make test-run fails, with several other errors after this:

 - ch-run as root: run image (skipped: standard scope)
 [sudo] password for vagrant: on-zero gid refused                        74/120
 ✗ ch-run as root: root with non-zero gid refused
   (in test file run/ch-run_escalated.bats, line 70)
     `[[ $output = *'error ('* ]]' failed
   Sorry, user vagrant is not allowed to execute '/usr/bin/ch-run -v --version' as root:vagrant on localhost.localdomain.

(I have password-less sudo in the vm.)

I may not be able to investigate until next week; suggestions welcome in the meantime.

Another problem is that although Orion said he'd sponsored you, I noticed the ticket is still blocked on needssponsor.  I don't know how to check whether that's a problem with bugzilla or know whether there's anything that can be done before Orion returns, but can try to find out.

Continued apologies for the time this is taking. I'd also like to get it through as I've been recommending charliecloud, and thought I was better-placed to do it than I seem to be.

Comment 26 Dave Love 2019-07-12 10:50:21 UTC
OK, I've removed the FE-NEEDSPONSOR. I thought that was mechanized, but obviously not. Having remembered how to check, you do have the right privileges to maintain the package.

Comment 27 jogas 2019-07-12 17:04:29 UTC
(In reply to Dave Love from comment #25)
> More fundamentally, I tried the tests according to the instructions (in a
> centos7.1810 x86_64 vm).
> make test-run fails, with several other errors after this:
> 
>  - ch-run as root: run image (skipped: standard scope)
>  [sudo] password for vagrant: on-zero gid refused                       
> 74/120
>  ✗ ch-run as root: root with non-zero gid refused
>    (in test file run/ch-run_escalated.bats, line 70)
>      `[[ $output = *'error ('* ]]' failed
>    Sorry, user vagrant is not allowed to execute '/usr/bin/ch-run -v
> --version' as root:vagrant on localhost.localdomain.
> 
> (I have password-less sudo in the vm.)

This particular error occurs because the user who runs the test suite needs
to be a member of two groups and has a sudoer config that allows the secondary
group to sudo without a password. This requirement isn't covered very well
in section 2.2.2 (https://hpc.github.io/charliecloud/test.html#run).

The error message doesn't convey the issue. I've seen this error trip people up
enough times now to realize we need to file a bug; this test should skip if
the group requirements aren't met (bug filed: https://github.com/hpc/charliecloud/issues/485).

You mentioned you had more errors after this one, would you mind posting
them? There could be subtle things like this I should mention in the test
readme.


Best,
Jordan

Comment 28 Dave Love 2019-07-12 19:48:37 UTC
Created attachment 1590038 [details]
make test-run log

OK, here's the complete test log.  I haven't had a chance to look into it.
This was in a virtualbox vagrant VM with an up-to-date centos7 system.
The account is:
[vagrant@localhost test]$ id vagrant
uid=1000(vagrant) gid=1000(vagrant) groups=1000(vagrant),135(mock)

There's a nopasswd sudoers rule specifically for vagrant.

Comment 29 Dave Love 2019-07-15 10:45:00 UTC
I commented on the github bug report. I was just following README.TEST, which doesn't mention the requirement, though it was satisfied.

Comment 30 Dave Love 2019-07-19 15:37:54 UTC
Just to record what I said in mail, I think it's OK to drop the test package, pending figuring out what the issue is with that, as Charliecloud works in anger.

Another minor thing I noticed:  it's better to use the "https://github.com/<org>/%name/archive/%version/%name-%version.tar.gz" form for Source0 that's suggested in the packager doc.  Then you get an obviously-named tarball (especially under ~/rpmbuild).

Comment 31 jogas 2019-07-19 22:33:50 UTC
Dave,

Below is 0.9.10-6. I have removed the test suite (for now) and updated source0. I have a variation with the test suite saved, I hope to include in 0.10 or 0.11 once we can work the kinks out.

SPEC: https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/fedora-rawhide-x86_64/00975513-charliecloud/charliecloud.spec
SRPM: https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/fedora-rawhide-x86_64/00975513-charliecloud/charliecloud-0.9.10-6.fc31.src.rpm

Best,
Jordan

Comment 32 Dave Love 2019-07-22 12:48:23 UTC
Sorry to go on about it, but I think you still need to attend to the doc package.  It's bundling things which are packaged, and under licences that aren't listed (although I suspect others do the same).
Also, is there actually something wrong with building the doc on el7 as I did?
A minor rpmlint complaint: the doc package summary shouldn't have a full stop ^W^Wperiod.

Comment 33 jogas 2019-07-22 16:15:38 UTC
Morning Dave, here is another iteration:

SPEC: https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/fedora-rawhide-x86_64/00976340-charliecloud/charliecloud.spec
SRPM: https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/fedora-rawhide-x86_64/00976340-charliecloud/charliecloud-0.9.10-7.fc31.src.rpm

I moved the sphinx and sphinx-rtd_theme package requirements under the doc subpackage. I've also added the sed lines you used to modify the doc-src Makefile to ignore warnings (aside from the 'with doc' conditional, the handling is very similar). Finally, the period from doc subpackage summary has been removed.

Now, regarding your other doc package concerns, I guess I do not understand what the issue(s) is (are). We list sphinx and sphinx-rtd_theme as package dependencies. These packages supply their own licenses. Our doc-src Makefile and rsts (ASL 2.0) use these other packages to generate man pages and html files. Are you saying the resulting documentation artifacts fall under the sphinx MIT license? And that their licenses need to be listed in /usr/share/licenses/charliecloud-doc-<version>? Apologies if I am missing something trivial or am misinterpreting you. 

Best,
Jordan

Comment 34 Dave Love 2019-07-22 20:59:17 UTC
Apologies for not being clear.  The doc packages bundle various files (font, css, and js, I think) that are packaged -- see my example for what I found.  They presumably come under the packager instructions for bundling.  I don't think they need to be bundled, but if they are, there need to be "Provides: bundled..." in the spec as well as listing the other licences (although they're all BSD/MIT-ish, I think).
I may have that wrong, and should maybe check with the -devel list.
Is that clearer?

Comment 35 jogas 2019-07-22 22:49:29 UTC
Aha! OK, I understand what you are saying now.

It appears that our needed fonts, css, and js bits get installed to /usr/lib/$python/site-packages/sphinx_rtd_theme/static/{fonts, css, js (respectively) } when the -doc package dependencies are installed. I can remove our duplicate bundled versions, e.g., doc/_static/fonts, doc/_static/css, and doc/_static/js, and replace them with symlinks to the packaged versions at /usr/lib/$python/site-packages/sphinx_rtd_theme/static/{fonts, css, js} at %build or %install time.

Note the html files work perfectly with the symlinks. Would this, in addition to adding the BSD and MIT license to -doc be sufficient? The other files in doc are unique to our package (i.e., generated with sphinx); would we still need the virtual provides?

Best,
Jordan

Comment 36 jogas 2019-07-24 17:05:44 UTC
Dave,

See the following:

SPEC: https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/fedora-rawhide-x86_64/00977732-charliecloud/charliecloud.spec
SRPM: https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/fedora-rawhide-x86_64/00977732-charliecloud/charliecloud-0.9.10-8.fc31.src.rpm

The doc package no longer bundles the font, css, and js artifacts already provided by
sphinx_rtd_theme. We now provide symlinks to these artifacts in %{_docdir}/%{name}-doc-%{verson}/html/_static.

I also added the BSD and MIT license to the doc subpackage license field.

Best,
Jordan

Comment 37 Dave Love 2019-07-25 15:11:14 UTC
Sorry I don't have time to check this properly today. A couple of things immediately, though. Doc should be in %_pkgdocdir (implicitly if you use "%doc ..." to install it). There's a /usr/lib/... which should probably be replaced by something like %python_sitelib though I'd have to check how to make that work across epel/fedora; at least the warning should be defeated with %_prefix/lib if necessary.

I'll try to have a proper go at it tomorrow.  I still don't understand why you can't ship the doc on el7.

Comment 38 jogas 2019-07-25 22:27:18 UTC
Dave,

No worries. Tomorrow, when you have an opportunity to test, use the following:

SPEC: https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/fedora-rawhide-x86_64/00978631-charliecloud/charliecloud.spec
SRPM: https://copr-be.cloud.fedoraproject.org/results/jogas/charliecloud/fedora-rawhide-x86_64/00978631-charliecloud/charliecloud-0.9.10-9.fc31.src.rpm

We now use _pkgdocir; _prefix for the site-package variable (note that %{python_sitelib} and %{python3_sitelib} expanded to nothing inside the %install section, thus I settled on using %{_prefix}); and provide html documentation on epel.

Thanks again Dave.

Comment 39 Dave Love 2019-07-27 11:20:28 UTC
OK, I've approved it, finally. I could be a co-maintainer if you'd like, and I can try to help with the test suite business later.

Comment 40 jogas 2019-08-02 16:59:55 UTC
(In reply to Dave Love from comment #39)
> OK, I've approved it, finally. I could be a co-maintainer if you'd like, and
> I can try to help with the test suite business later.

Great!

I've been out on travel this past week. I will continue working on this on Monday.

Re. co-maintenance: sure. I am also going to support a package for umoci soon, any interest in co-maintaining that as well?

Comment 41 Gwyn Ciesla 2019-08-02 19:01:35 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/Charliecloud

Comment 42 Gwyn Ciesla 2019-08-07 19:48:55 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/charliecloud

Comment 43 Fedora Update System 2019-08-08 14:30:21 UTC
FEDORA-EPEL-2019-c7b6c39f2f has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2019-c7b6c39f2f

Comment 44 Fedora Update System 2019-08-09 00:11:07 UTC
charliecloud-0.9.10-10.el7 has been pushed to the Fedora EPEL 7 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-EPEL-2019-c7b6c39f2f

Comment 45 Fedora Update System 2019-08-21 21:12:10 UTC
FEDORA-EPEL-2019-13ac8917cd has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2019-13ac8917cd

Comment 46 Fedora Update System 2019-08-22 04:03:54 UTC
charliecloud-0.9.10-11.el7 has been pushed to the Fedora EPEL 7 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-EPEL-2019-13ac8917cd

Comment 47 Fedora Update System 2019-08-22 14:38:45 UTC
FEDORA-EPEL-2019-13ac8917cd has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2019-13ac8917cd

Comment 48 Fedora Update System 2019-08-23 02:14:38 UTC
charliecloud-0.9.10-12.el7 has been pushed to the Fedora EPEL 7 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-EPEL-2019-13ac8917cd

Comment 49 Fedora Update System 2019-09-06 13:26:15 UTC
charliecloud-0.9.10-12.el7 has been pushed to the Fedora EPEL 7 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.