Bug 1856557

This is a re-review for unretirement: https://pagure.io/releng/issue/9600

Taking this review.

One blocking issue right away: The latest version seems to be 217.21, the package is for 217.8 right now. Can you update it to the latest version?
https://github.com/jenkinsci/trilead-ssh2/releases

Also, the versioning scheme is very weird here (both upstream and in the package).
Maybe do something like this instead? The "patch level" definitely shouldn't be in the Release tag. I'd say that ".jenkins" shouldn't be part of the Release tag, either, since it has nothing to do with the versioning, but with the origin of the sources.

%global upstream_version 217
%global patch_level 21
Version: %{upstream_version}.%{patch_level}
Release: 1%{?dist}
Source0: https://github.com/jenkinsci/%{name}/archive/%{name}-build%{upstream_version}-jenkins-%{patch_level}.tar.gz
%setup -q -n %{name}-%{name}-build%{upstream_version}-jenkins-%{patch_level}

A comment in the .spec file that the package is built from the Jenkins fork of trilead-ssh2 (and why?) is enough.

(In reply to Fabio Valentini from comment #2)
> Taking this review.
> 
> One blocking issue right away: The latest version seems to be 217.21, the
> package is for 217.8 right now. Can you update it to the latest version?
> https://github.com/jenkinsci/trilead-ssh2/releases
> 

Err... Probably. "8" is the last version before they added deps on stuff not in Fedora

> Also, the versioning scheme is very weird here (both upstream and in the
> package).
> Maybe do something like this instead? The "patch level" definitely shouldn't
> be in the Release tag. I'd say that ".jenkins" shouldn't be part of the
> Release tag, either, since it has nothing to do with the versioning, but
> with the origin of the sources.
> 

Yeah that's fair

> 
> A comment in the .spec file that the package is built from the Jenkins fork
> of trilead-ssh2 (and why?) is enough.

Comment added

See updated spec file:

Spec URL: https://fedorapeople.org/~mbooth/reviews/trilead-ssh2.spec
SRPM URL: https://fedorapeople.org/~mbooth/reviews/trilead-ssh2-217.21-14.fc33.src.rpm

(In reply to Mat Booth from comment #3)
> Err... Probably. "8" is the last version before they added deps on stuff not
> in Fedora

That's probably a good guess ... thanks for updating it.
It would probably be good to unbundle jbcrypt in the long run, but bundling one (!) file for now is fine with me.
Not sure if it's necessary to ship the actual ISC license text with the package, but I don't think the license requires it.

> Yeah that's fair

Since version was effectively bumped from 217 to 217.21, you should also reset Release number to 1 (also in the Changelog message).

> > 
> > A comment in the .spec file that the package is built from the Jenkins fork
> > of trilead-ssh2 (and why?) is enough.
> 
> Comment added

Thanks!

> See updated spec file:
> 
> Spec URL: https://fedorapeople.org/~mbooth/reviews/trilead-ssh2.spec
> SRPM URL:
> https://fedorapeople.org/~mbooth/reviews/trilead-ssh2-217.21-14.fc33.src.rpm


I've ran the fedora-review tool and looked at the .spec file, and everything seems to be in order, except one thing:

One file is recognised by licensecheck to be under an "NTP License":
trilead-ssh2-trilead-ssh2-build-217-jenkins-21/src/com/trilead/ssh2/crypto/digest/MD5.java

The license header in this class looks unlike anything I've seen before, and I don't know how to categorize it.
NTP License is not listed under fedora's "Good" or "Bad" Licenses either ...

(In reply to Fabio Valentini from comment #4)
> (In reply to Mat Booth from comment #3)
> > Err... Probably. "8" is the last version before they added deps on stuff not
> > in Fedora
> 
> That's probably a good guess ... thanks for updating it.
> It would probably be good to unbundle jbcrypt in the long run, but bundling
> one (!) file for now is fine with me.
> Not sure if it's necessary to ship the actual ISC license text with the
> package, but I don't think the license requires it.
> 
> > Yeah that's fair
> 
> Since version was effectively bumped from 217 to 217.21, you should also
> reset Release number to 1 (also in the Changelog message).
> 
> > > 
> > > A comment in the .spec file that the package is built from the Jenkins fork
> > > of trilead-ssh2 (and why?) is enough.
> > 
> > Comment added
> 
> Thanks!
> 
> > See updated spec file:
> > 
> > Spec URL: https://fedorapeople.org/~mbooth/reviews/trilead-ssh2.spec
> > SRPM URL:
> > https://fedorapeople.org/~mbooth/reviews/trilead-ssh2-217.21-14.fc33.src.rpm
> 
> 
> I've ran the fedora-review tool and looked at the .spec file, and everything
> seems to be in order, except one thing:
> 
> One file is recognised by licensecheck to be under an "NTP License":
> trilead-ssh2-trilead-ssh2-build-217-jenkins-21/src/com/trilead/ssh2/crypto/
> digest/MD5.java
> 
> The license header in this class looks unlike anything I've seen before, and
> I don't know how to categorize it.
> NTP License is not listed under fedora's "Good" or "Bad" Licenses either ...

NTP is just MIT: https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/message/XSDPIYGEEIVMTJCKPGAL6UCC6DKJHZGK/

(In reply to Robert-André Mauchin 🐧 from comment #5)
> NTP is just MIT:
> https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/
> message/XSDPIYGEEIVMTJCKPGAL6UCC6DKJHZGK/

Thanks for the link! Reading the message, I think the same applies here (RSA license).
Should RSA be added to the License field?

(In reply to Fabio Valentini from comment #6)
> (In reply to Robert-André Mauchin 🐧 from comment #5)
> > NTP is just MIT:
> > https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/
> > message/XSDPIYGEEIVMTJCKPGAL6UCC6DKJHZGK/
> 
> Thanks for the link! Reading the message, I think the same applies here (RSA
> license).
> Should RSA be added to the License field?

That's what openjdk package did, so I do so here too:

(Now also with fixed release tag)

Spec URL: https://fedorapeople.org/~mbooth/reviews/trilead-ssh2.spec
SRPM URL: https://fedorapeople.org/~mbooth/reviews/trilead-ssh2-217.21-2.fc33.src.rpm

Great. Package APPROVED :)

Thanks for the review, rawhide build is here: https://koji.fedoraproject.org/koji/buildinfo?buildID=1544193