Bug 1732894 - Review Request: jgit - Eclipse JGit
Summary: Review Request: jgit - Eclipse JGit
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Fabio Valentini
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2019-07-24 15:58 UTC by Mat Booth
Modified: 2019-07-25 14:45 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2019-07-25 14:45:46 UTC
decathorpe: fedora-review+


Attachments (Terms of Use)

Description Mat Booth 2019-07-24 15:58:53 UTC
Spec URL: https://fedorapeople.org/~mbooth/reviews/jgit.spec
SRPM URL: https://fedorapeople.org/~mbooth/reviews/jgit-5.4.0-3.fc30.src.rpm
Description:
A pure Java implementation of the Git version control system and command
line interface.

Fedora Account System Username: mbooth

Comment 1 Fabio Valentini 2019-07-24 19:21:50 UTC
I can look at this package in a few hours.

Splitting jgit into its own package was also a solution I thought about. :)

Comment 2 Miro Hrončok 2019-07-24 22:47:29 UTC
Here's an automatically pre-filed Fedora-Review template:

Generic:
[ ]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[ ]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "Unknown or generated", "*No copyright* BSD 3-clause "New" or
     "Revised" License", "BSD 3-clause "New" or "Revised" License",
     "Eclipse Public License (v1.0)", "*No copyright* Eclipse Public
     License (v2.0)", "Eclipse Public License". 639 files have unknown
     license. Detailed output of licensecheck in
     /home/churchyard/rpmbuild/FedoraReview/1732894-jgit/licensecheck.txt
[ ]: License file installed when any subpackage combination is installed.
[ ]: Package must own all directories that it creates.
     Note: Directories without known owners: /etc/ant.d
[ ]: Package contains no bundled libraries without FPC exception.
[ ]: Changelog in prescribed format.
[ ]: 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
[ ]: Package uses nothing in %doc for runtime.
[ ]: Package consistently uses macros (instead of hard-coded directory
     names).
[ ]: Package is named according to the Package Naming Guidelines.
[ ]: Package does not generate any conflict.
[ ]: Package obeys FHS, except libexecdir and /usr/target.
[ ]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[ ]: Requires correct, justified where necessary.
[ ]: Spec file is legible and written in American English.
[ ]: Package contains systemd file(s) if in need.
[ ]: Package is not known to require an ExcludeArch tag.
[ ]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 10240 bytes in 1 files.
[ ]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: Package requires other packages for directories it uses.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: %config files are marked noreplace or the reason is justified.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package must not depend on deprecated() packages.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: No %config files under /usr.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

Java:
[x]: Bundled jar/class files should be removed before build

===== 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).
[ ]: Package functions as described.
[ ]: Latest version is packaged.
[ ]: Package does not include license text files separate from upstream.
[ ]: 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.
[ ]: %check is present and all tests pass.
[ ]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Fully versioned dependency in subpackages if applicable.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

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

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: jgit-5.4.0-3.fc31.noarch.rpm
          jgit-javadoc-5.4.0-3.fc31.noarch.rpm
          jgit-5.4.0-3.fc31.src.rpm
jgit.noarch: E: explicit-lib-dependency jzlib
jgit.noarch: W: name-repeated-in-summary C JGit
jgit.noarch: W: no-manual-page-for-binary jgit
jgit.src: W: name-repeated-in-summary C JGit
3 packages and 0 specfiles checked; 1 errors, 3 warnings.




Rpmlint (installed packages)
----------------------------
jgit.noarch: E: explicit-lib-dependency jzlib
jgit.noarch: W: name-repeated-in-summary C JGit
jgit.noarch: W: invalid-url URL: https://www.eclipse.org/jgit/ <urlopen error [Errno -2] Name or service not known>
jgit.noarch: W: no-manual-page-for-binary jgit
jgit-javadoc.noarch: W: invalid-url URL: https://www.eclipse.org/jgit/ <urlopen error [Errno -2] Name or service not known>
2 packages and 0 specfiles checked; 1 errors, 4 warnings.



