https://bugzilla.redhat.com/show_bug.cgi?id=2404675



--- Comment #4 from jiri vanek <[email protected]> ---
Thanx a lot for careful review!

All should be fixed.

(In reply to Marián Konček from comment #3)
> = NOTES from the reviewer:
> == IMPORTANT:
> * %prep section prints .jar and .class files but does not remove them. They
> should be removed unless required. At least the gradle.jar should be removed.
> * test/resources contains zip files containing .class files with unknown
> licensing, I don't think we can distribute these in the source RPM. So you
> may have to write a `generate-tarball.sh` script.

Thanx! All removed now in prep. That should be ok.  Except gradle.jar it is
testing data nad tests do not run. Afiak it is ok to leave them in sources.

> * License field is invalid SPDX expression, should be: `GPL-3.0-or-later`.

fixed

> * Please replace `BuildRequires:  java-25-devel` with
> `javapackages-local-openjdk25` which is the standard form of expressing
> requiring Java for building packages.

fixed... but javapackages-local-openjdk25 is available only in f43 onwards. For
older java-25-devele must be used anyway.
> 
> == LESS IMPORTANT
> * Consider using %autorelease since you are already using %autochangelog.

Pleas no. Autorelease had bitten me to often. Great feature was unluckily
implemnted in weird way.

> * Inconsistent indentation.

Fixed with bwest intentions, hope yu will find it satisfying. 

> * Description is incomplete.

fixed
> * Summary could use more precise capital letters, like: `JD Java decompiler
> library`.

ok, capitlaised JD. but intentionally left Java lowercase.

> * $RPM_BUILD_ROOT can be replaced with %{buildroot}.

Thanx! WAs not aware actually.

> * There are trailing whitespaces after License and %description, maybe more.

Should begone, no more found
> * Debug prints (pwd, ls) in %install sgould not be in a standard .spec.

Well, why not...but removed if you need so.
> * Please unindent the code under the %build section.

done.
> * Various random spaces that make no sense inside the %build section.

done! 

> * Backticks for subprocesses are discouraged in favor of $().
Done!

> 
> Package Review
> ==============
> 
> Legend:
> [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
> [ ] = Manual review needed
> 
> 
> Issues:
> =======
> - The License field must be a valid SPDX expression.
>   Note: Not a valid SPDX expression 'GPL-3.0'.
>   See: https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1

fixed.

> - Javadoc documentation files are generated and included in -javadoc
>   subpackage
>   Note: No javadoc subpackage present. Note: Javadocs are optional for
>   Fedora versions >= 21
>   See: https://fedoraproject.org/wiki/Packaging:Java#Javadoc_installation

No javadocs intended. WOPuld be of poor quality anyway.

> - Javadocs are placed in %{_javadocdir}/%{name} (no -%{version} symlink)
>   Note: No javadoc subpackage present
>   See: https://fedoraproject.org/wiki/Packaging:Java#Javadoc_installation
> - Bundled jar/class files should be removed before build
>   Note: Jar files in source (see attachment)
>   See: https://docs.fedoraproject.org/en-US/packaging-
>   guidelines/Java/#_pre_built_dependencies

Fixed. Thanx  forget to remove rm, after printing them....

> [!]: Sources contain only permissible code or content.

I dont think the testing data or gradle jar are a reason for custom genertae
sources. I had removed all ajrs, zips and classes in prep, that hsoudl be
enough.
...
> [?]: Package functions as described.

Library can be used. => ok

> [x]: Latest version is packaged.
> [x]: Package does not include license text files separate from upstream.
> [-]: Sources are verified with gpgverify first in %prep if upstream
>      publishes signatures.
>      Note: gpgverify is not used.
> [!]: %check is present and all tests pass.

nope - test data gone, gradl enot present, no intention to work on them
anymore. Not worthy.

> [?]: Packages should try to preserve timestamps of original installed
>      files.

No "install" included.

> [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]: Package should compile and build into binary rpms on all supported
>      architectures.
> [x]: Spec use %global instead of %define unless justified.
> 
> Java:
> [!]: Package uses upstream build method (ant/maven/etc.)

gradle, nothign to do about it.
> [x]: Packages are noarch unless they use JNI
> 
> ===== 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)

Weird. But shodl be aligned now.

> [x]: Rpmlint is run on all installed packages.
>      Note: There are rpmlint messages (see attachment).
> 
> 
> Rpmlint
> -------
> Checking: jd-core-1.1.3-0.fc44.noarch.rpm
>           jd-core-1.1.3-0.fc44.src.rpm
> ============================ rpmlint session starts
> ============================
> rpmlint: 2.6.1
> configuration:
>     /usr/lib/python3.13/site-packages/rpmlint/configdefaults.toml
>     /etc/xdg/rpmlint/fedora-legacy-licenses.toml
>     /etc/xdg/rpmlint/fedora-spdx-licenses.toml
>     /etc/xdg/rpmlint/fedora.toml
>     /etc/xdg/rpmlint/scoring.toml
>     /etc/xdg/rpmlint/users-groups.toml
>     /etc/xdg/rpmlint/warn-on-functions.toml
> rpmlintrc: [PosixPath('/tmp/tmpawwty7tb')]
> checks: 32, packages: 2
> 
> jd-core.noarch: W: summary-not-capitalized jd java decompiler library
> jd-core.src: W: summary-not-capitalized jd java decompiler library
> jd-core.noarch: E: spelling-error ('decompiler', 'Summary(en_US) decompiler
> -> recompile, compiler')
> jd-core.noarch: E: spelling-error ('decompiler', '%description -l en_US
> decompiler -> recompile, compiler')
> jd-core.src: E: spelling-error ('decompiler', 'Summary(en_US) decompiler ->
> recompile, compiler')
> jd-core.src: E: spelling-error ('decompiler', '%description -l en_US
> decompiler -> recompile, compiler')
> jd-core.spec: W: no-%check-section
> jd-core.noarch: W: invalid-license GPL-3.0
> jd-core.src: W: invalid-license GPL-3.0
> jd-core.noarch: W: incoherent-version-in-changelog 1.1.3-1 ['1.1.3-0.fc44',
> '1.1.3-0']
>  2 packages and 0 specfiles checked; 4 errors, 6 warnings, 9 filtered, 4
> badness; has taken 0.3 s 
> 
> 
> 
> 
> Rpmlint (installed packages)
....
Walked through. Fixed what could be fixed.

Thanx again for nice review!


-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
https://bugzilla.redhat.com/show_bug.cgi?id=2404675

Report this comment as SPAM: 
https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202404675%23c4

-- 
_______________________________________________
package-review mailing list -- [email protected]
To unsubscribe send an email to [email protected]
Fedora Code of Conduct: 
https://docs.fedoraproject.org/en-US/project/code-of-conduct/
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: 
https://lists.fedoraproject.org/archives/list/[email protected]
Do not reply to spam, report it: 
https://pagure.io/fedora-infrastructure/new_issue

Reply via email to