Bug 1187713 - Review Request: netty-tcnative - Fork of Tomcat Native with improved OpenSSL and mavenized build
Summary: Review Request: netty-tcnative - Fork of Tomcat Native with improved OpenSSL ...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Zbigniew Jędrzejewski-Szmek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: 1187718
TreeView+ depends on / blocked
 
Reported: 2015-01-30 18:01 UTC by jiri vanek
Modified: 2015-02-18 17:56 UTC (History)
4 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2015-02-12 11:37:03 UTC
Type: Bug
Embargoed:
zbyszek: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Comment 1 Zbigniew Jędrzejewski-Szmek 2015-01-30 18:56:48 UTC
Remove dot from summary.

BuildRequires:  java-devel should not be needed.

=======
- All build dependencies are listed in BuildRequires, except for any that are
  listed in the exceptions section of Packaging Guidelines.
  Note: These BR are not needed: make tar
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2

In principle this could be removed, but is allowed.


[ ]: Package requires other packages for directories it uses.
     Note: No known owner of /usr/share/maven-poms/netty-tcnative,
     /usr/lib/java/netty-tcnative
[ ]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/share/maven-poms/netty-
     tcnative, /usr/lib/java/netty-tcnative
Java:
[x]: Bundled jar/class files should be removed before build
[x]: Packages have proper BuildRequires/Requires on jpackage-utils
     Note: Maven packages do not need to (Build)Require jpackage-utils. It is
     pulled in by maven-local
Please remove.

[x]: Javadoc documentation files are generated and included in -javadoc
     subpackage
Yes.

[x]: Javadoc subpackages should not have Requires: jpackage-utils
This should be fixed.

[x]: Javadocs are placed in %{_javadocdir}/%{name} (no -%{version} symlink)

===== SHOULD items =====

Generic:
[!]: Sources can be downloaded from URI in Source: tag
     Note: Could not download Source0: https://github.com/netty/netty-
     tcnative/releases/tag/netty-tcnative-1.1.30.Fork2.tar.gz
This link does not work.

[ ]: 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).
No, see above.

[ ]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in netty-
     tcnative-javadoc
OK.

Java:
[ ]: Packages are noarch unless they use JNI
     Note: netty-tcnative subpackage is not noarch. Please verify manually
Should be noarch.

===== 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)
[ ]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
     Note: Arch-ed rpms have a total of 2396160 bytes in /usr/share

Rpmlint
-------
Checking: netty-tcnative-1.1.30-0.fc22.x86_64.rpm
          netty-tcnative-javadoc-1.1.30-0.fc22.x86_64.rpm
          netty-tcnative-1.1.30-0.fc22.src.rpm
netty-tcnative.x86_64: W: summary-ended-with-dot C Netty-tcnative is a fork of Tomcat Native.
Should be fixed.

netty-tcnative.x86_64: W: name-repeated-in-summary C Netty-tcnative
Should be fixed.

netty-tcnative.x86_64: W: spelling-error %description -l en_US mavenization -> magnetization, humanization, maximization
netty-tcnative.x86_64: W: incoherent-version-in-changelog 1.1.30.Fork2.0 ['1.1.30-0.fc22', '1.1.30-0']
Should be fixed.

netty-tcnative.x86_64: E: no-binary
netty-tcnative.x86_64: W: only-non-binary-in-usr-lib
netty-tcnative.x86_64: W: no-documentation
netty-tcnative.src: W: summary-ended-with-dot C Netty-tcnative is a fork of Tomcat Native.
netty-tcnative.src: W: name-repeated-in-summary C Netty-tcnative
netty-tcnative.src: W: spelling-error %description -l en_US mavenization -> magnetization, humanization, maximization
netty-tcnative.src: W: strange-permission netty-tcnative-1.1.30.Fork2.tar.gz 0640L
netty-tcnative.src: W: invalid-url Source0: https://github.com/netty/netty-tcnative/releases/tag/netty-tcnative-1.1.30.Fork2.tar.gz HTTP Error 404: Not Found
3 packages and 0 specfiles checked; 1 errors, 11 warnings.




Rpmlint (installed packages)
----------------------------
Cannot parse rpmlint output:


Diff spec file in url and in SRPM
---------------------------------
--- /var/tmp/1187713-netty-tcnative/srpm/netty-tcnative.spec	2015-01-30 13:38:39.140804828 -0500
+++ /var/tmp/1187713-netty-tcnative/srpm-unpacked/netty-tcnative.spec	2015-01-29 10:33:27.000000000 -0500
@@ -10,8 +10,4 @@
 Source0:        https://github.com/netty/netty-tcnative/releases/tag/%{name}-%{namedversion}.tar.gz
 
-#dont know how to configure requires, just guessing
-Requires:  java-headless
-Requires:  apr
-Requires:  openssl
 
 BuildRequires:  maven-local
@@ -25,8 +21,7 @@
 BuildRequires:  openssl-devel
 BuildRequires:  java-devel
