Fedora Account System
Red Hat Associate
Red Hat Customer
Spec URL: https://fedorapeople.org/~mbooth/reviews/trilead-ssh2.spec SRPM URL: https://fedorapeople.org/~mbooth/reviews/trilead-ssh2-217-13.jenkins8.fc33.src.rpm Fedora Account System Username: mbooth Description: Trilead SSH-2 for Java is a library which implements the SSH-2 protocol in pure Java (tested on J2SE 1.4.2 and 5.0). It allows one to connect to SSH servers from within Java programs. It supports SSH sessions (remote command execution and shell access), local and remote port forwarding, local stream forwarding, X11 forwarding and SCP. There are no dependencies on any JCE provider, as all crypto functionality is included.
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