Source checksums
----------------
https://git.eclipse.org/c/jgit/jgit.git/snapshot/jgit-5.4.0.201906121030-r.tar.xz :
  CHECKSUM(SHA256) this package     : 8a3cb11479c6e47bf8fe49c09608c5be18c5da40d6d9dd08778dcbbdce104672
  CHECKSUM(SHA256) upstream package : 8a3cb11479c6e47bf8fe49c09608c5be18c5da40d6d9dd08778dcbbdce104672


Requires
--------
jgit (rpmlib, GLIBC filtered):
    /usr/bin/sh
    apache-sshd
    bouncycastle
    config(jgit)
    java-headless
    javapackages-filesystem
    jzlib
    mvn(args4j:args4j)
    mvn(com.google.code.gson:gson)
    mvn(com.googlecode.javaewah:JavaEWAH)
    mvn(com.jcraft:jsch)
    mvn(com.jcraft:jzlib)
    mvn(net.i2p.crypto:eddsa)
    mvn(org.apache.ant:ant)
    mvn(org.apache.commons:commons-compress)
    mvn(org.apache.httpcomponents:httpclient)
    mvn(org.apache.httpcomponents:httpcore)
    mvn(org.apache.sshd:sshd-osgi)
    mvn(org.apache.sshd:sshd-sftp)
    mvn(org.bouncycastle:bcpg-jdk15on)
    mvn(org.bouncycastle:bcpkix-jdk15on)
    mvn(org.bouncycastle:bcprov-jdk15on)
    mvn(org.eclipse.jetty:jetty-servlet)
    mvn(org.osgi:osgi.core)
    mvn(org.slf4j:slf4j-api)
    mvn(org.slf4j:slf4j-simple)

jgit-javadoc (rpmlib, GLIBC filtered):
    javapackages-filesystem



Provides
--------
jgit:
    config(jgit)
    jgit
    mvn(org.eclipse.jgit:org.eclipse.jgit)
    mvn(org.eclipse.jgit:org.eclipse.jgit-parent:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.ant)
    mvn(org.eclipse.jgit:org.eclipse.jgit.ant:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.archive)
    mvn(org.eclipse.jgit:org.eclipse.jgit.archive:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.http.apache)
    mvn(org.eclipse.jgit:org.eclipse.jgit.http.apache:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.http.server)
    mvn(org.eclipse.jgit:org.eclipse.jgit.http.server:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.junit)
    mvn(org.eclipse.jgit:org.eclipse.jgit.junit.http)
    mvn(org.eclipse.jgit:org.eclipse.jgit.junit.http:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.junit.ssh)
    mvn(org.eclipse.jgit:org.eclipse.jgit.junit.ssh:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.junit:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.lfs)
    mvn(org.eclipse.jgit:org.eclipse.jgit.lfs.server)
    mvn(org.eclipse.jgit:org.eclipse.jgit.lfs.server:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.lfs:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.pgm)
    mvn(org.eclipse.jgit:org.eclipse.jgit.pgm:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.ssh.apache)
    mvn(org.eclipse.jgit:org.eclipse.jgit.ssh.apache:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.ui)
    mvn(org.eclipse.jgit:org.eclipse.jgit.ui:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit:pom:)
    osgi(org.eclipse.jgit)
    osgi(org.eclipse.jgit.ant)
    osgi(org.eclipse.jgit.archive)
    osgi(org.eclipse.jgit.http.apache)
    osgi(org.eclipse.jgit.http.server)
    osgi(org.eclipse.jgit.junit)
    osgi(org.eclipse.jgit.junit.http)
    osgi(org.eclipse.jgit.junit.ssh)
    osgi(org.eclipse.jgit.lfs)
    osgi(org.eclipse.jgit.lfs.server)
    osgi(org.eclipse.jgit.pgm)
    osgi(org.eclipse.jgit.ssh.apache)
    osgi(org.eclipse.jgit.ui)