-BuildRequires:  maven-hawtjni-plugin
 #parent pom is needed
 BuildRequires:  netty 
-
+BuildRequires:  maven-hawtjni-plugin
 
 %description


Requires
--------
netty-tcnative (rpmlib, GLIBC filtered):
    java-headless
    jpackage-utils

netty-tcnative-javadoc (rpmlib, GLIBC filtered):
    jpackage-utils

Should be removed.

Provides
--------
netty-tcnative:
    mvn(io.netty:netty-tcnative)
    mvn(io.netty:netty-tcnative:pom:)
    netty-tcnative
    netty-tcnative(x86-64)
    osgi(io.netty.tcnative)

netty-tcnative-javadoc:
    netty-tcnative-javadoc
    netty-tcnative-javadoc(x86-64)

Should be noarch.

Comment 2 jiri vanek 2015-01-31 05:41:09 UTC
Hi!

Thank you for review. I will get back to this next week. However. You are writing this should be made noarch - but in fact, it is native package. But... my jar do not contains (correctly) the native parts. Only classes. However not even rpm is containing them => So I made it wrongly and forget to pack the native parts completly!

In next iteration the javadoc and main jar will be noarch, but native lib will be regular ARCHed one.

As for url, it is : https://github.com/netty/netty-tcnative/archive/netty-tcnative-1.1.30.Fork2.tar.gz
not sure how it slipped...

mea culpa!

