Fedora Account System
Red Hat Associate
Red Hat Customer
Spec URL: https://trix.fedorapeople.org/olla.spec SRPM URL: https://trix.fedorapeople.org/olla-0.0.27-1.fc45.src.rpm A writeup of https://fedoraproject.org/wiki/SIGs/AI-ML#Olla Reproducible: Always
Copr build: https://copr.fedorainfracloud.org/coprs/build/10579177 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2486352-olla/fedora-rawhide-x86_64/10579177-olla/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.
Spec URL: https://trix.fedorapeople.org/olla.spec SRPM URL: https://trix.fedorapeople.org/olla-0.0.28-1.fc45.src.rpm Updated to 0.28 Added systemd support
Created attachment 2146163 [details] The .spec file difference from Copr build 10579177 to 10622240
Copr build: https://copr.fedorainfracloud.org/coprs/build/10622240 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2486352-olla/fedora-rawhide-x86_64/10622240-olla/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.
Here are my initial remarks (not really blocking): 1. Is there a need to put systemd integration behind a flag for Fedora? It looks like much noise and I don't really see a benefit there. 2. The "olla.service" file sets the target to "default.target". However, that usually is just a symlink to "graphical.target". The docs [1] also suggest using "graphical.target" directly. Any reason for choosing "default.target" over "graphical.target"? 3. Please use "%{_sharedstatedir}" instead of "%{_var}/lib" 4. Why did you put the configuration files in "%{_var}/lib/olla" instead of "%{_sysconfdir}/olla"? 5. Why did you configure the log directory to be "/var/lib/olla/logs" instead of "/var/log/olla"? [1] https://docs.fedoraproject.org/en-US/packaging-guidelines/Systemd/
1. Is there a need to put systemd integration behind a flag for Fedora? It looks like much noise and I don't really see a benefit there. ok. 2. The "olla.service" file sets the target to "default.target". However, that usually is just a symlink to "graphical.target". The docs [1] also suggest using "graphical.target" directly. Any reason for choosing "default.target" over "graphical.target"? or multi-user.target, which is also a symlink to default. This is network application, it could be run in the gui-less server. 3. Please use "%{_sharedstatedir}" instead of "%{_var}/lib" ok. 4. Why did you put the configuration files in "%{_var}/lib/olla" instead of "%{_sysconfdir}/olla"? 5. Why did you configure the log directory to be "/var/lib/olla/logs" instead of "/var/log/olla"? To keep the dir created count down to 1.