jgit-javadoc:
    jgit-javadoc

Comment 3 Miro Hrončok 2019-07-24 22:48:03 UTC
An interesting part of licensecheck.txt:

*No copyright* Eclipse Public License (v2.0)
--------------------------------------------
jgit-5.4.0.201906121030-r/org.eclipse.jgit.packaging/org.eclipse.jgit.feature/license.html
jgit-5.4.0.201906121030-r/org.eclipse.jgit.packaging/org.eclipse.jgit.http.apache.feature/license.html
jgit-5.4.0.201906121030-r/org.eclipse.jgit.packaging/org.eclipse.jgit.junit.feature/license.html
jgit-5.4.0.201906121030-r/org.eclipse.jgit.packaging/org.eclipse.jgit.lfs.feature/license.html
jgit-5.4.0.201906121030-r/org.eclipse.jgit.packaging/org.eclipse.jgit.pgm.feature/license.html
jgit-5.4.0.201906121030-r/org.eclipse.jgit.packaging/org.eclipse.jgit.source.feature/license.html
jgit-5.4.0.201906121030-r/org.eclipse.jgit.packaging/org.eclipse.jgit.ssh.apache.feature/license.html

Comment 4 Fabio Valentini 2019-07-24 22:55:42 UTC
I don't know why, but I can't even build this package on rawhide (because apache-sshd apparently is missing from fedora-rawhide-x86_64 repos here :/ )

No matching package to install: 'mvn(org.apache.sshd:sshd-osgi) >= 2.2.0'
No matching package to install: 'mvn(org.apache.sshd:sshd-sftp) >= 2.2.0'

Comment 5 Miro Hrončok 2019-07-24 22:58:09 UTC
Use:

    mock -r fedora-rawhide-x86_64 --enablerepo=local jgit-5.4.0-3.fc30.src.rpm

Or:

    fedora-review -b 1732894 -m fedora-rawhide-x86_64 --mock-options '\--enablerepo=local'

Comment 6 Fabio Valentini 2019-07-24 23:01:01 UTC
Ah, I see. While the commit to update it to version 2.2.0 is old, the non-modular builds were only done yesterday. I got a bit confused by the three weeks old commit.

Comment 7 Nicolas De Amicis 2019-07-25 08:38:49 UTC
I ran fedora-review -b 1732894 -m fedora-rawhide-x86_64 --mock-options '\--enablerepo=local' and I review the output.
It seems ok to me

Comment 8 Miro Hrončok 2019-07-25 09:28:13 UTC
Nicolas, thanks for you input, but:

1) this bugzilla is assigned to Fabio

2) this is not how one does a review "I've seen something and it looks good" - please read https://docs.fedoraproject.org/en-US/packaging-guidelines/ReviewGuidelines/

Comment 9 Nicolas De Amicis 2019-07-25 10:10:00 UTC
I apologize for my beginner mistakes.
1) I received an email from Mat to review this package. I know that I'm not the good reviewer.
2) I read https://fedoraproject.org/wiki/Package_Review_Process but I will read the documentation carefully

"The only man who never makes mistakes is the man who never does anything." Theodore Roosevelt

Comment 10 Miro Hrončok 2019-07-25 10:25:01 UTC
Don't worry about it, mistakes is where one learns. Most importantly -> anything you observe and consider OK, you should document here.

Good example:

"I've run rpmlint and this is the output:

jgit.noarch: E: explicit-lib-dependency jzlib
jgit.noarch: W: name-repeated-in-summary C JGit
jgit.noarch: W: no-manual-page-for-binary jgit
jgit.src: W: name-repeated-in-summary C JGit
3 packages and 0 specfiles checked; 1 errors, 3 warnings.

I consider all of the errors and warnings irrelevant, because ..."


---------

Bad example:

"I've seen the rpmlint output and it is OK"