Comment 3 Zbigniew Jędrzejewski-Szmek 2015-01-31 13:27:31 UTC
(In reply to jiri vanek from comment #2)
> Thank you for review. I will get back to this next week. However. You are
> writing this should be made noarch - but in fact, it is native package.
> But... my jar do not contains (correctly) the native parts. Only classes.
> However not even rpm is containing them => So I made it wrongly and forget
> to pack the native parts completly!
OK.

> In next iteration the javadoc and main jar will be noarch, but native lib
> will be regular ARCHed one.
Maybe if the native part is small (or the jar is small) it doesn't make sense to split. It certainly is not required.

Comment 4 jiri vanek 2015-02-04 16:22:22 UTC
Updated packages/sources:

src rpm: https://jvanek.fedorapeople.org/elasticsearch/v4/netty-tcnative/netty-tcnative-1.1.30-0.fc22.src.rpm

spec: https://jvanek.fedorapeople.org/elasticsearch/v4/netty-tcnative/netty-tcnative.spec

rpms: https://jvanek.fedorapeople.org/elasticsearch/v4/netty-tcnative/

I hope also the native lib issues are fixed (and of course all your nits). Please note following two sources were added:
https://jvanek.fedorapeople.org/elasticsearch/v4/netty-tcnative/fixLibNames.patch
Its pure existence is really really suspiciius!
But see the test...
https://jvanek.fedorapeople.org/elasticsearch/v4/netty-tcnative/CheckLibrary.java
and its etting in %check... HMM:(

Comment 5 Mikolaj Izdebski 2015-02-04 16:36:44 UTC
First, Java native libraries (JNI) should not be installed directly in %{_libdir} - they are not usable outsides Java world and as such they should be placed in subdirectory of %{_libdir}. Secondly, packages in Fedora should use System.load() instead of System.loadLibrary() to load JNI libraries. See:
http://fedoraproject.org/wiki/Packaging:Java#Packaging_JAR_files_that_use_JNI

It would also be good to run included tests or at least comment why they are skipped.

Comment 6 Mikolaj Izdebski 2015-02-04 16:40:15 UTC
Also note that this is a fork of tomcat-native, which is already included in Fedora.

Comment 7 jiri vanek 2015-02-04 16:51:00 UTC
(In reply to Mikolaj Izdebski from comment #6)
> Also note that this is a fork of tomcat-native, which is already included in
> Fedora.

Sure, but the reason for inclusion is here.

Comment 8 jiri vanek 2015-02-04 16:54:26 UTC
(In reply to Mikolaj Izdebski from comment #5)
> First, Java native libraries (JNI) should not be installed directly in
> %{_libdir} - they are not usable outsides Java world and as such they should
> be placed in subdirectory of %{_libdir}. Secondly, packages in Fedora should
> use System.load() instead of System.loadLibrary() to load JNI libraries. See:
> http://fedoraproject.org/wiki/Packaging:Java#Packaging_JAR_files_that_use_JNI

Yes. But:
 * Usage of system.load will require much more patching
 * tomcat is putting its libraris directly to  %{_libdir} too. As this is for, I inclined to keep the same schema.

Those two points are strongly connected, and I have been really thinking which one to use. Really a lot. Tried this and yours agian mine and then that.. At the end this was most strightforwad
 * little of patching
 * most imiliar to upstream behaviour
 * most close to tomcat.

Unless you insists, I would like to keep with this approach.
> 
> It would also be good to run included tests or at least comment why they are
> skipped.

Ugh. I will double check this.

Comment 9 Mikolaj Izdebski 2015-02-04 17:04:26 UTC
(In reply to jiri vanek from comment #7)
> (In reply to Mikolaj Izdebski from comment #6)
> > Also note that this is a fork of tomcat-native, which is already included in
> > Fedora.
> 
> Sure, but the reason for inclusion is here.

Sure, but I would consider adding required changes to tomcat-native (if possible) instead of packaging a fork.

(In reply to jiri vanek from comment #8)
>  * Usage of system.load will require much more patching

It's 10-line patch, not that much IMHO. An example is provided in packaging guidelines I linked above.

>  * tomcat is putting its libraris directly to  %{_libdir} too. As this is
> for, I inclined to keep the same schema.

That's a bug in tomcat-native. There is no reason to duplicate the same bug in another package.

> Those two points are strongly connected, and I have been really thinking
> which one to use. Really a lot. Tried this and yours agian mine and then
> that.. At the end this was most strightforwad
>  * little of patching
>  * most imiliar to upstream behaviour
>  * most close to tomcat.
> 
> Unless you insists, I would like to keep with this approach.

The right way is the one described in packaging guidelines - not putting .so file directly in %{_libdir} and loading it using System.load(). But OFC Zbyszek has the final word on this.

Comment 10 jiri vanek 2015-02-05 08:22:40 UTC
(In reply to jiri vanek from comment #4)
> Updated packages/sources:
> 
> src rpm:
> https://jvanek.fedorapeople.org/elasticsearch/v4/netty-tcnative/netty-
> tcnative-1.1.30-0.fc22.src.rpm
> 
> spec:
> https://jvanek.fedorapeople.org/elasticsearch/v4/netty-tcnative/netty-
> tcnative.spec
> 
> rpms: https://jvanek.fedorapeople.org/elasticsearch/v4/netty-tcnative/
> 
> I hope also the native lib issues are fixed (and of course all your nits).
> Please note following two sources were added:
> https://jvanek.fedorapeople.org/elasticsearch/v4/netty-tcnative/fixLibNames.
> patch
> Its pure existence is really really suspiciius!
> But see the test...
> https://jvanek.fedorapeople.org/elasticsearch/v4/netty-tcnative/CheckLibrary.
> java
> and its etting in %check... HMM:(

https://github.com/netty/netty-tcnative/issues/23

Comment 11 Zbigniew Jędrzejewski-Szmek 2015-02-06 00:28:57 UTC
The major question is whether this package should be built at all, i.e. if the fork is substantial enough. From what I can see, the answer is yes: the sources are completely rearranged, the build system is different, and according to http://netty.io/wiki/forked-tomcat-native.html there are various fixes.

Second question is what to do with the guidelines which say that java libraries should not be installed in %{_libdir}. Personally, I think that those guidelines are a waste of packagers' time. But since it's the law, I would very much prefer to abide by them, unless it is impossible for some reason. Looking at o.a.t.j.Library.java in the sources, it should be a trivial patch. If you go for the Library() constructor, it should be a one-line thing.

.la files are generally considered harmful. Is there some specific reason to install it?

Comment 12 jiri vanek 2015-02-06 11:01:08 UTC
(In reply to Zbigniew Jędrzejewski-Szmek from comment #11)
> The major question is whether this package should be built at all, i.e. if
> the fork is substantial enough. From what I can see, the answer is yes: the
> sources are completely rearranged, the build system is different, and
> according to http://netty.io/wiki/forked-tomcat-native.html there are
> various fixes.

yes. Thank you for confirmation.

> 
> Second question is what to do with the guidelines which say that java
> libraries should not be installed in %{_libdir}. Personally, I think that
> those guidelines are a waste of packagers' time. But since it's the law, I
> would very much prefer to abide by them, unless it is impossible for some
> reason. Looking at o.a.t.j.Library.java in the sources, it should be a
> trivial patch. If you go for the Library() constructor, it should be a
> one-line thing.

Ok I will revisit.

> 
> .la files are generally considered harmful. Is there some specific reason to
> install it?

No. It was in build image. I will remove it.

Comment 13 jiri vanek 2015-02-06 13:51:37 UTC
Directory updated:

spec: https://jvanek.fedorapeople.org/elasticsearch/v4/netty-tcnative/netty-tcnative.spec

srpm: https://jvanek.fedorapeople.org/elasticsearch/v4/netty-tcnative/netty-tcnative-1.1.30-0.fc21.src.rpm

updated patch: https://jvanek.fedorapeople.org/elasticsearch/v4/netty-tcnative/fixLibNames.patch.in

The test file is still valid, but only for manual work.I had verified on x86_64 system. Then both my mock roots failed with brokne gcc/libtools[1]
https://jvanek.fedorapeople.org/elasticsearch/v4/netty-tcnative/CheckLibrary.java
So I dont have RPMS for you this time, and if you wont to rebuild, dont clean your mock-roots:)

All issues shuld be fixed.


[1]
Error: Package: libtool-2.4.2-31.fc22.x86_64 (fedora)
           Requires: gcc = 4.9.2
           Installed: gcc-5.0.0-0.7.fc22.x86_64 (@fedora)
               gcc = 5.0.0-0.7.fc22

Comment 14 jiri vanek 2015-02-06 13:52:24 UTC
btw, why are .la dangerous?

Comment 15 Zbigniew Jędrzejewski-Szmek 2015-02-06 14:04:45 UTC
(In reply to jiri vanek from comment #14)
> btw, why are .la dangerous?
They aren't dangerous, but they are useless on Linux (linking works just fine without them, so they just obfuscate), but they shouldn't be removed in stable updates (iiuc, other .la files created based on this library will refer to the .la file). The only reason they are often installed is that libtool cannot be told not to install them.

Comment 16 Zbigniew Jędrzejewski-Szmek 2015-02-06 15:28:04 UTC
The patch to load libraries is wrong. It makes the jar file architecture dependent,
but it should not be. Just try loading from /usr/lib64, and then from /usr/lib.
This will work on both 64 and 32 bit archs.

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

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[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]: 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]: 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 %doc.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "Apache (v2.0)", "*No copyright* Apache (v2.0)". Detailed output of
     licensecheck in /var/tmp/1187713-netty-tcnative/licensecheck.txt
[!]: License file installed when any subpackage combination is installed.
No license is installed with -javadocs.

[!]: Package requires other packages for directories it uses.
     Note: No known owner of /usr/share/maven-poms/netty-tcnative, /usr/lib64
     /netty-tcnative, /usr/lib/java/netty-tcnative
[!]: Package must own all directories that it creates.
     Note: Directories without known owners: /usr/share/maven-poms/netty-
     tcnative, /usr/lib64/netty-tcnative, /usr/lib/java/netty-tcnative
Those should be added.

[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.
[-]: 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]: 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
[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]: Package does not own files or directories owned by other packages.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[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]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install' ' DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package do not use a name that already exist
[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.

Java:
[x]: Bundled jar/class files should be removed before build
     Note: Test run failed
[x]: Packages have proper BuildRequires/Requires on jpackage-utils
     Note: Maven packages do not need to (Build)Require jpackage-utils. It is
     pulled in by maven-local
[x]: Javadoc documentation files are generated and included in -javadoc
     subpackage
[x]: Javadoc subpackages should not have Requires: jpackage-utils
[x]: Javadocs are placed in %{_javadocdir}/%{name} (no -%{version} symlink)

Maven:
[x]: If package contains pom.xml files install it (including depmaps) even
     when building with ant
[x]: POM files have correct Maven mapping
[x]: Maven packages should use new style packaging
[x]: Old add_to_maven_depmap macro is not being used
[x]: Packages DO NOT have Requires(post) and Requires(postun) on jpackage-
     utils for %update_maven_depmap macro
[x]: Package DOES NOT use %update_maven_depmap in %post/%postun
[x]: Packages use %{_mavenpomdir} instead of %{_datadir}/maven2/poms

===== 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.
[!]: Final provides and requires are sane (see attachments).
See comments below

[x]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in netty-
     tcnative-javadoc
Not needed.

[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: Patches link to upstream bugs/comments/lists or are otherwise justified.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[?]: Package should compile and build into binary rpms on all supported
     architectures.
I don't think it would actually build, because of the binary conflict.

[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[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]: Dist tag is present (not strictly required in GL).
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

Java:
[x]: Packages are noarch unless they use JNI
     Note: netty-tcnative subpackage is not noarch.

[x]: Package uses upstream build method (ant/maven/etc.)

===== EXTRA items =====

Generic:
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Package should not use obsolete m4 macros
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: netty-tcnative-1.1.30-0.fc22.x86_64.rpm
          netty-tcnative-javadoc-1.1.30-0.fc22.x86_64.rpm
          netty-tcnative-1.1.30-0.fc22.src.rpm
netty-tcnative.x86_64: W: name-repeated-in-summary C Netty-tcnative
netty-tcnative.x86_64: W: spelling-error %description -l en_US mavenization -> magnetization, humanization, maximization
netty-tcnative.x86_64: W: incoherent-version-in-changelog 1.1.30.Fork2.0 ['1.1.30-0.fc22', '1.1.30-0']
???

netty-tcnative.x86_64: W: no-documentation
netty-tcnative.src: W: name-repeated-in-summary C Netty-tcnative
rpmlint is right here. The summary is meaningless.

netty-tcnative.src: W: spelling-error %description -l en_US mavenization -> magnetization, humanization, maximization
netty-tcnative.src: W: strange-permission netty-tcnative-1.1.30.Fork2.tar.gz 0640L
netty-tcnative.src:68: W: macro-in-comment %{_jnidir}
netty-tcnative.src:68: W: macro-in-comment %{name}
netty-tcnative.src:68: W: macro-in-comment %{name}
Please fix those.

netty-tcnative.src:62: W: mixed-use-of-spaces-and-tabs (spaces: line 4, tab: line 62)
And this.

netty-tcnative.src: W: patch-not-applied Patch1: fixLibNames.patch.in
3 packages and 0 specfiles checked; 0 errors, 12 warnings.
OK.


Rpmlint (installed packages)
----------------------------
Cannot parse rpmlint output:


Requires
--------
netty-tcnative (rpmlib, GLIBC filtered):
    apr
    java-headless
    jpackage-utils
    libapr-1.so.0()(64bit)
    libc.so.6()(64bit)
    libcrypto.so.10()(64bit)
    libcrypto.so.10(OPENSSL_1.0.1)(64bit)
    libcrypto.so.10(OPENSSL_1.0.1_EC)(64bit)
    libcrypto.so.10(libcrypto.so.10)(64bit)
    libdl.so.2()(64bit)
    libpthread.so.0()(64bit)
    libssl.so.10()(64bit)
    libssl.so.10(libssl.so.10)(64bit)
    openssl
This also seems unecessary. IIUC, only ssl libs are required, and that dependency is provided automatically.

    rtld(GNU_HASH)

netty-tcnative-javadoc (rpmlib, GLIBC filtered):
    jpackage-utils
This seems unnecessary.


Provides
--------
netty-tcnative:
    libnetty-tcnative-1.1.30.Fork2.so()(64bit)
    mvn(io.netty:netty-tcnative)
    mvn(io.netty:netty-tcnative:pom:)
    netty-tcnative
    netty-tcnative(x86-64)
    osgi(io.netty.tcnative)

netty-tcnative-javadoc:
    netty-tcnative-javadoc
    netty-tcnative-javadoc(x86-64)



Unversioned so-files
--------------------
netty-tcnative: /usr/lib64/netty-tcnative/libnetty-tcnative.so
OK.

Source checksums
----------------
https://github.com/netty/netty-tcnative/archive/netty-tcnative-1.1.30.Fork2.tar.gz :
  CHECKSUM(SHA256) this package     : ef30d9b3c24704c960b3db59f50b8df01db0eec097e1552b2b9d8e558b9bfd6b
  CHECKSUM(SHA256) upstream package : ef30d9b3c24704c960b3db59f50b8df01db0eec097e1552b2b9d8e558b9bfd6b


Generated by fedora-review 0.5.2 (63c24cb) last change: 2014-07-14
Command line :/usr/bin/fedora-review -b 1187713
Buildroot used: fedora-rawhide-x86_64
Active plugins: Generic, Shell-api, Java, C/C++
Disabled plugins: Python, SugarActivity, fonts, Haskell, Ocaml, Perl, R, PHP, Ruby
Disabled flags: EXARCH, EPEL5, BATCH, DISTTAG

Comment 17 jiri vanek 2015-02-09 09:08:34 UTC
(In reply to Zbigniew Jędrzejewski-Szmek from comment #16)
> The patch to load libraries is wrong. It makes the jar file architecture
> dependent,
yes, intentionally

> but it should not be. Just try loading from /usr/lib64, and then from
> /usr/lib.
> This will work on both 64 and 32 bit archs.

Well -  this patch is fedora only, and fedora do not support multilib jni packages (http://fedoraproject.org/wiki/Packaging:Java#Packaging_JAR_files_that_use_JNI - uttermost end - ". Java packages using JNI do not support multiarch installation."

Although I see your point, I would rather stay with my approach - to fail immediately if wrong arch is found.

If you disagree, and insists, I will follow your opinion.


> 
> ===== MUST items =====
> 
> [!]: License file installed when any subpackage combination is installed.
> No license is installed with -javadocs.

Eh... No license file does exists in whole project.

> 
> [!]: Package requires other packages for directories it uses.
>      Note: No known owner of /usr/share/maven-poms/netty-tcnative, /usr/lib64
>      /netty-tcnative, /usr/lib/java/netty-tcnative
> [!]: Package must own all directories that it creates.
>      Note: Directories without known owners: /usr/share/maven-poms/netty-
>      tcnative, /usr/lib64/netty-tcnative, /usr/lib/java/netty-tcnative
> Those should be added.

fixed  (sorry, I thought mvn_install is handling those three)
> 
...snip...
> file
>      from upstream, the packager SHOULD query upstream to include it.
> [!]: Final provides and requires are sane (see attachments).
> See comments below
> 
> [x]: Fully versioned dependency in subpackages if applicable.
>      Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in netty-
>      tcnative-javadoc
> Not needed.
> 
> [?]: Package functions as described.

The testclass runs fine on both x86_64 and i386

> [x]: Latest version is packaged.
> [x]: Package does not include license text files separate from upstream.
> [x]: Patches link to upstream bugs/comments/lists or are otherwise justified.
> [-]: Description and summary sections in the package spec file contains
>      translations for supported Non-English languages, if available.
> [?]: Package should compile and build into binary rpms on all supported
>      architectures.
I have not tested arms...

> I don't think it would actually build, because of the binary conflict.

What do you mean?/Wy do you think so?

> 
...
> 
> 
> Rpmlint
> -------
> Checking: netty-tcnative-1.1.30-0.fc22.x86_64.rpm
>           netty-tcnative-javadoc-1.1.30-0.fc22.x86_64.rpm
>           netty-tcnative-1.1.30-0.fc22.src.rpm
> netty-tcnative.x86_64: W: name-repeated-in-summary C Netty-tcnative
> netty-tcnative.x86_64: W: spelling-error %description -l en_US mavenization
> -> magnetization, humanization, maximization
> netty-tcnative.x86_64: W: incoherent-version-in-changelog 1.1.30.Fork2.0
> ['1.1.30-0.fc22', '1.1.30-0']
> ???
> 
humph. No Named tags allowed? Oook. followed.

> netty-tcnative.x86_64: W: no-documentation
> netty-tcnative.src: W: name-repeated-in-summary C Netty-tcnative
> rpmlint is right here. The summary is meaningless.
> 
fixed

> netty-tcnative.src: W: spelling-error %description -l en_US mavenization ->
> magnetization, humanization, maximization
> netty-tcnative.src: W: strange-permission netty-tcnative-1.1.30.Fork2.tar.gz
> 0640L
> netty-tcnative.src:68: W: macro-in-comment %{_jnidir}
> netty-tcnative.src:68: W: macro-in-comment %{name}
> netty-tcnative.src:68: W: macro-in-comment %{name}
> Please fix those.

done
> 
> netty-tcnative.src:62: W: mixed-use-of-spaces-and-tabs (spaces: line 4, tab:
> line 62)
> And this.
also fixed.
> 
> netty-tcnative.src: W: patch-not-applied Patch1: fixLibNames.patch.in
> 3 packages and 0 specfiles checked; 0 errors, 12 warnings.
> OK.
> 
> 
> Rpmlint (installed packages)
> ----------------------------
> Cannot parse rpmlint output:
> 
> 
> Requires
> --------
> netty-tcnative (rpmlib, GLIBC filtered):
>     apr
>     java-headless
>     jpackage-utils
>     libapr-1.so.0()(64bit)
>     libc.so.6()(64bit)
>     libcrypto.so.10()(64bit)
>     libcrypto.so.10(OPENSSL_1.0.1)(64bit)
>     libcrypto.so.10(OPENSSL_1.0.1_EC)(64bit)
>     libcrypto.so.10(libcrypto.so.10)(64bit)
>     libdl.so.2()(64bit)
>     libpthread.so.0()(64bit)
>     libssl.so.10()(64bit)
>     libssl.so.10(libssl.so.10)(64bit)
>     openssl
> This also seems unecessary. IIUC, only ssl libs are required, and that
> dependency is provided automatically.
Ok, I have removed (how had you found it?)
Requires:  apr
Requires:  openssl
But added
Requires:  java-headless
Requires:  jpackage-utils
(http://fedoraproject.org/wiki/Packaging:Java#BuildRequires_and_Requires ?) 
> 
>     rtld(GNU_HASH)
> 
> netty-tcnative-javadoc (rpmlib, GLIBC filtered):
>     jpackage-utils
> This seems unnecessary.
This seems to be autoadded

> 
> 
> Provides
> --------
> netty-tcnative:
>     libnetty-tcnative-1.1.30.Fork2.so()(64bit)
>     mvn(io.netty:netty-tcnative)
>     mvn(io.netty:netty-tcnative:pom:)
>     netty-tcnative
>     netty-tcnative(x86-64)
>     osgi(io.netty.tcnative)
> 
> netty-tcnative-javadoc:
>     netty-tcnative-javadoc
>     netty-tcnative-javadoc(x86-64)
> 

One more patch added - build on i386 was failing...

Directories updated (note, fixLibNames.patch.in kept as it was)


Thank you!


spec: https://jvanek.fedorapeople.org/elasticsearch/v4/netty-tcnative/netty-tcnative.spec

srpm: https://jvanek.fedorapeople.org/elasticsearch/v4/netty-tcnative/netty-tcnative-1.1.30-0.fc21.src.rpm

kept patch: https://jvanek.fedorapeople.org/elasticsearch/v4/netty-tcnative/fixLibNames.patch.in

new patch: https://jvanek.fedorapeople.org/elasticsearch/v4/netty-tcnative/i388aprFix.patch (applied always, I'm not sure how arm32/64 will need it right now)

Comment 18 Zbigniew Jędrzejewski-Szmek 2015-02-09 15:47:39 UTC
(In reply to jiri vanek from comment #17)
> (In reply to Zbigniew Jędrzejewski-Szmek from comment #16)
> > The patch to load libraries is wrong. It makes the jar file architecture
> > dependent,
> yes, intentionally
> 
> > but it should not be. Just try loading from /usr/lib64, and then from
> > /usr/lib.
> > This will work on both 64 and 32 bit archs.
> 
> Well -  this patch is fedora only, and fedora do not support multilib jni
> packages
> (http://fedoraproject.org/wiki/Packaging:
> Java#Packaging_JAR_files_that_use_JNI - uttermost end - ". Java packages
> using JNI do not support multiarch installation."
> 
> Although I see your point, I would rather stay with my approach - to fail
> immediately if wrong arch is found.
> 
> If you disagree, and insists, I will follow your opinion.
I do disagree. It generates an unnecessary conflict (which is detected
only during transaction check, which is nasty for users):

$ sudo dnf install results/netty-tcnative-1.1.30-0.fc22.x86_64.rpm
...
Error: Transaction check error:
  file /usr/lib/java/netty-tcnative/netty-tcnative.jar from install of netty-tcnative-1.1.30-0.fc22.x86_64 conflicts with file from package netty-tcnative-1.1.30-0.fc22.i686
  file /usr/share/maven-metadata/netty-tcnative.xml from install of netty-tcnative-1.1.30-0.fc22.x86_64 conflicts with file from package netty-tcnative-1.1.30-0.fc22.i686

> > [!]: License file installed when any subpackage combination is installed.
> > No license is installed with -javadocs.
> 
> Eh... No license file does exists in whole project.
Ah, OK.
 
> > 
> > [!]: Package requires other packages for directories it uses.
> >      Note: No known owner of /usr/share/maven-poms/netty-tcnative, /usr/lib64
> >      /netty-tcnative, /usr/lib/java/netty-tcnative
> > [!]: Package must own all directories that it creates.
> >      Note: Directories without known owners: /usr/share/maven-poms/netty-
> >      tcnative, /usr/lib64/netty-tcnative, /usr/lib/java/netty-tcnative
> > Those should be added.
> 
> fixed  (sorry, I thought mvn_install is handling those three)
> > 
> ...snip...
> > file
> >      from upstream, the packager SHOULD query upstream to include it.
> > [!]: Final provides and requires are sane (see attachments).
> > See comments below
> > 
> > [x]: Fully versioned dependency in subpackages if applicable.
> >      Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in netty-
> >      tcnative-javadoc
> > Not needed.
> > 
> > [?]: Package functions as described.
> 
> The testclass runs fine on both x86_64 and i386
I wasn't trying to say that the package does not work, but only that I didn't test it :)
 
> > [x]: Latest version is packaged.
> > [x]: Package does not include license text files separate from upstream.
> > [x]: Patches link to upstream bugs/comments/lists or are otherwise justified.
> > [-]: Description and summary sections in the package spec file contains
> >      translations for supported Non-English languages, if available.
> > [?]: Package should compile and build into binary rpms on all supported
> >      architectures.
> I have not tested arms...
> 
> > I don't think it would actually build, because of the binary conflict.
> 
> What do you mean?/Wy do you think so?
I thought that it would detect the conflict between files. Apparently it does
not, the check is only done for noarch packages. So the build is OK.

> > Requires
> > --------
> > netty-tcnative (rpmlib, GLIBC filtered):
> >     apr
> >     java-headless
> >     jpackage-utils
> >     libapr-1.so.0()(64bit)
> >     libc.so.6()(64bit)
> >     libcrypto.so.10()(64bit)
> >     libcrypto.so.10(OPENSSL_1.0.1)(64bit)
> >     libcrypto.so.10(OPENSSL_1.0.1_EC)(64bit)
> >     libcrypto.so.10(libcrypto.so.10)(64bit)
> >     libdl.so.2()(64bit)
> >     libpthread.so.0()(64bit)
> >     libssl.so.10()(64bit)
> >     libssl.so.10(libssl.so.10)(64bit)
> >     openssl
> > This also seems unecessary. IIUC, only ssl libs are required, and that
> > dependency is provided automatically.
> Ok, I have removed (how had you found it?)
It links to the library, but does not seems to call any binaries directly.

> Requires:  apr
> Requires:  openssl
> But added
> Requires:  java-headless
> Requires:  jpackage-utils
> (http://fedoraproject.org/wiki/Packaging:Java#BuildRequires_and_Requires ?) 
I think they are added automatically because of the BR:maven-local,
but adding them explicitly does not hurt.

> > 
> >     rtld(GNU_HASH)
> > 
> > netty-tcnative-javadoc (rpmlib, GLIBC filtered):
> >     jpackage-utils
> > This seems unnecessary.
> This seems to be autoadded

-javadoc should be noarch! I think it might go away when the
package is noarch.

Comment 19 jiri vanek 2015-02-10 13:05:13 UTC
(In reply to Zbigniew Jędrzejewski-Szmek from comment #18)
> (In reply to jiri vanek from comment #17)
> > (In reply to Zbigniew Jędrzejewski-Szmek from comment #16)
> > > The patch to load libraries is wrong. It makes the jar file architecture
> > > dependent,
> > yes, intentionally
> > 
> > > but it should not be. Just try loading from /usr/lib64, and then from
> > > /usr/lib.
> > > This will work on both 64 and 32 bit archs.
> > 
> > Well -  this patch is fedora only, and fedora do not support multilib jni
> > packages
> > (http://fedoraproject.org/wiki/Packaging:
> > Java#Packaging_JAR_files_that_use_JNI - uttermost end - ". Java packages
> > using JNI do not support multiarch installation."
> > 
> > Although I see your point, I would rather stay with my approach - to fail
> > immediately if wrong arch is found.
> > 
> > If you disagree, and insists, I will follow your opinion.
> I do disagree. It generates an unnecessary conflict (which is detected
> only during transaction check, which is nasty for users):
> 
> $ sudo dnf install results/netty-tcnative-1.1.30-0.fc22.x86_64.rpm
> ...
> Error: Transaction check error:
>   file /usr/lib/java/netty-tcnative/netty-tcnative.jar from install of
> netty-tcnative-1.1.30-0.fc22.x86_64 conflicts with file from package
> netty-tcnative-1.1.30-0.fc22.i686
>   file /usr/share/maven-metadata/netty-tcnative.xml from install of
> netty-tcnative-1.1.30-0.fc22.x86_64 conflicts with file from package
> netty-tcnative-1.1.30-0.fc22.i686
> 
...snip...
> > > This seems unnecessary.
> > This seems to be autoadded
> 
> -javadoc should be noarch! I think it might go away when the
> package is noarch.


javadoc made noarch.
spec: https://jvanek.fedorapeople.org/elasticsearch/v4/netty-tcnative/netty-tcnative.spec
srpm: https://jvanek.fedorapeople.org/elasticsearch/v4/netty-tcnative/netty-tcnative-1.1.30-0.fc21.src.rpm

Last thing we disagree is the arch/noarch lib/path/lib64 convention.

I'm still keeping my approach, beacuse I somehow feel that what you wont me to do is wrong:

- from fedora guidelines the multilib jni packages are not supported
- my main package contains both jar and nativelib => not noarch

if I split natvie and java files to two packages and made jar "multilib" (search in both lib and lib64) then yes, your case you shown with dnf will work. However,
- the multilib is not supposed to be supported
- there is real danger that jar run in 64 jvm will load 32b library or vice versa (afaik it is why jni is not supported multilib)
- the jar without so and vice versa dont have sense.

Thank you for patience!

Comment 20 Zbigniew Jędrzejewski-Szmek 2015-02-11 15:47:45 UTC
(In reply to jiri vanek from comment #19)
> Last thing we disagree is the arch/noarch lib/path/lib64 convention.
> 
> I'm still keeping my approach, beacuse I somehow feel that what you wont me
> to do is wrong:
> 
> - from fedora guidelines the multilib jni packages are not supported
> - my main package contains both jar and nativelib => not noarch
> 
> if I split natvie and java files to two packages and made jar "multilib"
> (search in both lib and lib64) then yes, your case you shown with dnf will
> work. However,
> - the multilib is not supposed to be supported
> - there is real danger that jar run in 64 jvm will load 32b library or vice
> versa (afaik it is why jni is not supported multilib)
> - the jar without so and vice versa dont have sense.
OK. This feels to be a suboptimal situation to me, but I don't see an easy
solution. I was wrong in thinking that the packages for the two architectures
can be installed simultaneously when the patch to use .load() is modified to
not include an architecture-specific path. It turns out that the jar contains
headers which include a timestamp, so the resulting jar files are different
anyway. A work-around could be added, but it's not trivial.

I believe all issues have been fixed or determined to be unfixable.
Package is APPROVED.

Comment 21 jiri vanek 2015-02-11 18:06:25 UTC
Thank you!

Comment 22 jiri vanek 2015-02-11 18:27:02 UTC
New Package SCM Request
=======================
Package Name: netty-tcnative
Short Description: Fork of Tomcat Native with improved OpenSSL and mavenized build
Upstream URL:ttps://github.com/netty/netty/wiki/Forked-Tomcat-Native
Owners: jvanek
Branches: f22
InitialCC: mgoldman

Comment 23 jiri vanek 2015-02-11 18:27:33 UTC
New Package SCM Request
=======================
Package Name: netty-tcnative
Short Description: Fork of Tomcat Native with improved OpenSSL and mavenized build
Upstream URL: https://github.com/netty/netty/wiki/Forked-Tomcat-Native
Owners: jvanek
Branches: f22
InitialCC: mgoldman

Comment 24 Gwyn Ciesla 2015-02-11 19:24:40 UTC
Git done (by process-git-requests).

Comment 25 Pierre-YvesChibon 2015-02-12 08:31:38 UTC
The FAS user mgoldman does not exist!

I'm fixing the problem on pkgdb itself.

Comment 26 jiri vanek 2015-02-12 11:37:03 UTC
Pushed and build. Thank you all!

Comment 27 Zbigniew Jędrzejewski-Szmek 2015-02-18 15:40:43 UTC
In light of https://bugzilla.redhat.com/show_bug.cgi?id=902086#c74, please remove explicit Requires on jpackage-utils and java-headless.

Comment 28 jiri vanek 2015-02-18 17:56:37 UTC
done.


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