Spec URL: https://gil.fedorapeople.org/ecc-25519-java.spec SRPM URL: https://gil.fedorapeople.org/ecc-25519-java-1.0.1-1.fc23.src.rpm Description: A library for Java and Android to use Ed25519 and Curve25518. Fedora Account System Username: gil sshj dependency new url https://github.com/hierynomus/sshj/tags
jerboaa's scratch build of java-1.8.0-openjdk?#d28765c33d068af9ff432a92443b93beeef88a22 for git://pkgs.fedoraproject.org/java-1.8.0-openjdk?#d28765c33d068af9ff432a92443b93beeef88a22 and rawhide failed http://koji.fedoraproject.org/koji/taskinfo?taskID=12181621
Spec URL: https://gil.fedorapeople.org/ecc-25519-java.spec SRPM URL: https://gil.fedorapeople.org/ecc-25519-java-1.0.1-1.fc24.src.rpm
Spec URL: https://gil.fedorapeople.org/ecc-25519-java.spec SRPM URL: https://gil.fedorapeople.org/ecc-25519-java-1.0.3-1.fc24.src.rpm - update to 1.0.3 Task info: http://koji.fedoraproject.org/koji/taskinfo?taskID=15809989
I'm taking the review. But first I wonder why this isn't part of the standard openjdk distribution. Is there any issue with this implementation of the ECC algo ? Blocking FE-Legal about this.
I'm looking into this.
Any new news?
The curves in this implementation are permissible for Fedora. Lifting FE-Legal.
@Nicolas can you continue with the review?
Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated Issues: ======= - This seems like a Java package, please install fedora-review-plugin-java to get additional checks ===== 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. Note: Checking patched sources after %prep for licenses. Licenses found: "Apache (v2.0)", "Unknown or generated", "*No copyright* Apache (v2.0)". 75 files have unknown license. Detailed output of licensecheck in /home/builder/1290342-ecc-25519-java/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [x]: If the package is under multiple licenses, the licensing breakdown must be documented in the spec. [x]: Package contains no bundled libraries without FPC exception. [!]: 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 [?]: Package uses nothing in %doc for runtime. [!]: 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. [-]: 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. Note: Documentation size is 10240 bytes in 2 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: No rpmlint messages. [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 must own all directories that it creates. [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]: Dist tag is present. [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 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: [x]: 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). [x]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in ecc-25519-java-javadoc [x]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: 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. [?]: 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: No rpmlint messages. [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: ecc-25519-java-1.0.3-1.fc26.noarch.rpm ecc-25519-java-javadoc-1.0.3-1.fc26.noarch.rpm ecc-25519-java-1.0.3-1.fc26.src.rpm 3 packages and 0 specfiles checked; 0 errors, 0 warnings. Summary: - I'm not sure we can consider the license is public domain since few sources files do not contains a license header. I know this would fall under the definition of public domain in the guideline. But would be contradicted by: https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Public_Domain I think that the intention of the author is to have everything licensed under ASL 2.0 but it would requires clarification. - You are missing a "-" between email and package EVR in changelog. rpmlint haven't found it and I don't expect it's a hard requirement. But at least that the format used by the rpmdev-bumpspec tool - What the point to use /usr/bin/perl over perl (as package) directly ? or at least the appropriate macro ? None of theses issues are blocker, so it should be okay
(In reply to Nicolas Chauvet (kwizart) from comment #9) > [!]: License field in the package spec file matches the actual license. > Note: Checking patched sources after %prep for licenses. Licenses > found: "Apache (v2.0)", "Unknown or generated", "*No copyright* Apache > (v2.0)". 75 files have unknown license. Detailed output of > licensecheck in /home/builder/1290342-ecc-25519-java/licensecheck.txt "75 files have unknown license" refer to these files? ./ECC-25519-Android/ecc-25519/src/main/java/com/github/dazoe/android/Ed25519.java /ECC-25519-Android*/ecc-25519/src/main/jni/* i don't want import these features > [!]: Changelog in prescribed format. for me is ok > [?]: Package uses nothing in %doc for runtime. > [!]: Package consistently uses macros (instead of hard-coded directory > names). > [-]: %check is present and all tests pass. test suite are executed as primary task, is not necessary add useless "sections" > [?]: Packages should try to preserve timestamps of original installed > files. yes handled by our java tools > Summary: > - I'm not sure we can consider the license is public domain since few > sources files do not contains a license header. I know this would fall under > the definition of public domain in the guideline. But would be contradicted > by: > https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Public_Domain > I think that the intention of the author is to have everything licensed > under ASL 2.0 but it would requires clarification. Not necessary, under Public Domain license are those files that i remove as bundled code ECC-25519-Java/src/main/java/djb > - You are missing a "-" between email and package EVR in changelog. rpmlint > haven't found it and I don't expect it's a hard requirement. But at least > that the format used by the rpmdev-bumpspec tool I dont want use "-" in my spec file, and i am not interested to use that tool > - What the point to use /usr/bin/perl over perl (as package) directly ? or > at least the appropriate macro ? I need only the binary for change the build scripts ... > None of theses issues are blocker, so it should be okay Spec URL: https://gil.fedorapeople.org/ecc-25519-java.spec SRPM URL: https://gil.fedorapeople.org/ecc-25519-java-1.0.3-1.fc24.src.rpm - fix license field
Please,
@Nicolas Please, can you change fedora-review field with "+"
(In reply to gil cattaneo from comment #10) > > - You are missing a "-" between email and package EVR in changelog. rpmlint > > haven't found it and I don't expect it's a hard requirement. But at least > > that the format used by the rpmdev-bumpspec tool > > I dont want use "-" in my spec file, and i am not interested to use that tool It's a weird answer. I wouldn't have expected it. The problem is that other maintainers and provenpackager will use theses tools, or even use parser that may break if the "-" isn't found. Above all it's improving readability. > > - What the point to use /usr/bin/perl over perl (as package) directly ? or > > at least the appropriate macro ? > > I need only the binary for change the build scripts ... okay, but %{_bindir} should be used here why not using the macro ? Also I don't think perl packaging will change it's packaging scheme not to have the perl binary in the main perl package. That been said, I would use sed (that is in the default buildroot) instead to avoid a dependency on perl (that is no more in the default buildroot).
(In reply to Nicolas Chauvet (kwizart) from comment #13) > (In reply to gil cattaneo from comment #10) > > > > - You are missing a "-" between email and package EVR in changelog. rpmlint > > > haven't found it and I don't expect it's a hard requirement. But at least > > > that the format used by the rpmdev-bumpspec tool > > > > I dont want use "-" in my spec file, and i am not interested to use that tool > > It's a weird answer. I wouldn't have expected it. > The problem is that other maintainers and provenpackager will use theses > tools, or even use parser that may break if the "-" isn't found. > Above all it's improving readability. NOT is a problem, this is because the people you've indicated you have no problems to make changes in my spec files. and I repeat I am not interested just about aesthetics without any functionality > > > - What the point to use /usr/bin/perl over perl (as package) directly ? or > > > at least the appropriate macro ? > > > > I need only the binary for change the build scripts ... > okay, but %{_bindir} should be used here why not using the macro ? > Also I don't think perl packaging will change it's packaging scheme not to > have the perl binary in the main perl package. Done > That been said, I would use sed (that is in the default buildroot) instead > to avoid a dependency on perl (that is no more in the default buildroot). Not in my spec file, unless it is forbidden to use in some guideline Spec URL: https://gil.fedorapeople.org/ecc-25519-java.spec SRPM URL: https://gil.fedorapeople.org/ecc-25519-java-1.0.3-1.fc24.src.rpm - use "%{_bindir}" macro
(In reply to gil cattaneo from comment #14) > (In reply to Nicolas Chauvet (kwizart) from comment #13) > > (In reply to gil cattaneo from comment #10) > > > > > > - You are missing a "-" between email and package EVR in changelog. rpmlint > > > > haven't found it and I don't expect it's a hard requirement. But at least > > > > that the format used by the rpmdev-bumpspec tool > > > > > > I dont want use "-" in my spec file, and i am not interested to use that tool > > > > It's a weird answer. I wouldn't have expected it. > > The problem is that other maintainers and provenpackager will use theses > > tools, or even use parser that may break if the "-" isn't found. > > Above all it's improving readability. > > NOT is a problem, this is because the people you've indicated you have no > problems to make changes in my spec files. and I repeat I am not interested > just about aesthetics without any functionality I'm afraid you don't consider readability as the feature. There is something weird reading most of your spec changelogs. It makes eyes bleeding because something is missing, until one discover what it is. > > > > - What the point to use /usr/bin/perl over perl (as package) directly ? or > > > > at least the appropriate macro ? > > > > > > I need only the binary for change the build scripts ... > > okay, but %{_bindir} should be used here why not using the macro ? > > Also I don't think perl packaging will change it's packaging scheme not to > > have the perl binary in the main perl package. > Done > > That been said, I would use sed (that is in the default buildroot) instead > > to avoid a dependency on perl (that is no more in the default buildroot). > Not in my spec file, unless it is forbidden to use in some guideline https://fedoraproject.org/wiki/Changes/Build_Root_Without_Perl https://fedoraproject.org/wiki/Packaging:Guidelines#Scripting_inside_of_spec_files Given that perl isn't in the default buildroot anymore, it will simplify the dependency graph. If you can show me a rationale argument for both issues, I'm okay to approve the package. Alternative is that I will resign from review and you will find a new reviewer.
Just to be clear on rpmdev-newspec, I'm not saying that you must use it. You can adapt your current tools to use this scheme instead.
Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated ISSUES ====== [!]: Requires correct, justified where necessary. - please justify the use of perl (in spec also) QUESTIONS ========= Are these two the only sources the package compiles? ECC-25519-Java/src/main/java/net/vrallev/java/ecc/Ecc25519Helper.java ECC-25519-Java/src/main/java/net/vrallev/java/ecc/KeyHolder.java ===== 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. [x]: License file installed when any subpackage combination is installed. [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. [?]: 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. [!]: 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. [-]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 10240 bytes in 2 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. [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 must own all directories that it creates. [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]: Dist tag is present. [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 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 [x]: Packages have proper BuildRequires/Requires on jpackage-utils [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 metadata) even when building with ant [x]: Old add_to_maven_depmap macro is not being used ===== 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). [x]: 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. [-]: 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. [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. Java: [x]: Package uses upstream build method (ant/maven/etc.) [x]: Packages are noarch unless they use JNI ===== EXTRA items ===== Generic: [x]: Rpmlint is run on all installed packages. Note: No rpmlint messages. Rpmlint ------- Checking: ecc-25519-java-1.0.3-1.fc26.noarch.rpm ecc-25519-java-javadoc-1.0.3-1.fc26.noarch.rpm ecc-25519-java-1.0.3-1.fc26.src.rpm 3 packages and 0 specfiles checked; 0 errors, 0 warnings. Rpmlint (installed packages) ---------------------------- 2 packages and 0 specfiles checked; 0 errors, 0 warnings. Requires -------- ecc-25519-java (rpmlib, GLIBC filtered): java-headless javapackages-tools mvn(net.i2p.crypto:eddsa) mvn(org.zeromq:curve25519-java) ecc-25519-java-javadoc (rpmlib, GLIBC filtered): javapackages-tools Provides -------- ecc-25519-java: ecc-25519-java mvn(net.vrallev.ecc:ecc-25519-java) osgi(net.vrallev.ecc.25519-java) ecc-25519-java-javadoc: ecc-25519-java-javadoc Source checksums ---------------- https://github.com/vRallev/ECC-25519/archive/464d4be3451f3742a00d6503a85d6b3b399b85da/ECC-25519-1.0.3.tar.gz : CHECKSUM(SHA256) this package : 8d756ad54710eae7e5ad16e75d0690aff304d9b8db1302cde804633e1d95d5d6 CHECKSUM(SHA256) upstream package : 8d756ad54710eae7e5ad16e75d0690aff304d9b8db1302cde804633e1d95d5d6 Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02 Command line :/usr/bin/fedora-review --rpm-spec -n ecc-25519-java-1.0.3-1.fc24.src.rpm Buildroot used: fedora-rawhide-x86_64 Active plugins: Generic, Shell-api, Java Disabled plugins: C/C++, Python, fonts, SugarActivity, Ocaml, Perl, Haskell, R, PHP Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6
(In reply to Tomas Repik from comment #17) > ISSUES > ====== > [!]: Requires correct, justified where necessary. > - please justify the use of perl (in spec also) I should add a comment in the spec file? it is not obvious its use? > QUESTIONS > ========= > Are these two the only sources the package compiles? > ECC-25519-Java/src/main/java/net/vrallev/java/ecc/Ecc25519Helper.java > ECC-25519-Java/src/main/java/net/vrallev/java/ecc/KeyHolder.java Yes, bundled code is removed in the %prep section Originally this library should contain: ECC-25519-Java/src/main/java/djb curve25519-java or org.zeromq:curve25519-java ECC-25519-Java/src/main/java/net/i2p ed25519-java or net.i2p.crypto:eddsa:0.1.0 thanks
(In reply to gil cattaneo from comment #18) > I should add a comment in the spec file? > it is not obvious its use? I mean to find/replace strings in source files one usually uses sed. I've seen perl used for the first time here. I can't see the advantage of using perl over using standard sed. If there was a specific reson to use perl and thus add a build dependency, just mention it somewhere. Otherwise, if there was no advantage of using perl please consider rewriting it with sed. Thanks
(In reply to Tomas Repik from comment #19) > (In reply to gil cattaneo from comment #18) > > I should add a comment in the spec file? > > it is not obvious its use? > > I mean to find/replace strings in source files one usually uses sed. I've > seen perl used for the first time here. I can't see the advantage of using > perl over using standard sed. If there was a specific reson to use perl and > thus add a build dependency, just mention it somewhere. Otherwise, if there > was no advantage of using perl please consider rewriting it with sed. Done > Thanks Spec URL: https://gil.fedorapeople.org/ecc-25519-java.spec SRPM URL: https://gil.fedorapeople.org/ecc-25519-java-1.0.3-1.fc24.src.rpm
Thanks you did a good job!
Thanks for the review! create new SCM requests: https://admin.fedoraproject.org/pkgdb/package/requests/8993
Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/rpms/ecc-25519-java
Task info: http://koji.fedoraproject.org/koji/taskinfo?taskID=16660852