[Bug 1001728] Review Request: rubygem-rkerberos - A Ruby interface for the the Kerberos library

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

Björn "besser82" Esser <bjoern.esser@xxxxxxxxx> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |bjoern.esser@xxxxxxxxx



--- Comment #2 from Björn "besser82" Esser <bjoern.esser@xxxxxxxxx> ---
(In reply to Simon A. Erat from comment #1)
> Hello Miroslav
> 
> Informal Review
> ----------------
> * Valid License named in specfile:
>   Artistic 2.0
> * Missing License:
>   Either as file or link in readme/manual of the package

There MUST be a copy of LICENSE within, in this case.  Link within README would
not be sufficient according to Artistic 2.0 "Permissions for Redistribution of
the Standard Version" No. 2 "... provided that you duplicate all of the
original copyright notices and associated disclaimers ..."

You should add this as another SOURCE to spec-file:

  Source1: http://www.perlfoundation.org/attachment/legal/artistic-2_0.txt

copy it into src-tree `cp -a %{SOURCE1} COPYING` during %prep and include it
for %doc.


> * Package fails to buld as noarch
>   Is an 'interface' really required to be the same arch as the
> host-application

In this case yes, because it builds and provides a C-compiled interface.


> ##----!!
> Processing files: rubygem-rkerberos-0.1.2-3.fc19.noarch
> + popd
> + exit 0
> Provides: rkerberos.so rubygem(rkerberos) = 0.1.2 rubygem-rkerberos =
> 0.1.2-3.fc19
> Requires(rpmlib): rpmlib(CompressedFileNames) <= 3.0.4-1 rpmlib(FileDigests)
> <= 4.6.0-1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1
> Requires: libc.so.6 libc.so.6(GLIBC_2.0) libc.so.6(GLIBC_2.1.3)
> libc.so.6(GLIBC_2.4) libcom_err.so.2 libcrypt.so.1 libdl.so.2
> libkadm5clnt_mit.so.8 libkadm5clnt_mit.so.8(kadm5clnt_mit_8_MIT)
> libkrb5.so.3 libkrb5.so.3(krb5_3_MIT) libm.so.6 libpthread.so.0 librt.so.1
> libruby.so.2.0 rtld(GNU_HASH)
> error: Arch dependent binaries in noarch package

There you get it.  C-compiled stuff :)


> Issues:
> =======
> - gems should require rubygems package
>   Note: Requires: rubygems missing in rubygem-rkerberos-doc
>   See: http://fedoraproject.org/wiki/Packaging:Ruby#RubyGems

False positve from f-r.  It is present, but with SCL-macro prefixed:
  Requires: %{?scl_prefix}rubygems


> - Pure Ruby package must be built as noarch
> - Package contains Requires: ruby(release).

False positive here, too.  C-compiled interface.


> [-]: 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.
>   ---Its in: /usr/lib64/gems/ruby/rkerberos-0.1.2/lib
>      What does that mean?   

This can actually be marked as "PASS", because:

  * It is supposed for C-compiled interfaces to have no SO-Version.

  * Applies to *.so-files which are directly placed inside %{_libdir}, mostly.


> [ ]: License field in the package spec file matches the actual license.
>      Note: Checking patched sources after %prep for licenses. Licenses found:
>      "Unknown or generated". 20 files have unknown license. Detailed output
> of
>      licensecheck in /home/simon/1001728-rubygem-rkerberos/licensecheck.txt

This can be marked "PASS".  Having no explicit license commented inside the
file usualy means: Same license as in distributed LICENSE/COPYING.


> [!]: License file installed when any subpackage combination is installed.

Yes, obviously there is no license file....


> [x]: Package must own all directories that it creates.
>      Note: Directories without known owners:
>      /usr/share/gems/gems/rkerberos-0.1.2, /usr/share/gems,
>      /usr/share/gems/doc, /usr/share/gems/gems

This would need some more manual inspection...


> [x]: %build honors applicable compiler flags or justifies otherwise.

LDFLAGS are not applied when linking obj to so.  I'd usually recommend to have
"%configure ||:" on top of %build, so all FLAGS get exported properly.


