Spec URL: https://kalev.fedorapeople.org/snapshot.spec SRPM URL: https://kalev.fedorapeople.org/snapshot-45.0-1.fc40.src.rpm Description: Take pictures and videos on your computer, tablet, or phone. Fedora Account System Username: kalev Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=106922813
I'm slightly unsure how to best handle the aperture crate. It's developed in-tree, and referenced as 'path = "./aperture"' in Cargo.toml. Perhaps it would make sense to package it separately, but for now I've gone with including it in the same rpm. Opinions welcome.
Quick comments before I do a full review: > I'm slightly unsure how to best handle the aperture crate. It's developed > in-tree, and referenced as 'path = "./aperture"' in Cargo.toml. Perhaps it > would make sense to package it separately, but for now I've gone with > including it in the same rpm. Opinions welcome. I see the "aperture" crate is also published separately: https://crates.io/crates/aperture There are two options for handling this case: - use the version bundled in snapshot - package from crates.io, replace "path" dependency with a "normal" dependency So long as snapshot is the only user of the "aperture" crate, I think both are OK. But as soon as more things start depending on it, I would try to unbundle it. ------------------------------------------------------------ > %global tarball_version %%(echo %{version} | tr '~' '.') I think you could replace this with "%{version_no_tilde .}": """ $ rpm --define "version 45.0~rc1" -E "%{version_no_tilde .}" 45.0.rc1 """ ------------------------------------------------------------ What is the license of snapshot itself? It might be good to add either SourceLicense: GPL-3.0-or-later or a comment like # snapshot: GPL-3.0-or-later # crate dependencies: # <... list crate licenses> ------------------------------------------------------------ > Provides: bundled(rust-aperture) = 0.3.1 This should be in a slightly different format to match other Provides for Rust crates: Provides: bundled(crate(aperture)) = 0.3.1 ------------------------------------------------------------ You mentioned on Matrix that you needed gstreamer1-plugin-gtk4 for this application, but I don't see it mentioned in the spec file? Should it be a dependency (Requires: gstreamer1-plugin-gtk4), or is it somehow added automatically?
(In reply to Fabio Valentini from comment #2) > Quick comments before I do a full review: > > > I'm slightly unsure how to best handle the aperture crate. It's developed > > in-tree, and referenced as 'path = "./aperture"' in Cargo.toml. Perhaps it > > would make sense to package it separately, but for now I've gone with > > including it in the same rpm. Opinions welcome. > > I see the "aperture" crate is also published separately: > https://crates.io/crates/aperture Right, that's why I brought this up. > There are two options for handling this case: > - use the version bundled in snapshot > - package from crates.io, replace "path" dependency with a "normal" > dependency > > So long as snapshot is the only user of the "aperture" crate, I think both > are OK. > But as soon as more things start depending on it, I would try to unbundle it. I think I'll go with the bundled version for now as I have that ready, and discuss with upstream what they think is the best way to handle it. Another aspect is that it looks like the aperture crate is getting updates more often than the app itself: aperture 0.3.2 was released without a corresponding snapshot release, so unbundling would help us get updates faster. (0.3.2 was a tiny unimportant release, but still.) > > %global tarball_version %%(echo %{version} | tr '~' '.') > > I think you could replace this with "%{version_no_tilde .}": > > """ > $ rpm --define "version 45.0~rc1" -E "%{version_no_tilde .}" > 45.0.rc1 > """ I'd like to stick with the current way for now as that's how it's done in all other GNOME packages, but this definitely could use some overhaul. I've been thinking of adding gnome-srpm-macros which would contain macros for constructing the source url for download.gnome.org and handling the ~ and . translations in version strings. > What is the license of snapshot itself? It might be good to add either > SourceLicense: GPL-3.0-or-later > > or a comment like > # snapshot: GPL-3.0-or-later > # crate dependencies: > # <... list crate licenses> Oh, good idea! I added both :) > > Provides: bundled(rust-aperture) = 0.3.1 > > This should be in a slightly different format to match other Provides for > Rust crates: > > Provides: bundled(crate(aperture)) = 0.3.1 Fixed. > You mentioned on Matrix that you needed gstreamer1-plugin-gtk4 for this > application, but I don't see it mentioned in the spec file? > Should it be a dependency (Requires: gstreamer1-plugin-gtk4), or is it > somehow added automatically? It just uses the rust crate directly and doesn't go through the plugin interface, if I understand it right. I may be wrong of course :) I'm having some trouble reading rust code. So it just needs it as a build dep, which is picked up automatically thanks to the %cargo_generate_buildrequires. Spec URL: https://kalev.fedorapeople.org/snapshot.spec SRPM URL: https://kalev.fedorapeople.org/snapshot-45.0-1.fc40.src.rpm Diff to the the previous version: https://paste.centos.org/view/74fd2061 Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=106960088
Err, that should have been: Spec URL: https://kalev.fedorapeople.org/snapshot.spec SRPM URL: https://kalev.fedorapeople.org/snapshot-45.0-2.fc40.src.rpm
(In reply to Kalev Lember from comment #3) > I think I'll go with the bundled version for now as I have that ready, and > discuss with upstream what they think is the best way to handle it. > > Another aspect is that it looks like the aperture crate is getting updates > more often than the app itself: aperture 0.3.2 was released without a > corresponding snapshot release, so unbundling would help us get updates > faster. (0.3.2 was a tiny unimportant release, but still.) Sounds good to me. This can be resolved after the package is imported. > > > %global tarball_version %%(echo %{version} | tr '~' '.') > > > > I think you could replace this with "%{version_no_tilde .}": > > > > """ > > $ rpm --define "version 45.0~rc1" -E "%{version_no_tilde .}" > > 45.0.rc1 > > """ > > I'd like to stick with the current way for now as that's how it's done in > all other GNOME packages, but this definitely could use some overhaul. I've > been thinking of adding gnome-srpm-macros which would contain macros for > constructing the source url for download.gnome.org and handling the ~ and . > translations in version strings. Sure, that works as well. :) > > What is the license of snapshot itself? It might be good to add either > > SourceLicense: GPL-3.0-or-later > > > > or a comment like > > # snapshot: GPL-3.0-or-later > > # crate dependencies: > > # <... list crate licenses> > > Oh, good idea! I added both :) :thumbsup: > > > Provides: bundled(rust-aperture) = 0.3.1 > > > > This should be in a slightly different format to match other Provides for > > Rust crates: > > > > Provides: bundled(crate(aperture)) = 0.3.1 > > Fixed. :thumbsup: > > You mentioned on Matrix that you needed gstreamer1-plugin-gtk4 for this > > application, but I don't see it mentioned in the spec file? > > Should it be a dependency (Requires: gstreamer1-plugin-gtk4), or is it > > somehow added automatically? > > It just uses the rust crate directly and doesn't go through the plugin > interface, if I understand it right. I may be wrong of course :) I'm having > some trouble reading rust code. > > So it just needs it as a build dep, which is picked up automatically thanks > to the %cargo_generate_buildrequires. Ah ... that makes sense. Somewhat. Thanks for the clarification. I'll do the full review now with the latest version of the spec / SRPM.
Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed ===== MUST items ===== C/C++: [-]: Provides: bundled(gnulib) in place as required. [x]: Package does not contain kernel modules. [x]: Package does not contain any libtool archives (.la) [x]: Package contains no static executables. [x]: Rpath absent or only used for internal libs. Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: License field in the package spec file matches the actual license. [x]: License file installed when any subpackage combination is installed. [x]: %build honors applicable compiler flags or justifies otherwise. [~]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [x]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: The spec file handles locales properly. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [x]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %license. [x]: The License field must be a valid SPDX expression. [x]: Package requires other packages for directories it uses. [x]: Package must own all directories that it creates. [x]: Package does not own files or directories owned by other packages. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Package contains desktop file if it is a GUI application. [x]: Package installs a %{name}.desktop using desktop-file-install or desktop-file-validate if there is such a file. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package must not depend on deprecated() packages. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package does not use a name that already exists. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [-]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Final provides and requires are sane (see attachments). [?]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [x]: Patches link to upstream bugs/comments/lists or are otherwise justified. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. [x]: Package should compile and build into binary rpms on all supported architectures. [x]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [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]: Fully versioned dependency in subpackages if applicable. [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]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [!]: Spec file according to URL is the same as in SRPM. [x]: Rpmlint is run on debuginfo package(s). [x]: Rpmlint is run on all installed packages. [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. Rpmlint ------- Checking: snapshot-45.0-2.fc40.x86_64.rpm snapshot-debuginfo-45.0-2.fc40.x86_64.rpm snapshot-debugsource-45.0-2.fc40.x86_64.rpm snapshot-45.0-2.fc40.src.rpm ============================ rpmlint session starts ============================ rpmlint: 2.4.0 configuration: /usr/lib/python3.12/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/tmpy9z7ir10')] checks: 31, packages: 4 snapshot.src: W: strange-permission snapshot.spec 600 snapshot.x86_64: W: no-manual-page-for-binary snapshot 4 packages and 0 specfiles checked; 0 errors, 2 warnings, 0 badness; has taken 0.8 s Rpmlint (debuginfo) ------------------- Checking: snapshot-debuginfo-45.0-2.fc40.x86_64.rpm ============================ rpmlint session starts ============================ rpmlint: 2.4.0 configuration: /usr/lib/python3.12/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/tmpqq8pg_v3')] checks: 31, packages: 1 1 packages and 0 specfiles checked; 0 errors, 0 warnings, 0 badness; has taken 0.3 s Rpmlint (installed packages) ---------------------------- ============================ rpmlint session starts ============================ rpmlint: 2.4.0 configuration: /usr/lib/python3.12/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 checks: 31, packages: 3 snapshot.x86_64: W: no-manual-page-for-binary snapshot 3 packages and 0 specfiles checked; 0 errors, 1 warnings, 0 badness; has taken 0.3 s Source checksums ---------------- https://download.gnome.org/sources/snapshot/45/snapshot-45.0.tar.xz : CHECKSUM(SHA256) this package : ee478ee090731a0b0824b66bb1746bd800dcbfe87ac8358499449af39cfcdb67 CHECKSUM(SHA256) upstream package : ee478ee090731a0b0824b66bb1746bd800dcbfe87ac8358499449af39cfcdb67 Requires -------- snapshot (rpmlib, GLIBC filtered): gstreamer1-plugins-bad-free(x86-64) hicolor-icon-theme libadwaita-1.so.0()(64bit) libadwaita-1.so.0(LIBADWAITA_1_0)(64bit) libc.so.6()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libgcc_s.so.1(GCC_3.3)(64bit) libgcc_s.so.1(GCC_4.2.0)(64bit) libgio-2.0.so.0()(64bit) libglib-2.0.so.0()(64bit) libgobject-2.0.so.0()(64bit) libgraphene-1.0.so.0()(64bit) libgstgl-1.0.so.0()(64bit) libgstpbutils-1.0.so.0()(64bit) libgstreamer-1.0.so.0()(64bit) libgstvideo-1.0.so.0()(64bit) libgtk-4.so.1()(64bit) libm.so.6()(64bit) rtld(GNU_HASH) snapshot-debuginfo (rpmlib, GLIBC filtered): snapshot-debugsource (rpmlib, GLIBC filtered): Provides -------- snapshot: application() application(org.gnome.Snapshot.desktop) bundled(crate(aperture)) metainfo() metainfo(org.gnome.Snapshot.metainfo.xml) snapshot snapshot(x86-64) snapshot-debuginfo: debuginfo(build-id) snapshot-debuginfo snapshot-debuginfo(x86-64) snapshot-debugsource: snapshot-debugsource snapshot-debugsource(x86-64) Generated by fedora-review 0.10.0 (e79b66b) last change: 2023-07-24 Command line :/usr/bin/fedora-review -b 2241553 -o --enablerepo local Buildroot used: fedora-rawhide-x86_64 Active plugins: C/C++, Generic, Shell-api Disabled plugins: Java, Python, Haskell, Perl, PHP, SugarActivity, R, fonts, Ocaml Disabled flags: EXARCH, EPEL6, EPEL7, DISTTAG, BATCH ================================================================================ Package APPROVED.
Thanks a lot for the review, Fabio!
The Pagure repository was created at https://src.fedoraproject.org/rpms/snapshot
FEDORA-2023-cd720eda86 has been submitted as an update to Fedora 40. https://bodhi.fedoraproject.org/updates/FEDORA-2023-cd720eda86
FEDORA-2023-cd720eda86 has been pushed to the Fedora 40 stable repository. If problem still persists, please make note of it in this bug report.