[Bug 2272140] New: Review Request: cosmic-term - Terminal emulator for the COSMIC desktop environment
https://bugzilla.redhat.com/show_bug.cgi?id=2272140
Bug ID: 2272140 Summary: Review Request: cosmic-term - Terminal emulator for the COSMIC desktop environment Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Assignee: nobody@fedoraproject.org Reporter: ryanbrue@hotmail.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://download.copr.fedorainfracloud.org/results/ryanabx/cosmic-epoch/fedo... SRPM URL: https://download.copr.fedorainfracloud.org/results/ryanabx/cosmic-epoch/fedo... Description: cosmic-term is a terminal emulator built for the COSMIC desktop environment Fedora Account System Username: ryanabx
https://bugzilla.redhat.com/show_bug.cgi?id=2272140
Ryan Brue ryanbrue@hotmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |2271980 | |(COSMICDesktopPackageReview | |s)
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=2271980 [Bug 2271980] Tracking: COSMIC Desktop related package review tracker
https://bugzilla.redhat.com/show_bug.cgi?id=2272140
--- Comment #1 from Ryan Brue ryanbrue@hotmail.com --- Spec URL: https://download.copr.fedorainfracloud.org/results/ryanabx/cosmic-epoch/fedo... SRPM URL: https://download.copr.fedorainfracloud.org/results/ryanabx/cosmic-epoch/fedo... Description: cosmic-term is a terminal emulator built for the COSMIC desktop environment Fedora Account System Username: ryanabx
https://bugzilla.redhat.com/show_bug.cgi?id=2272140
--- Comment #2 from Ryan Brue ryanbrue@hotmail.com --- Spec URL: https://download.copr.fedorainfracloud.org/results/ryanabx/cosmic-epoch/fedo... SRPM URL: https://download.copr.fedorainfracloud.org/results/ryanabx/cosmic-epoch/fedo... Description: cosmic-term is a terminal emulator built for the COSMIC desktop environment Fedora Account System Username: ryanabx
https://bugzilla.redhat.com/show_bug.cgi?id=2272140
Carl George 🤠 carl@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|nobody@fedoraproject.org |carl@redhat.com CC| |carl@redhat.com Doc Type|--- |If docs needed, set a value Flags| |fedora-review?
https://bugzilla.redhat.com/show_bug.cgi?id=2272140
--- Comment #3 from Ryan Brue ryanbrue@hotmail.com --- Spec URL: https://download.copr.fedorainfracloud.org/results/ryanabx/cosmic-epoch/fedo... SRPM URL: https://download.copr.fedorainfracloud.org/results/ryanabx/cosmic-epoch/fedo... Description: cosmic-term is a terminal emulator built for the COSMIC desktop environment Fedora Account System Username: ryanabx
https://bugzilla.redhat.com/show_bug.cgi?id=2272140
--- Comment #4 from Carl George 🤠 carl@redhat.com --- Sorry for the delay in getting to this review. Here are the improvements I was able to spot so far.
================================================================================
The cargo_install_lib macro is used to prevent %cargo_install from installing library sources. Since you aren't using %cargo_install, you can remove this macro definition.
================================================================================
Since you're using vendored dependencies, the licenses for your sources is likely the same for your binaries. In this scenario, you should just omit the SourceLicense tag entirely, as it's purpose is to indicate that the license of the sources differs from the binaries. I understand this was part of what rust2rpm generated, so I submitted an improvement to not do that in the future. For now we still need to remove it from this spec file.
https://pagure.io/fedora-rust/rust2rpm/pull-request/277
================================================================================
The package is installing a .desktop file, but isn't installing it with desktop-file-install or running desktop-file-validate on it during %check. Since this is being put in place by the justfile, the latter approach of validating would be a cleaner solution.
desktop-file-validate %{buildroot}%{_datadir}/applications/com.system76.CosmicTerm.desktop
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_desktop_file_ins...
================================================================================
Minor style thing, you are defining a ver macro, but it's only used in one place. I personally would avoid the macro and just use the value directly.
================================================================================
Your snapshot notation in the Version tag is not correct per the guidelines. You are correct to use a tilde to sort lower than 0.1.0 (in case upstream releases the first version as 0.1.0) but the snapshot information must also start with a caret. Your commitdate is also too long, it should be in the YYYYMMDD format. If you want to keep the date and SCM type, the date must come first. Alternatively, you can drop the date and just use an integer that you increment each time you do a new snapshot. Personally, I find the integer approach more appealing, since it gives you simple control of the sorting without overloading the version with extra information. The date and SCM type simply aren't relevant for upgrade path sorting. Here are some examples you could use that follow the guidelines (macros expanded for clarity).
0.1.0~^20240529.634a247 0.1.0~^20240529git634a247 0.1.0~^1.634a247 <-- my favorite for simplicity 0.1.0~^1.git634a247
https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_snaps...
================================================================================
You are using a mix of unnumbered and numbered sources. For predictability and to avoid accidentally reusing a number, I would stick with one style or the other.
================================================================================
You have a build requirement on git-core, but I don't see that used anywhere in the spec file, or in the upstream justfile. If this is just for creation of the vendor tarball, you can leave it out as that takes place outside of the build process.
================================================================================
%cargo_prep accepts a -v flag to specfiy the vendor directory, but it isn't being used here. I'm not sure how this is working without that, but I know the rust guidelines are fairly new so this might not be a problem.
https://docs.fedoraproject.org/en-US/packaging-guidelines/Rust/#_using_vendo...
================================================================================
It's probably overkill to list out every icon in %files. This would be an appropriate use of a wildcard, which will also prevent build failures if upstream adds more resolutions.
%{_datadir}/icons/hicolor/*/apps/com.system76.CosmicTerm.svg
It's also a bit weird that upstream is installing svgs in these directories. Most applications use pngs of the stated resolution, and place their svg in a scalable directory. This is probably worth bringing to upstream's attention, but isn't a blocker for the review.
================================================================================
Speaking of the icon files, they are resulting in unowned directories. These directories are provided by the hicolor-icon-theme package, so the easiest fix is to just add a requires on that package.
https://docs.fedoraproject.org/en-US/packaging-guidelines/UnownedDirectories...
================================================================================
One more optional stylistic tip, you're using the string com.system76.CosmicTerm quite a bit. That is something called reverse domain name notation. In my spec files I'll usually define an rdnn macro at the top of the spec file, then use %{rdnn} for each instance instead of typing out the full thing. That also helps if upstream ever changes that string (unlikely, but quite possible).
https://en.wikipedia.org/wiki/Reverse_domain_name_notation https://src.fedoraproject.org/rpms/butt/blob/rawhide/f/butt.spec
================================================================================
There are a few rpmlint errors/warnings that should be clearable.
cosmic-term.spec:76: W: macro-in-comment %{SOURCE2} cosmic-term.spec:80: W: macro-in-comment %{SOURCE2}
You can resolve these by using a double percent sign to "escape" the macro.
================================================================================
cosmic-term.x86_64: E: invalid-desktopfile /usr/share/applications/com.system76.CosmicTerm.desktop value "COSMIC;System;TerminalEmulator;" for key "Categories" in group "Desktop Entry" contains an unregistered value "COSMIC"; values extending the format should start with "X-"
It seems like you can fix this one by patching the .desktop file to change COSMIC to X-COSMIC. That would also be a good patch to send upstream.
================================================================================
cosmic-term.x86_64: W: unused-direct-shlib-dependency /usr/bin/cosmic-term /lib64/libxkbcommon.so.0 cosmic-term-debugsource.x86_64: E: files-duplicated-waste 841807
I actually don't know how to resolve these two for a rust application. It would be worth bringing up in the Fedora Rust Matrix channel to see if there is a known way to resolve them.
https://docs.fedoraproject.org/en-US/package-maintainers/CommonRpmlintIssues...
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com