Spec URL: https://github.com/LorbusChris/libreoffice-online-rpm/blob/spec/libreoffice-online.spec RAW Spec: https://raw.githubusercontent.com/LorbusChris/libreoffice-online-rpm/spec/libreoffice-online.spec SRPM URL: https://copr-be.cloud.fedoraproject.org/results/lorbus/libreoffice-online/fedora-rawhide-x86_64/00607283-libreoffice-online/libreoffice-online-canary-20170924054239.fc28.src.rpm COPR Builds: https://copr.fedorainfracloud.org/coprs/lorbus/libreoffice-online/monitor/ Description: This spec builds libreoffice-online, or otherwise known as loolwsd For now, it builds canary rpms from upstream master, as there is not yet a release containing the required commits (namely support for OpenSSL 1.1.0). Will update accordingly after next release. I would appreciate a first round of reviews now, prior to this. Fedora Account System Username: lorbus
Version 5.4.2.2 has been released. I have updated the spec accordingly. SPEC: https://github.com/LorbusChris/libreoffice-online-rpm/blob/master/libreoffice-online.spec COPRs: https://copr.fedorainfracloud.org/coprs/lorbus/libreoffice-online/monitor/
>env BUILDING_FROM_RPMBUILD=yes make ... I believe using /bin/env inside spec files is discouraged. >%__install -D -m 444 loolwsd.service %{buildroot}%{_unitdir}/loolwsd.service >install -d -m 755 %{buildroot}%{_localstatedir}/adm/fillup-templates Why the mix of "install" and "%__install"? Non-macro forms of system executables are preferred. As for "%{buildroot}%{_sysconfdir}/cron.d/loolwsd.cron", I think it'd be cleaner to use cat and a heredoc to create the file: >cat > output_file <<EOF >Line 1 of file. >Line 2 of file. >EOF You could, alternatively, just create said file and add it as Source1. >%{_datadir}/%{name}/favicon.ico >%{_datadir}/%{name}/discovery.xml >%{_datadir}/%{name}/robots.txt >%{_datadir}/%{name}/loleaflet/* This makes the package own the files inside %{_datadir}/%{name}, but the directory itself is unowned.
Thanks for the review, it's highly appreciated! Do you have any ideas how to do the build without the env var? My idea would be to patch the configure and make files so that configure recognizes a --building-from-rpmbuild flag (or similar) instead of the makefile using the env var. Does that sound about right to you or do you see an easier way of doing it? The other issues mentioned will be adressed shortly! Cheers, Chris
One more thing that probably needs to be adressed before getting this into Fedora: The loleaflet part of the build invokes `npm install` in the build. This isn't allowed in Fedora. There is a npm-shrinkwrap file for versioning the deps. Does anybody know of a good way to source these deps? Maybe add an individual source file for each of them or `npm install` locally beforehand and tar node_modules to put it in one source file?
Quite a lot of stuff that's available from npm is available in Fedora repos as 'nodejs-*' packages. There's a chance that we might be already packaging all the needed dependencies, though using this will probably mean you need to patch the Makefile accordingly.
I patched the build to not use env. (Upstream PR has been submitted here: https://gerrit.libreoffice.org/#/c/49097/) https://github.com/LorbusChris/libreoffice-online-rpm/blob/master/libreoffice-online.spec Unfortunately there's still quite a few nodejs deps missing, especially browserify, which by itself already has lots of unpackackged deps.. I've begun to package them and will create individual Review Requests for them. Could be a little while.
The %post section IMHO looks very dubious, my thoughts and suggestions: - http://rpm.org/wiki/Releases/4.7.0#POSIX.1edraft15filecapabilities says there is %caps() meanwhile, thus no need for direct setcap calls, I would guess. - Why is %{_localstatedir}/cache/loolwsd created and owned rather using %attr(750, lool, lool) %{_localstatedir}/cache/loolwsd in %files? - Why is %{_localstatedir}/cache/loolwsd/* removed in %post? That maybe should be documented inside of the spec file...if really needed. - Why does loolwsd need a directory on the same filesystem like LibreOffice? And shouldn't something like this (if really needed) either go to a systemd unit file as ExecStartPre or just get part of %files? There is no documented reason why it has to be on the same filesystem...or? Could you add bugzilla "depends on" for the missing nodejs dependencies (if there are meanwhile individual review requests) so that somebody can easily see which dependencies are missing?
Please consider to move: CXXFLAGS="%{optflags} -Wno-error" from [1] to %configure. [1] https://github.com/LorbusChris/libreoffice-online-rpm/blob/master/libreoffice-online.spec#L46
This is an automatic check from review-stats script. This review request ticket hasn't been updated for some time. We're sorry it is taking so long. If you're still interested in packaging this software into Fedora repositories, please respond to this comment clearing the NEEDINFO flag. You may want to update the specfile and the src.rpm to the latest version available and to propose a review swap on Fedora devel mailing list to increase chances to have your package reviewed. If this is your first package and you need a sponsor, you may want to post some informal reviews. Read more at https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group. Without any reply, this request will shortly be considered abandoned and will be closed. Thank you for your patience.