Comment 11 Mat Booth 2019-07-25 10:45:37 UTC
(In reply to Nicolas De Amicis from comment #9)
> I apologize for my beginner mistakes.
> 1) I received an email from Mat to review this package. I know that I'm not
> the good reviewer.
> 2) I read https://fedoraproject.org/wiki/Package_Review_Process but I will
> read the documentation carefully
> 
> "The only man who never makes mistakes is the man who never does anything."
> Theodore Roosevelt

Thanks for taking a look anyway

I later also notified Fabio (decathorpe) about this review since he is also interesting in the gradle problem. It looks like he got there first and assigned it to himself -- but additional reviews are always welcome; more eyes is more better :-)

Comment 12 Mat Booth 2019-07-25 10:50:49 UTC
(In reply to Miro Hrončok from comment #3)
> An interesting part of licensecheck.txt:
> 
> *No copyright* Eclipse Public License (v2.0)
> --------------------------------------------
> jgit-5.4.0.201906121030-r/org.eclipse.jgit.packaging/org.eclipse.jgit.
> feature/license.html
> jgit-5.4.0.201906121030-r/org.eclipse.jgit.packaging/org.eclipse.jgit.http.
> apache.feature/license.html
> jgit-5.4.0.201906121030-r/org.eclipse.jgit.packaging/org.eclipse.jgit.junit.
> feature/license.html
> jgit-5.4.0.201906121030-r/org.eclipse.jgit.packaging/org.eclipse.jgit.lfs.
> feature/license.html
> jgit-5.4.0.201906121030-r/org.eclipse.jgit.packaging/org.eclipse.jgit.pgm.
> feature/license.html
> jgit-5.4.0.201906121030-r/org.eclipse.jgit.packaging/org.eclipse.jgit.source.
> feature/license.html
> jgit-5.4.0.201906121030-r/org.eclipse.jgit.packaging/org.eclipse.jgit.ssh.
> apache.feature/license.html

This project is split into two parts, the jgit core and the Eclipse features. The Eclipse features are apparently EPL licenced, but the jgit core itself is BSD licensed

This package only builds and ships the BSD licensed portion (jgit lib and command line tool) and the EPL licensed Eclipse features will remain in the existing "eclipse-jgit" package.

Comment 13 Mat Booth 2019-07-25 10:56:49 UTC
(In reply to Miro Hrončok from comment #10)
> jgit.noarch: E: explicit-lib-dependency jzlib

This error is a false positive is because jzlib is an unfortunately named pure java library (it is java re-implementation of zlib) and rpmlint always incorrectly flags it as a native lib.

The reason I have this explicit dep is because it jgit "optionally" supports transport compression, but in reality you nearly always need it so better to just mandate this very tiny dep to avoid confusing user experience (principle of least surprise!)

Comment 14 Fabio Valentini 2019-07-25 10:57:47 UTC
A few comments outside the review template:

0. It looks like at least some of the included files are indeed licensed EPL (1.0 / 2.0), not BSD. Please make sure they're not shipped as part of the binary RPM, or adapt License tag accordingly and add a license file to the package with %license.

1. If git is a BR for tests, you could move it under %{with tests} as well.

2. "%package -n jgit-javadoc" -> "%package javadoc" (same for description), I guess this is a leftover from the eclipse-jgit packaging.

3. Do we need to build the source jar? otherwise, we can remove maven-source-plugin from the pom and BRs.

4. Why are failing tests ignored even when the build is run "--with tests"?

5. I think you could truncate the %changelog as well, since this is a new package.

6. Will eclipse-jgit have a versioned dependency on this package, to make sure they're updated at the same time?

PS: Will splitting eclipse-jgit into two packages also solve the bootstrap issue in eclipse-jgit?




Full review below.

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

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


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

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[?]: License field in the package spec file matches the actual license.
[?]: License file installed when any subpackage combination is installed.
[x]: Package must own all directories that it creates.
[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]: 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.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: Package requires other packages for directories it uses.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: %config files are marked noreplace or the reason is justified.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package must not depend on deprecated() packages.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: No %config files under /usr.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

Java:
[x]: Bundled jar/class files should be removed before build

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

Generic:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[-]: Fully versioned dependency in subpackages if applicable.
[?]: 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.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[?]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

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

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: jgit-5.4.0-3.fc31.noarch.rpm
          jgit-javadoc-5.4.0-3.fc31.noarch.rpm
          jgit-5.4.0-3.fc31.src.rpm
jgit.noarch: E: explicit-lib-dependency jzlib
jgit.noarch: W: name-repeated-in-summary C JGit
jgit.noarch: W: no-manual-page-for-binary jgit
jgit.src: W: name-repeated-in-summary C JGit
jgit.src: W: invalid-url Source0: https://git.eclipse.org/c/jgit/jgit.git/snapshot/jgit-5.4.0.201906121030-r.tar.xz The read operation timed out
3 packages and 0 specfiles checked; 1 errors, 4 warnings.




Rpmlint (installed packages)
----------------------------
perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LC_CTYPE = "C.UTF-8",
	LANG = "en_GB.UTF-8"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LC_CTYPE = "C.UTF-8",
	LANG = "en_GB.UTF-8"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
jgit-javadoc.noarch: W: invalid-url URL: https://www.eclipse.org/jgit/ <urlopen error [Errno -2] Name or service not known>
jgit.noarch: E: explicit-lib-dependency jzlib
jgit.noarch: W: name-repeated-in-summary C JGit
jgit.noarch: W: invalid-url URL: https://www.eclipse.org/jgit/ <urlopen error [Errno -2] Name or service not known>
jgit.noarch: W: no-manual-page-for-binary jgit
2 packages and 0 specfiles checked; 1 errors, 4 warnings.



Source checksums
----------------
https://git.eclipse.org/c/jgit/jgit.git/snapshot/jgit-5.4.0.201906121030-r.tar.xz :
  CHECKSUM(SHA256) this package     : 8a3cb11479c6e47bf8fe49c09608c5be18c5da40d6d9dd08778dcbbdce104672
  CHECKSUM(SHA256) upstream package : 8a3cb11479c6e47bf8fe49c09608c5be18c5da40d6d9dd08778dcbbdce104672


Requires
--------
jgit (rpmlib, GLIBC filtered):
    /usr/bin/sh
    apache-sshd
    bouncycastle
    config(jgit)
    java-headless
    javapackages-filesystem
    jzlib
    mvn(args4j:args4j)
    mvn(com.google.code.gson:gson)
    mvn(com.googlecode.javaewah:JavaEWAH)
    mvn(com.jcraft:jsch)
    mvn(com.jcraft:jzlib)
    mvn(net.i2p.crypto:eddsa)
    mvn(org.apache.ant:ant)
    mvn(org.apache.commons:commons-compress)
    mvn(org.apache.httpcomponents:httpclient)
    mvn(org.apache.httpcomponents:httpcore)
    mvn(org.apache.sshd:sshd-osgi)
    mvn(org.apache.sshd:sshd-sftp)
    mvn(org.bouncycastle:bcpg-jdk15on)
    mvn(org.bouncycastle:bcpkix-jdk15on)
    mvn(org.bouncycastle:bcprov-jdk15on)
    mvn(org.eclipse.jetty:jetty-servlet)
    mvn(org.osgi:osgi.core)
    mvn(org.slf4j:slf4j-api)
    mvn(org.slf4j:slf4j-simple)

jgit-javadoc (rpmlib, GLIBC filtered):
    javapackages-filesystem



Provides
--------
jgit:
    config(jgit)
    jgit
    mvn(org.eclipse.jgit:org.eclipse.jgit)
    mvn(org.eclipse.jgit:org.eclipse.jgit-parent:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.ant)
    mvn(org.eclipse.jgit:org.eclipse.jgit.ant:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.archive)
    mvn(org.eclipse.jgit:org.eclipse.jgit.archive:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.http.apache)
    mvn(org.eclipse.jgit:org.eclipse.jgit.http.apache:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.http.server)
    mvn(org.eclipse.jgit:org.eclipse.jgit.http.server:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.junit)
    mvn(org.eclipse.jgit:org.eclipse.jgit.junit.http)
    mvn(org.eclipse.jgit:org.eclipse.jgit.junit.http:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.junit.ssh)
    mvn(org.eclipse.jgit:org.eclipse.jgit.junit.ssh:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.junit:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.lfs)
    mvn(org.eclipse.jgit:org.eclipse.jgit.lfs.server)
    mvn(org.eclipse.jgit:org.eclipse.jgit.lfs.server:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.lfs:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.pgm)
    mvn(org.eclipse.jgit:org.eclipse.jgit.pgm:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.ssh.apache)
    mvn(org.eclipse.jgit:org.eclipse.jgit.ssh.apache:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit.ui)
    mvn(org.eclipse.jgit:org.eclipse.jgit.ui:pom:)
    mvn(org.eclipse.jgit:org.eclipse.jgit:pom:)
    osgi(org.eclipse.jgit)
    osgi(org.eclipse.jgit.ant)
    osgi(org.eclipse.jgit.archive)
    osgi(org.eclipse.jgit.http.apache)
    osgi(org.eclipse.jgit.http.server)
    osgi(org.eclipse.jgit.junit)
    osgi(org.eclipse.jgit.junit.http)
    osgi(org.eclipse.jgit.junit.ssh)
    osgi(org.eclipse.jgit.lfs)
    osgi(org.eclipse.jgit.lfs.server)
    osgi(org.eclipse.jgit.pgm)
    osgi(org.eclipse.jgit.ssh.apache)
    osgi(org.eclipse.jgit.ui)

jgit-javadoc:
    jgit-javadoc

Comment 15 Fabio Valentini 2019-07-25 10:59:05 UTC
(I see the licensing has been cleared up in a comment that was posted while I had this page open to write my review, so disregard that.)

Comment 16 Mat Booth 2019-07-25 11:17:16 UTC
(In reply to Fabio Valentini from comment #14)
> A few comments outside the review template:
> 
> 0. It looks like at least some of the included files are indeed licensed EPL
> (1.0 / 2.0), not BSD. Please make sure they're not shipped as part of the
> binary RPM, or adapt License tag accordingly and add a license file to the
> package with %license.

I will a comment to the spec file explaining it too :-)

> 
> 1. If git is a BR for tests, you could move it under %{with tests} as well.

Yes, good point

> 
> 2. "%package -n jgit-javadoc" -> "%package javadoc" (same for description),
> I guess this is a leftover from the eclipse-jgit packaging.

Yes, good guess :-)

> 
> 3. Do we need to build the source jar? otherwise, we can remove
> maven-source-plugin from the pom and BRs.
> 

True -- I'm pretty sure the existing eclipse-jgit package does not ship the source for the eclipse plugin part, so it should be okay to do this.


> 4. Why are failing tests ignored even when the build is run "--with tests"?
> 

Good question! There used to be an intermittant failure, but I don't recall the details and it may be fixed in recent versions. Let's try not ignoring it :-)

