Bug 2274250

This package built on koji:  https://koji.fedoraproject.org/koji/taskinfo?taskID=116150262

Note: I'm not 100% I got the alternatives stuff right (or even if alternatives is the right thing to use here).

Some early comments.

Why are clblast, tclap and sha2 bundled ?

could a %check be added ?
maybe katago runtests

$ rpm -i katogo-opengl
$ katago benchmark
terminate called after throwing an instance of 'StringError'
  what():  -model MODELFILENAME.bin.gz was not specified to tell KataGo where to find the neural net model, and default was not found at /usr/bin/default_model.bin.gz
Aborted (core dumped)

Where is model ?
And looking for default in bin/ is not great.

katago.x86_64: W: package-with-huge-docs 99%
This package is mostly /usr/share/katago/doc, maybe have a doc subpackage.

some lint errors
katago-opencl.x86_64: I: package-supports-update-alternatives
katago-opencl.x86_64: E: non-readable /usr/bin/katago 0
katago-opencl.x86_64: W: non-executable-in-bin /usr/bin/katago 0
katago-opencl.x86_64: W: no-manual-page-for-binary katago-opencl
katago-opencl.x86_64: W: no-documentation
katago-opencl.x86_64: E: alternative-link-missing /etc/alternatives/katago
katago-opencl.x86_64: E: alternative-generic-name-not-symlink /usr/bin/katago

Thanks!

> Why are clblast, tclap and sha2 bundled ?

clblast is a heavily cut down and modified copy of the datafiles that clblast upstream provides; tclap is a modified copy of a oneoff upstream version; sha2 is a modified copy and it's deeply integrated into the source.

> could a %check be added ?
> maybe katago runtests

The tests don't work without a model

> Where is model ?

https://katagotraining.org, see the included README.fedora. This changes often and doesn't have a stable URI, so I didn't think it made sense to include it in the package.

> And looking for default in bin/ is not great.

Agreed, afaict it looks for where the binary is, but the code wasn't super clear. I'll get an upstream issue filed for this.

> katago.x86_64: W: package-with-huge-docs 99%
> This package is mostly /usr/share/katago/doc, maybe have a doc subpackage.

Sure, I can do that. It would mean the main package becomes just empty, which is probably fine.

> some lint errors

I suspect these are all side effects of alternatives not playing well with rpmlint, but I will double check.

> Agreed, afaict it looks for where the binary is, but the code wasn't super clear. I'll get an upstream issue filed for this.

Looks like this is by design: https://github.com/lightvector/KataGo/blob/cf75cc7dc21ee219bea8e60d869c142332c1b57c/cpp/dataio/homedata.cpp#L86

Spec URL: https://dcavalca.fedorapeople.org/review/katago/katago.spec
SRPM URL: https://dcavalca.fedorapeople.org/review/katago/katago-1.14.1-1.fc41.src.rpm

Changelog:
- move docs to a -doc subpackage

When i do

# rpm -ihv review-katago/results/katago-1.14.1-1.fc41.x86_64.rpm 
error: Failed dependencies:
	katago-backend = 1.14.1-1.fc41 is needed by katago-1.14.1-1.fc41.x86_64

Then i have to figure out which to use 'eigen' or 'opencl' 
I think it would be easier on the user if the 'eigen' subpackage was collapsed to the main package.

The setup assumes the user is going to read the README.fedora.
I did not the first time.

The nn url in the readme could be improved to https://katagotraining.org/networks/

Could a katago-setup setup script be added to automate the instructions ?

Some other nits from the review
katago.spec:40: W: unversioned-explicit-provides bundled(clblast)                                                                                                                
katago.spec:43: W: unversioned-explicit-provides bundled(sha2)                                                                                                                   
Take a guess on the versions.
                                                                                                                                                 
katago.spec:60: W: mixed-use-of-spaces-and-tabs (spaces: line 12, tab: line 60)

Is there going to be a follow on gui package to go with this engine ?

(In reply to Tom Rix from comment #7)
> When i do
> 
> # rpm -ihv review-katago/results/katago-1.14.1-1.fc41.x86_64.rpm 
> error: Failed dependencies:
> 	katago-backend = 1.14.1-1.fc41 is needed by katago-1.14.1-1.fc41.x86_64
> 
> Then i have to figure out which to use 'eigen' or 'opencl' 
> I think it would be easier on the user if the 'eigen' subpackage was
> collapsed to the main package.

This is a side effect of using rpm -i. If the packages are all in a repo and one does dnf install katago, dnf should automatically pull in the eigen subpackage.
 
> The setup assumes the user is going to read the README.fedora.
> I did not the first time.
> 
> The nn url in the readme could be improved to
> https://katagotraining.org/networks/

Will fix, thanks

> Could a katago-setup setup script be added to automate the instructions ?

Not easily, as there isn't a stable URL to the "last good network". We'd have to scrape it from that web page, which doesn't seem worth the trouble.

> Some other nits from the review
> katago.spec:40: W: unversioned-explicit-provides bundled(clblast)           
> 
> katago.spec:43: W: unversioned-explicit-provides bundled(sha2)              
> 
> Take a guess on the versions.

These don't really _have_ a version unfortunately :(

> katago.spec:60: W: mixed-use-of-spaces-and-tabs (spaces: line 12, tab: line
> 60)

Will fix, thanks.

> Is there going to be a follow on gui package to go with this engine ?

Yes, I have a half-done package for q5go I'm planning to submit for review.

Spec URL: https://dcavalca.fedorapeople.org/review/katago/katago.spec
SRPM URL: https://dcavalca.fedorapeople.org/review/katago/katago-1.14.1-1.fc41.src.rpm

Changelog:
- update URL in the README
- fix tabs

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

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2274250-katago/fedora-rawhide-x86_64/07482070-katago/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.

Thanks!
This is enough for me.
Appoved.

The Pagure repository was created at https://src.fedoraproject.org/rpms/katago

FEDORA-2024-89b551f20d (katago-1.14.1-1.fc41) has been submitted as an update to Fedora 41.
https://bodhi.fedoraproject.org/updates/FEDORA-2024-89b551f20d

FEDORA-2024-89b551f20d (katago-1.14.1-1.fc41) has been pushed to the Fedora 41 stable repository.
If problem still persists, please make note of it in this bug report.

FEDORA-2024-a8933eeeee (katago-1.14.1-1.fc40) has been submitted as an update to Fedora 40.
https://bodhi.fedoraproject.org/updates/FEDORA-2024-a8933eeeee

FEDORA-2024-a8933eeeee has been pushed to the Fedora 40 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2024-a8933eeeee \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2024-a8933eeeee

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

FEDORA-2024-a8933eeeee (katago-1.14.1-1.fc40) has been pushed to the Fedora 40 stable repository.
If problem still persists, please make note of it in this bug report.