Spec URL: https://kalev.fedorapeople.org/papers.spec SRPM URL: https://kalev.fedorapeople.org/papers-47~beta-1.fc42.src.rpm Description: Papers is a document viewer for multiple document formats for GNOME. Fedora Account System Username: kalev
Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=122183114 The package is a mix of C code and rust code. Thanks to Fabio Valentini for updating the rust gtk4 stack over the last few days - this made the rust side of papers packaging very straightforward. I've based the packaging here on evince packaging (papers is a fork of evince), but tried to clean it up and simplify as much as possible. The rust side of the packaging tries to use the same patterns as other new GNOME rust-based apps - e.g. loupe, snapshot. For now I've left out eln support to simplify the review. I'll work on eln conditionals once the package is accepted into Fedora.
Are the docs and introspection not built on purpose? Presumably, the introspection will be used by things currently using evince-libs (e.g. sushi) when they port to gtk4. Also, the new `%bcond check 1` syntax is preferred.
Thanks for taking the review! Yes, docs and introspection are deliberately disabled - introspection needs a new dep, https://github.com/gtk-rs/gir that we don't have in Fedora yet, and there's also nothing that makes use of the introspection bindings yet. I've left this as an exercise for later once we have a consumer. Developer docs are disabled because introspection is disabled - it needs introspection for building docs. As for %bcond syntax - I've used the same pattern that rust2rpm uses and I'd like to keep it the same just so that it's consistent with generated rust packages. Beyond that, I like the new syntax better as well :)
Issues: ======= - If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. Note: No gcc, gcc-c++ or clang found in BuildRequires See: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/ - Sources used to build the package match the upstream source, as provided in the spec URL. Note: Upstream MD5sum check error, diff is in /home/yselkowi/tmp/2305882-papers/diff.txt See: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/
Also, please filter provides: https://docs.fedoraproject.org/en-US/packaging-guidelines/AutoProvidesAndRequiresFiltering/#_preventing_filesdirectories_from_being_scanned_for_deps_pre_scan_filtering
Thanks, I added gcc buildrequires and provides filtering. Can you explain please what issue do you see with the sources not matching? /home/yselkowi/tmp/2305882-papers/diff.txt is not publicly available. Spec URL: https://kalev.fedorapeople.org/papers.spec SRPM URL: https://kalev.fedorapeople.org/papers-47~beta-2.fc42.src.rpm Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=122700307 * Fri Aug 30 2024 Kalev Lember <klember> - 47~beta-2 - Package review fixes (rhbz#2305882) - Explicitly BR gcc - Filter out soname provides for plugins - Drop all references to gi-docgen as we don't currently install documentation
Copr build: https://copr.fedorainfracloud.org/coprs/build/7957424 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2305882-papers/fedora-rawhide-x86_64/07957424-papers/fedora-review/review.txt Found issues: - License file license.page is not marked as %license Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text Please know that there can be false-positives. --- 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.
WRT the source tarball, I wonder if you were using a manually generated git snapshot, or if they repushed the tarball upstream? Either way, that seems to be fixed in your latest SRPM. 1) The mjw locale has no directory ownership. AFAICS the reason is that locale is in ISO-639-3 but not in -2. Please file a bug on filesystem to fix that. 2) The /usr/share/help/$LOCALE directories have no ownership. filesystem provides /usr/share/help but no subdirectories. Please file another bug on filesystem to fix that. 3) evince-libs is still needed for some packages (e.g. sushi), but should/will there be an automatic upgrade path for evince->papers and the same for -previewer (probably depends on gtk3/4 being updated) and -thumbnailer (it seems silly to have both)? (In such a case, evince itself should no longer require those subpackages.) "License file license.page is not marked as %license" is a false positive.
> WRT the source tarball, I wonder if you were using a manually generated git snapshot, or if they repushed the tarball upstream? Either way, that seems to be fixed in your latest SRPM. No, it's the exact same tarball that's in both papers-47~beta-1.fc42.src.rpm and papers-47~beta-2.fc42.src.rpm, and matches the upstream tarball. I just double checked all the links above and it's the same tarball in all cases, so I strongly suspect it's something that was wrong on your end - but I'm glad it seems fixed now. > 1) The mjw locale has no directory ownership. AFAICS the reason is that locale is in ISO-639-3 but not in -2. Please file a bug on filesystem to fix that. Sure, here you go: https://bugzilla.redhat.com/show_bug.cgi?id=2311691 > 2) The /usr/share/help/$LOCALE directories have no ownership. filesystem provides /usr/share/help but no subdirectories. Please file another bug on filesystem to fix that. There's already https://bugzilla.redhat.com/show_bug.cgi?id=2281584 for this. > 3) evince-libs is still needed for some packages (e.g. sushi), but should/will there be an automatic upgrade path for evince->papers and the same for -previewer (probably depends on gtk3/4 being updated) and -thumbnailer (it seems silly to have both)? (In such a case, evince itself should no longer require those subpackages.) There are all good questions, and we'll need to figure out how to do all this as soon as Workstation wants to switch from evince to papers. For now however it's just a new package that is not going to be installed by default and I don't think it makes sense to add any kind of automatic upgrade path handling.
Thanks; as the directory ownership issues properly belong in filesystem, I don't think they should block this. License needs a fix: -License: GPL-2.0-or-later AND GPL-3.0-or-later AND LGPL-2.0-or-later AND LGPL-2.1-or-later AND MIT AND libtiff AND (MIT OR Apache-2.0) AND Unicode-DFS-2016 AND (Apache-2.0 OR MIT) AND (Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT) AND (BSD-2-Clause OR Apache-2.0 OR MIT) AND (MIT OR Apache-2.0) AND (Unlicense OR MIT) +License: GPL-2.0-or-later AND GPL-3.0-or-later AND LGPL-2.0-or-later AND LGPL-2.1-or-later AND MIT AND libtiff AND ((MIT OR Apache-2.0) AND Unicode-DFS-2016) AND (Apache-2.0 OR MIT) AND (Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT) AND (BSD-2-Clause OR Apache-2.0 OR MIT) AND (Unlicense OR MIT) Only `%meson_test` should be conditional on `%if %{with check}`, but the desktop-file-validate and appstream-util commands should be run regardless. Also, if I'm not mistaken, all the .metainfo.xml files need to be verified, not just the app's. In `%files libs`, this is safer and conformant with the guidelines (e.g. if .so.5 were to suddenly become .so.50 or .so.500): -%{_libdir}/libppsdocument-4.0.so.5* -%{_libdir}/libppsview-4.0.so.4* +%{_libdir}/libppsdocument-4.0.so.5{,.*} +%{_libdir}/libppsview-4.0.so.4{,.*}
> Only `%meson_test` should be conditional on `%if %{with check}`, but the desktop-file-validate and appstream-util commands should be run regardless. Also, if I'm not mistaken, all the .metainfo.xml files need to be verified, not just the app's. Why do you think so? For me, when I see a conditional that reads `%if %{with check}`, that sounds like something that would disable the whole check section, not just some part of it. I've applied the rest of the suggestions - thanks! Spec URL: https://kalev.fedorapeople.org/papers.spec SRPM URL: https://kalev.fedorapeople.org/papers-47~beta-5.fc42.src.rpm Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=123303870 * Thu Sep 12 2024 Kalev Lember <klember> - 47~beta-5 - Use globs to ensure all desktop and metainfo files get validated * Thu Sep 12 2024 Kalev Lember <klember> - 47~beta-4 - Tighten soname globs * Thu Sep 12 2024 Kalev Lember <klember> - 47~beta-3 - Remove duplicate '(MIT OR Apache-2.0)' from license tag
Created attachment 2046590 [details] The .spec file difference from Copr build 7957424 to 8014900
Copr build: https://copr.fedorainfracloud.org/coprs/build/8014900 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2305882-papers/fedora-rawhide-x86_64/08014900-papers/fedora-review/review.txt Found issues: - License file license.page is not marked as %license Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text - Upstream MD5sum check error, diff is in /var/lib/copr-rpmbuild/results/papers/diff.txt Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/ Please know that there can be false-positives. --- 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 Kalev Lember from comment #11) > > Only `%meson_test` should be conditional on `%if %{with check}`, but the desktop-file-validate and appstream-util commands should be run regardless. Also, if I'm not mistaken, all the .metainfo.xml files need to be verified, not just the app's. > > Why do you think so? For me, when I see a conditional that reads `%if %{with > check}`, that sounds like something that would disable the whole check > section, not just some part of it. * Tests are usually disabled because of extra dependencies, flakiness, or other requirements that cannot be fulfilled (e.g. network access). None of those reasons apply to these validations. * Validation of these files can occur in either %install or %check. * Validation of these files is a MUST: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_desktop_file_install_usage https://docs.fedoraproject.org/en-US/packaging-guidelines/AppData/ * The conditional can be named "tests" instead of "check" if you feel that is less ambiguous; tests may be more common anyway (outside of Rust crates).
I think you misunderstand a bit what's going on here. First, the %check bcond is used when generating rust crate buildrequires and it needs to be there so that the buildrequires generator correctly pulls in rust crates that are required for tests. I assume you have not seen this before because you've worked in RHEL context where rust dependencies are bundled and cargo depgen is not used. The bcond has to be named 'check' because of that. Second, the %check bcond wrapping the %check section is common pattern in rust crates. I assume you've not seen this either because in RHEL the crates are bundled. See any random rust- package, e.g. https://src.fedoraproject.org/rpms/rust-gst-plugin-reqwest/blob/rawhide/f/rust-gst-plugin-reqwest.spec Third, the %check bcond as it is now in the spec file is *enabled* and it is only there to be able to build tests - I have no intention of disabling it. Again, it is there to be *able* to run tests, not to disable them. All the guidelines mentions you have above are not at all relevant because these are all run as per the guidelines. Fourth, in the case of a hypothetical bootstrapping of a new architecture (think RISC V) it can be beneficial to have a way to disable self tests for initial bootstrapping. Completely wrapping the %check section in the %check bcond makes this easily possible. Again, this is not for main Fedora - it just makes it easier for other parties.
A fly-by comment from someone who knows absolutely nothing about Rust or how to package anything written in it. (In reply to Kalev Lember from comment #15) > Second, the %check bcond wrapping the %check section is common pattern in > rust crates. I assume you've not seen this either because in RHEL the crates > are bundled. See any random rust- package, e.g. > https://src.fedoraproject.org/rpms/rust-gst-plugin-reqwest/blob/rawhide/f/ > rust-gst-plugin-reqwest.spec This is also the pattern that go2rpm uses. For example, here are some RPMs that I recently added to Fedora: https://src.fedoraproject.org/rpms/golang-github-nvidia-nvml/blob/rawhide/f/golang-github-nvidia-nvml.spec https://src.fedoraproject.org/rpms/golang-github-nvidia-nvlib/blob/rawhide/f/golang-github-nvidia-nvlib.spec https://src.fedoraproject.org/rpms/golang-github-nvidia-container-toolkit/blob/rawhide/f/golang-github-nvidia-container-toolkit.spec https://src.fedoraproject.org/rpms/golang-tags-cncf-container-device-interface/blob/rawhide/f/golang-tags-cncf-container-device-interface.spec
Are you still interested in finishing the review, Yaakov?
Resetting the reviewer as per the "Reviewer not responding" section in the package review policy, https://docs.fedoraproject.org/en-US/fesco/Package_review_policy/#reviewer_not_responding
I was away on holidays for a few weeks. This needs an update to 47.0 before it can be reviewed again.
I wonder if you could let someone who is more familiar with rust packaging to review it instead? I don't find the back and forth here particularly helpful - I'm basically having to defend rust packaging best practices. As for updating to 47.0, there's a new build error when I try to build it: ``` error[E0599]: the method `set` exists for struct `RefCell<Option<DocumentModel>>`, but its trait bounds were not satisfied --> src/sidebar.rs:104:24 | 104 | self.model.set(Some(model)); | ^^^ | = note: the following trait bounds were not satisfied: `std::cell::RefCell<std::option::Option<papers_view::DocumentModel>>: glib::object::IsA<gio::Settings>` which is required by `std::cell::RefCell<std::option::Option<papers_view::DocumentModel>>: gdk4::prelude::SettingsExtManual` = help: items from traits can only be used if the trait is in scope help: trait `PropertySet` which provides `set` is implemented but not in scope; perhaps you want to import it | 4 + use glib::property::PropertySet; | ``` If anyone has ideas about the error above, that would be super helpful. I don't think this should block finishing the package review on this package, however, and the build issue can be solved separately after we get the initial package (47.beta) in.
The compiler error should give you all that you need to fix this issue: 1. error[E0599]: the method `set` exists for struct `RefCell<Option<DocumentModel>>`, but its trait bounds were not satisfied 2. help: trait `PropertySet` which provides `set` is implemented but not in scope; perhaps you want to import it: use glib::property::PropertySet; This looks like an issue in upstream code.
Thanks, Fabio! I am not convinced it's the right fix here though: it seems like it's leaking an implementation detail if DocumentModel users need to specify an additional import to be able to use it. I did some digging and it looks like the reason we are running into the build issue here and upstream is not is because glib-macros in Fedora is newer than what's in upstream Cargo.lock. In particular, it appears that this commit is the cause of all this: https://github.com/gtk-rs/gtk-rs-core/commit/795e2c5d25d45442c1f77ddabda21370365ccc45 I have still no clue what the right fix should be or where, but at least it's a bit clearer what's causing it :) I'll file an issue upstream.
https://gitlab.gnome.org/GNOME/Incubator/papers/-/issues/278
OK, I posted a fix for this upstream and here's the patch backported, together with updating to 47.0: Spec URL: https://kalev.fedorapeople.org/papers.spec SRPM URL: https://kalev.fedorapeople.org/papers-47.0-1.fc42.src.rpm Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=125529207 * Tue Nov 05 2024 Kalev Lember <klember> - 47.0-1 - Update to 47.0 - Fix the build with glib-macros 0.20.3
Created attachment 2055787 [details] The .spec file difference from Copr build 8014900 to 8213401
Copr build: https://copr.fedorainfracloud.org/coprs/build/8213401 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2305882-papers/fedora-rawhide-x86_64/08213401-papers/fedora-review/review.txt Found issues: - License file license.page is not marked as %license Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text Please know that there can be false-positives. --- 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.
License still needs the aforementioned fix: -License: GPL-2.0-or-later AND GPL-3.0-or-later AND LGPL-2.0-or-later AND LGPL-2.1-or-later AND MIT AND libtiff AND (MIT OR Apache-2.0) AND Unicode-DFS-2016 AND (Apache-2.0 OR MIT) AND (Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT) AND (BSD-2-Clause OR Apache-2.0 OR MIT) AND (MIT OR Apache-2.0) AND (Unlicense OR MIT) +License: GPL-2.0-or-later AND GPL-3.0-or-later AND LGPL-2.0-or-later AND LGPL-2.1-or-later AND MIT AND libtiff AND ((MIT OR Apache-2.0) AND Unicode-DFS-2016) AND (Apache-2.0 OR MIT) AND (Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT) AND (BSD-2-Clause OR Apache-2.0 OR MIT) AND (Unlicense OR MIT) That, and moving the desktop/appstream validation out of a conditional, and I think this looks good now.
Can you explain why you want to change the license and what is exactly changing in there? It's super hard to see in a long string like that. Thanks. As for desktop/appstream validation, I explained previously that it is deliberately in the conditional. Please see https://bugzilla.redhat.com/show_bug.cgi?id=2305882#c15
There needs to be an extra grouping around ((MIT OR Apache-2.0) AND Unicode-DFS-2016) since the entire statement is the license of a rust dependency. There is nothing about rust packaging guidelines that covers desktop/appstream validation. According to the guidelines, they can go in either %check or %install; choosing the latter would avoid this issue entirely.
I'll take a look.
Some quick first impressions: (In reply to Yaakov Selkowitz from comment #29) > There needs to be an extra grouping around ((MIT OR Apache-2.0) AND > Unicode-DFS-2016) since the entire statement is the license of a rust > dependency. This is not true. In SPDX expressions, "A AND (B AND C)" and "(A AND B) AND C" are equivalent, so you can drop parentheses around multiple clauses like that -> "A AND B AND C" (i.e. the SPDX "AND" operator is associative - and commutative, for that matter, so you can reorder freely, too). > There is nothing about rust packaging guidelines that covers > desktop/appstream validation. According to the guidelines, they can go in > either %check or %install; choosing the latter would avoid this issue > entirely. In this case, for simplicity, I would recommend to remove the %check bcond from the spec entirely, and use `%cargo_generate_buildrequires -a -t` explicitly. That avoids the whole issue on an even higher level. :) I'll do a full review later.
(In reply to Fabio Valentini from comment #31) > Some quick first impressions: > > (In reply to Yaakov Selkowitz from comment #29) > > There needs to be an extra grouping around ((MIT OR Apache-2.0) AND > > Unicode-DFS-2016) since the entire statement is the license of a rust > > dependency. > > This is not true. In SPDX expressions, "A AND (B AND C)" and "(A AND B) AND > C" are equivalent, so you can drop parentheses around multiple clauses like > that -> "A AND B AND C" (i.e. the SPDX "AND" operator is associative - and > commutative, for that matter, so you can reorder freely, too). Indeed, and see also https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/thread/F4MYD7U6D2ROAL3CAOHSYDL3H6TPWZOT/ that discusses a very similar situation and the conclusion is to simplify it as per above. > > There is nothing about rust packaging guidelines that covers > > desktop/appstream validation. According to the guidelines, they can go in > > either %check or %install; choosing the latter would avoid this issue > > entirely. > > In this case, for simplicity, I would recommend to remove the %check bcond > from the spec entirely, and use `%cargo_generate_buildrequires -a -t` > explicitly. That avoids the whole issue on an even higher level. :) Ah, good idea! Let me change that.
Spec URL: https://kalev.fedorapeople.org/papers.spec SRPM URL: https://kalev.fedorapeople.org/papers-47.0-2.fc42.src.rpm Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=125535067 * Tue Nov 05 2024 Kalev Lember <klember> - 47.0-2 - Remove the %check bcond and use %cargo_generate_buildrequires -t explicitly
If you're not going to group the first two clauses of "(MIT OR Apache-2.0) AND Unicode-DFS-2016 AND (Apache-2.0 OR MIT)" then the first and third of those clauses are duplicates.
Indeed - good eye. I've removed (Apache-2.0 OR MIT) in favour of (MIT OR Apache-2.0) now. Spec URL: https://kalev.fedorapeople.org/papers.spec SRPM URL: https://kalev.fedorapeople.org/papers-47.0-3.fc42.src.rpm
Copr build: https://copr.fedorainfracloud.org/coprs/build/8216361 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2305882-papers/fedora-rawhide-x86_64/08216361-papers/fedora-review/review.txt Found issues: - License file license.page is not marked as %license Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text Please know that there can be false-positives. --- 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.
Created attachment 2055814 [details] The .spec file difference from Copr build 8216361 to 8216390
Copr build: https://copr.fedorainfracloud.org/coprs/build/8216390 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2305882-papers/fedora-rawhide-x86_64/08216390-papers/fedora-review/review.txt Found issues: - License file license.page is not marked as %license Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuidelines/#_license_text Please know that there can be false-positives. --- 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.
I think this looks good now, but Fabio has taken the review.
That's fine, I asked Fabio to take the review and help finish this.
In general, the package looks good to me. Just some minor things that I'd like to have clarified and / or fixed: > https://fedoraproject.org/wiki/Changes/EncourageI686LeafRemoval This is not really applicable - it's not a removal, this package is just not going to be built for i686 from the start. You wouldn't need to document or justify this ExcludeArch at all, but it doesn't hurt to include the link, I guess. > # Filter out soname provides for plugins > %global __provides_exclude_from ^%{_libdir}/papers/.*\\.so$ Please put this at the top of the spec file. Putting %globals in the middle of the spec file makes them really hard to find, and it might even get lost (i.e. become part of the %description, if it were just a few lines later). > -Dintrospection=disabled I seem to recall that some Rust-written tool is missing to get this enabled? Is this on your roadmap, or should we put that on the Rust SIG wishlist? > %{_libdir}/libppsshell-4.0.so.4{,.*} > %{_libdir}/libppsdocument-4.0.so.5{,.*} > %{_libdir}/libppsview-4.0.so.4{,.*} Is it intentional that some libraries are in the "main" papers package while the other two are in the "-libs" subpackage? > %{_datadir}/thumbnailers/ It looks like nothing owns this directory? I'm not sure whether I'm using DNF5 correctly, but its repoquery gives me zero results. If that is the case, this package will need to co-own the /usr/share/thumbnailers/ directory.
(In reply to Fabio Valentini from comment #41) > In general, the package looks good to me. Just some minor things that I'd > like to have clarified and / or fixed: > > > https://fedoraproject.org/wiki/Changes/EncourageI686LeafRemoval > > This is not really applicable - it's not a removal, this package is just not > going to be built for i686 from the start. > You wouldn't need to document or justify this ExcludeArch at all, but it > doesn't hurt to include the link, I guess. I think I'd slightly prefer to have some kind of justification in case someone reads the spec file and wonders what is going on, but I'd be OK to removing it as well if you feel strongly about it. > > # Filter out soname provides for plugins > > %global __provides_exclude_from ^%{_libdir}/papers/.*\\.so$ > > Please put this at the top of the spec file. Putting %globals in the middle > of the spec file makes them really hard to find, and it might even get lost > (i.e. become part of the %description, if it were just a few lines later). Sure, let me move it, good idea. > > -Dintrospection=disabled > > I seem to recall that some Rust-written tool is missing to get this enabled? > Is this on your roadmap, or should we put that on the Rust SIG wishlist? Yes, the tool is called 'gir' and comes from https://github.com/gtk-rs/gir I think it would be good to get it packaged, but I haven't looked at it at all so far. We'll need to enable introspection once something starts using the library through introspection - a python app, for example. For now, I am not aware of anything but I'm sure something is going to appear sooner or later that wants to embed pdf viewing. If I remember right, self tests also depended on introspection, which is why they are disabled with -Dtests=false. Would be nice for it to be on the Rust SIG wishlist! > > %{_libdir}/libppsshell-4.0.so.4{,.*} > > %{_libdir}/libppsdocument-4.0.so.5{,.*} > > %{_libdir}/libppsview-4.0.so.4{,.*} > > Is it intentional that some libraries are in the "main" papers package while > the other two are in the "-libs" subpackage? It's been a while since I did the packaging, but as I recall, I tried to keep the -libs package size minimal and keep the libs that are only for the GUI app in the same subpackage as the GUI app. The idea being that this way, we don't install unnecessary libraries on a system that only ships -thumbnailer and -nautilus and -previewer subpackages. Silverblue for example would ship the app itself as a flatpak, but keep the system integration bits as rpms. I think this might deserve a comment in the spec file; let me add that. > > %{_datadir}/thumbnailers/ > > It looks like nothing owns this directory? > I'm not sure whether I'm using DNF5 correctly, but its repoquery gives me > zero results. > If that is the case, this package will need to co-own the > /usr/share/thumbnailers/ directory. Agreed - let me add the directory. I don't feel like figuring out the repoquery syntax for this right now, but on my F41 system there are a bunch of rpms that all co-own it: $ rpm -qf /usr/share/thumbnailers gdk-pixbuf2-2.42.12-6.fc41.x86_64 gnome-epub-thumbnailer-1.8-1.fc41.x86_64 rsvg-pixbuf-loader-2.59.1-1.fc41.x86_64 libgsf-1.14.53-1.fc41.x86_64 libjxl-0.10.3-5.fc41.x86_64
> I think I'd slightly prefer to have some kind of justification in case someone reads the spec file and wonders what is going on, but I'd be OK to removing it as well if you feel strongly about it. I don't feel strongly about it. It's just that this wasn't the kind of use I imagined for the Change Proposal when I filed it :) > Sure, let me move it, good idea. :thumbsup: > Would be nice for it to be on the Rust SIG wishlist! I'll add it. > I think this might deserve a comment in the spec file; let me add that. This is a good justification. I saw that the shared library that's in the "main" package also has no accompanying headers / unversioned so file, so it looks like it's internal to "the papers GUI". > Agreed - let me add the directory. I don't feel like figuring out the repoquery syntax for this right now, but on my F41 system there are a bunch of rpms that all co-own it: > $ rpm -qf /usr/share/thumbnailers > gdk-pixbuf2-2.42.12-6.fc41.x86_64 > gnome-epub-thumbnailer-1.8-1.fc41.x86_64 > rsvg-pixbuf-loader-2.59.1-1.fc41.x86_64 > libgsf-1.14.53-1.fc41.x86_64 > libjxl-0.10.3-5.fc41.x86_64 This is a better query than the one I had. :) But yes, it looks like the directory needs to be coowned.
Spec URL: https://kalev.fedorapeople.org/papers.spec SRPM URL: https://kalev.fedorapeople.org/papers-47.0-3.fc42.src.rpm Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=125569782 * Wed Nov 06 2024 Kalev Lember <klember> - 47.0-6 - Co-own /usr/share/thumbnailers directory * Wed Nov 06 2024 Kalev Lember <klember> - 47.0-5 - Add a spec file comment explaining why libppsshell is in the main package * Wed Nov 06 2024 Kalev Lember <klember> - 47.0-4 - Filter out soname provides for the nautilus extension - Move provides filtering to the top of the spec file
Thanks! I appreciate that you are thinking and looking at the package split. What do you think of the ``` cd shell-rs %cargo_generate_buildrequires -a -t cd ~- ``` hack that I did? cd ~- is to shut up cd so that it doesn't end up echoing lines in the generated build requires :) You haven't run across a cleaner way to do it, by any chance?
Thank you - package looks good to me now. I won't paste the full review template, but I think Yaakov and me went over everything more than once at this point. PACKAGE APPROVED. > What do you think of the > ``` > cd shell-rs > %cargo_generate_buildrequires -a -t > cd ~- > ``` > hack that I did? cd ~- is to shut up cd so that it doesn't end up echoing lines in the generated build requires :) You haven't run across a cleaner way to do it, by any chance? I don't think you even need to do "~-" - "cd .." would be enough. As far as I know, "cd" doesn't print to stdout at all and has no problems in %generate_buildrequires - maybe you're thinking of the problem caused by using pushd / popd?
Thanks for the review and the help here! > I don't think you even need to do "~-" - "cd .." would be enough. > > As far as I know, "cd" doesn't print to stdout at all and has no problems in %generate_buildrequires - maybe you're thinking of the problem caused by using pushd / popd? Yes, both pushd/popd and cd - (which is basically like popd, undoes last cd) print to stdout. 'cd ..' may be easier to read indeed and doesn't print. I'll ponder over this a bit.
The Pagure repository was created at https://src.fedoraproject.org/rpms/papers
FEDORA-2024-5e0c1d44d0 (papers-47.0-6.fc42) has been submitted as an update to Fedora 42. https://bodhi.fedoraproject.org/updates/FEDORA-2024-5e0c1d44d0
FEDORA-2024-5e0c1d44d0 (papers-47.0-6.fc42) has been pushed to the Fedora 42 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-2024-db4ad9762e (papers-47.0-6.fc41) has been submitted as an update to Fedora 41. https://bodhi.fedoraproject.org/updates/FEDORA-2024-db4ad9762e
FEDORA-2024-db4ad9762e has been pushed to the Fedora 41 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-db4ad9762e \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2024-db4ad9762e See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2024-db4ad9762e (papers-47.0-6.fc41) has been pushed to the Fedora 41 stable repository. If problem still persists, please make note of it in this bug report.