> 5. I think you could truncate the %changelog as well, since this is a new
> package.
> 

Sure, happy to do that.

> 6. Will eclipse-jgit have a versioned dependency on this package, to make
> sure they're updated at the same time?

Yes. FWIW the eclipse-egit package also has a versioned dependency on eclipse-jgit since this is all very tightly coupled.

> 
> PS: Will splitting eclipse-jgit into two packages also solve the bootstrap
> issue in eclipse-jgit?
> 
> 

Yes, this is bonus benefit for me since I will no longer have to deal with the split-phase build process :-) The maven pom-centric parts are moved to this package and the OSGi manifest-centric parts remain in eclipse-jgit

> 
> 
> Full review below.
> 

Thanks for the review, I will upload a revised spec shortly!

Comment 17 Mat Booth 2019-07-25 11:23:38 UTC
(In reply to Mat Booth from comment #13)
> (In reply to Miro Hrončok from comment #10)
> > jgit.noarch: E: explicit-lib-dependency jzlib
> 
> This error is a false positive is because jzlib is an unfortunately named
> pure java library (it is java re-implementation of zlib) and rpmlint always
> incorrectly flags it as a native lib.
> 
> The reason I have this explicit dep is because it jgit "optionally" supports
> transport compression, but in reality you nearly always need it so better to
> just mandate this very tiny dep to avoid confusing user experience
> (principle of least surprise!)

Oh! Forget this comment -- it looks like upstream made this dep non-optional so I don't need to specify it explicitly any longer -- I will make that change too.

Comment 18 Mat Booth 2019-07-25 11:35:02 UTC
Please consider my updated package:

Spec URL: https://fedorapeople.org/~mbooth/reviews/jgit.spec
SRPM URL: https://fedorapeople.org/~mbooth/reviews/jgit-5.4.0-4.fc30.src.rpm

Note about tests: Usually when past me has set the ignore failures flag it is because the tests are racy on arches where Java is quite slow like 32bit arm or s390x. I will keep an eye on it in koji, but if we start seeing intermittent failures again, I will probably restore this flag (and make better notes for future me this time :-)

Comment 19 Fabio Valentini 2019-07-25 11:49:24 UTC
I think you addressed all my comments.

But I think you might really want to ignore test failures again.
The .src.rpm built fine in local mock, but koji scratch builds failed miserably, for example

- x86_64: https://koji.fedoraproject.org/koji/taskinfo?taskID=36507521
- i686: https://koji.fedoraproject.org/koji/taskinfo?taskID=36507510
- ppc64le: https://koji.fedoraproject.org/koji/taskinfo?taskID=36507531
- aarch64: https://koji.fedoraproject.org/koji/taskinfo?taskID=36507532

armv7hl and s390x are still running but having the "fast" arches all fail doesn't bode well for the other two.

Comment 20 Mat Booth 2019-07-25 12:41:02 UTC
(In reply to Fabio Valentini from comment #19)
> I think you addressed all my comments.
> 
> But I think you might really want to ignore test failures again.
> The .src.rpm built fine in local mock, but koji scratch builds failed
> miserably, for example
> 
> - x86_64: https://koji.fedoraproject.org/koji/taskinfo?taskID=36507521
> - i686: https://koji.fedoraproject.org/koji/taskinfo?taskID=36507510
> - ppc64le: https://koji.fedoraproject.org/koji/taskinfo?taskID=36507531
> - aarch64: https://koji.fedoraproject.org/koji/taskinfo?taskID=36507532
> 
> armv7hl and s390x are still running but having the "fast" arches all fail
> doesn't bode well for the other two.

Oh wow, there must be something about the limitations of the koji build env.... "Mount point not found" ... "Mount point not found" ... "Mount point not found" ...

Back to ignoring failures for koji builds:

Spec URL: https://fedorapeople.org/~mbooth/reviews/jgit.spec
SRPM URL: https://fedorapeople.org/~mbooth/reviews/jgit-5.4.0-5.fc30.src.rpm

Comment 21 Fabio Valentini 2019-07-25 12:53:06 UTC
Looks good now.

Comment 22 Mat Booth 2019-07-25 13:06:07 UTC
(In reply to Fabio Valentini from comment #21)
> Looks good now.

Many thanks. Repo requested: https://pagure.io/releng/fedora-scm-requests/issue/13854

Comment 23 Gwyn Ciesla 2019-07-25 13:53:41 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/jgit

Comment 24 Mat Booth 2019-07-25 14:45:46 UTC
I did a all-architecture build of jgit:

* F30: https://koji.fedoraproject.org/koji/buildinfo?buildID=1326340
* F31: https://koji.fedoraproject.org/koji/buildinfo?buildID=1326358


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