Bug 2444639

Copr build:
https://copr.fedorainfracloud.org/coprs/build/10191241
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2444639-flood/fedora-rawhide-x86_64/10191241-flood/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Please look at the packaging guidelines:

https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/

My concerns are specifically the bundling which likely happens in this package, therefore these two sections are my concern:

https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/#_bundled_licenses
https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/#_automatic_requires_and_provides

Thank you for the review!

I've updated the package to properly use the LICENSE file that was already present in the source, as well as to include licenses of the bundled dependencies. To get the list of licenses I've modified the project's build script and ran it right after the commit of the last release.
Here's the build script change: https://github.com/mks-h/flood/commit/16746a31082d702372bf8dcf3580c721abfad538
I'll try to upstream this once the package passes review.

As for the automatic requires and provides, they aren't present in this package since the project doesn't distribute node_modules. Instead, the bundle is transformed and minified into chunks of JavaScript files under the assets directory (plus the entry-point in bin).

Here's the provides of the binary rpm:
flood = 4.13.0-3.fc45

Here's the requires of the binary rpm:
/bin/sh
/bin/sh
/bin/sh
/usr/bin/node
nodejs
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
rpmlib(PayloadFilesHavePrefix) <= 4.0-1
rpmlib(PayloadIsZstd) <= 5.4.18-1

The source RPM differs by only requiring this:
rpmlib(CompressedFileNames) <= 3.0.4-1
rpmlib(FileDigests) <= 4.6.0-1
systemd-rpm-macros

I've tried to add a Node.js require in %check, like documented: https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/#_build_testing_in_check
But since this isn't a library package, just requiring it makes it run the webserver (indefinitely).

I've also noticed and fixed the use of %setup macro from downloading the npm source twice (for some reason I've been using '-b 0' without '-T', so I removed it).

Last thing I changed, is I replaced the %autorelease macro with the manual release number, since my commit history is not a reliable way to tell how many builds I released on COPR. I can change it back to %autorelease before the package get its own Fedora Package Sources repo, if absolutely necessary. BTW, is it fine if the release number skips e.g. from 1 to 3 because of a commit that wasn't used to actually release a build?

Updated spec file: https://download.copr.fedorainfracloud.org/results/mks-h/Flood/fedora-43-x86_64/10198757-flood/flood.spec
Updated SRPM: https://download.copr.fedorainfracloud.org/results/mks-h/Flood/fedora-43-x86_64/10198757-flood/flood-4.13.0-4.fc43.src.rpm

Created attachment 2132367 [details]
The .spec file difference from Copr build 10191241 to 10198856

Copr build:
https://copr.fedorainfracloud.org/coprs/build/10198856
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2444639-flood/fedora-rawhide-x86_64/10198856-flood/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

(In reply to Maksym Hazevych from comment #3)
> Thank you for the review!

These were just a few hints. Sorry but I don't have ambitions to do a full review and I might went silent any time.

> I've updated the package to properly use the LICENSE file that was already
> present in the source, as well as to include licenses of the bundled
> dependencies. To get the list of licenses I've modified the project's build
> script and ran it right after the commit of the last release.
> Here's the build script change:
> https://github.com/mks-h/flood/commit/
> 16746a31082d702372bf8dcf3580c721abfad538
> I'll try to upstream this once the package passes review.

👍

> As for the automatic requires and provides, they aren't present in this
> package since the project doesn't distribute node_modules. Instead, the
> bundle is transformed and minified into chunks of JavaScript files under the
> assets directory (plus the entry-point in bin).

If the automation worked, that would be for your benefit. But if it does not work, then the provides still needs be provided somehow. This is the JS specific guideline referring to the generic guidelines:

https://docs.fedoraproject.org/en-US/packaging-guidelines/JavaScript/#_bundling_of_other_libraries

> I've tried to add a Node.js require in %check, like documented:
> https://docs.fedoraproject.org/en-US/packaging-guidelines/Node.js/
> #_build_testing_in_check
> But since this isn't a library package, just requiring it makes it run the
> webserver (indefinitely).

Good effort. The webserver could be run in background and queried by `curl`?

There is also a way to run upstream test suite:

https://github.com/jesec/flood/blob/cfc961c7e6620f3b53460034305488b1725d7278/package.json#L58

Although I don't know what it does specifically and I would not be surprised if `vitest` is not in Fedora.

Checking the Debian package included in the sources, it does something very minimal, but sometimes better than nothing:

https://github.com/jesec/flood/blob/master/distribution/debian/flood/tests/pkg-js/test

> BTW, is it fine if the release number skips e.g. from 1 to 3
> because of a commit that wasn't used to actually release a build?

That is fine. The only thing which matters is upgrade path. So the older package has to have lover NVR (name-version-release) then the new one. You can use `rpmdev-vercmp` for version comparison

> These were just a few hints.

I appreciate it nonetheless, the guidelines are quite overwhelming for a newcomer.

I've updated the package to have Provides, as well as the same kind of test in check as there is in the linked debian package.

> There is also a way to run upstream test suite:

The upstream test suite is quite complex, requires rTorrent to be installed, and seems to have a Dockerfile for its environment... It doesn't succeed for me, probably because I missed some of the complex setup. I don't think it's worth the trouble to try to make it work in a package check.

Updated spec file: https://download.copr.fedorainfracloud.org/results/mks-h/Flood/fedora-44-x86_64/10220436-flood/flood.spec
Updated SRPM: https://download.copr.fedorainfracloud.org/results/mks-h/Flood/fedora-44-x86_64/10220436-flood/flood-4.13.0-6.fc44.src.rpm

Also, there's both nodejs and nodejs-devel in the BuildRequires, because Fedora 42 and 43 wouldn't build with just nodejs-devel (unlike Fedora 44, which didn't mind).

Created attachment 2133234 [details]
The .spec file difference from Copr build 10198856 to 10220441

Copr build:
https://copr.fedorainfracloud.org/coprs/build/10220441
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2444639-flood/fedora-rawhide-x86_64/10220441-flood/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.