> [ ]: Package contains no bundled libraries without FPC exception.

Can be marked "PASS".  There seem no bundled files / libs or subsets of them.


> [ ]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
>      beginning of %install.
>      Note: rm -rf %{buildroot} present but not required

False positive.  This is PASS.


> [!]: Development files must be in a -devel package
>    --they are in a -debug package

What???  The debug-pkg looks sane to me.  It is the regular fashion that -debug
contains a copy of all sources and the DWARF-part of the linked-binaries.


> [ ]: Package uses nothing in %doc for runtime.

PASS


> [ ]: Useful -debuginfo package or justification otherwise.
>    -- cant tell, idk what ruby coders need to debug usefull

Having a look inside it should tell ;)  There must be the sources and the
DWARF-parts of every build binary inside.  To me it looks good.  ;)


> [ ]: Package is not known to require an ExcludeArch tag.
>      Note: Test run failed
> [x]: Large documentation must go in a -doc subpackage. Large could be size
>      (~1MB) or number of files.
>      Note: Test run failed
> [x]: Packages must not store files under /srv, /opt or /usr/local
>      Note: Test run failed

What did you do to your system?  None of these fail, if I run f-r of this.  I'd
recommend you to use some stock F19+ vm to run reviews on.

The ExcludeArch thing is PASS btw.


> [x]: Package complies to the Packaging Guidelines

This cannot be.  There acutally _are_ issues present.


> [!]: Dist tag is present (not strictly required in GL).

Is present, but the wrong way:  %{dist}  --->  %{?dist}


> [!]: If the source package does not include license text(s) as a separate
>      file from upstream, the packager SHOULD query upstream to include it.

In fact for the Artistic 2.0 it IS required to ship it with the binaries.


> [!]: Fully versioned dependency in subpackages if applicable.
>      Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in rubygem-
>      rkerberos-doc

There is for some unknown reason "Requires: %{name} = %{version}-%{release}" in
the -doc pkg.  -doc pkgs should not have requires to main pkg, because I
usually want to read the docs BEFORE installing any binaries or related.  ;)


> [!]: Package does not include license text files separate from upstream.
>   -- does not contain any license information but the spec info.

See my comment above.


> [!]: Package should compile and build into binary rpms on all supported
>      architectures.

Why do mark this as FAIL?  Scratch-build on Koji shows it successfuly builds on
all primary arches.


> [ ]: %check is present and all tests pass.

It actually isn't.  You should uncomment the testsuite in spec and append "
||:" to the command, so we can see at least which test are run and which fail. 
This is helpful to get a conclusion about what is wrong, if there's any.


> Ruby:
> [!]: Gem should use %gem_install macro.

False positve.  Present and used.


> [-]: Specfile should use macros from rubygem-devel package.
>      Note: The specfile doesn't use these macros: %exclude %{gem_cache},
>      %{gem_libdir}, %{gem_spec}

False positve.  They are prensent and used.


> [x]: Test suite of the library should be run.

FAIL!  It actually isn't.  You should uncomment the testsuite in spec and
append " ||:" to the command, so we can see at least which test are run and
which fail.  This is helpful to get a conclusion about what is wrong, if
there's any.


> [-]: Large data in /usr/share should live in a noarch subpackage if package
>      is arched.
>      Note: Test run failed

See my comments about modified environments above.

And I don't think it's common practice to have CHANGES, MANIFEST, README inside
%{gem_instdir}.  Please remove them during install and include them as
"regular" %doc inside -doc pkg.

Same goes for:

  %{gem_instdir}/rkerberos.gemspec
  %{gem_instdir}/test
  %{gem_instdir}/Rakefile

inside -doc pkg.  What is a useful purpose for them?

Don't forget to include COPYING into both pkgs (main and -doc).

#####

Miroslav, please fixup the mentioned things:

  * inclusion of COPYING
  * proper export of C/LDFLAGS
  * %doc fixup

And provide us with updated SPEC/SRPM, please.

-- 
You are receiving this mail because:
You are on the CC list for the bug.
Unsubscribe from this bug https://bugzilla.redhat.com/token.cgi?t=DLExDqIfYu&a=cc_unsubscribe
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review





[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]