[Bug 2053822] New: Review Request: featherwallet-1.0.1 - Free and open-source Monero desktop wallet
https://bugzilla.redhat.com/show_bug.cgi?id=2053822
Bug ID: 2053822 Summary: Review Request: featherwallet-1.0.1 - Free and open-source Monero desktop wallet Product: Fedora Version: rawhide Status: NEW Component: Package Review Assignee: nobody@fedoraproject.org Reporter: js-fedora@nil.im QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://nil.im/featherwallet.spec SRPM URL: https://nil.im/featherwallet-1.0.1-1.fc35.src.rpm Description: Free and open-source Monero desktop wallet
Feather is a free, open-source Monero wallet. It is written in C++ with the Qt framework.
Fedora Account System Username: js Successful Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=82726257
https://bugzilla.redhat.com/show_bug.cgi?id=2053822
Oleg Girko ol+redhat@infoserver.lv changed:
What |Removed |Added ---------------------------------------------------------------------------- Doc Type|--- |If docs needed, set a value CC| |ol+redhat@infoserver.lv
--- Comment #1 from Oleg Girko ol+redhat@infoserver.lv --- 1. I think, using "%{_builddir}/feather/redhat-linux-build/bin/feather" in %install section is not portable. Since out of source build was introduced, the directory where build happens changed at least once, and I won't be surprised if it will change in the future again. Better use "%{_vpath_builddir}/bin/feather" instead. Also, it will improve readability. 2. Or even better, try to use %cmake_install to install binary, and resort to manual installation only if this method doesn't work, but do it with "install" command, not "cp". 3. Using "%{_builddir}/feather/" prefix in %install section is unnecessary and harms readability: %install script is already run in this directory.
Please note that this is not a proper complete review, I've just listed things that I found by looking at the spec file.
https://bugzilla.redhat.com/show_bug.cgi?id=2053822
--- Comment #2 from Jonathan S. js-fedora@nil.im --- Thanks for the review!
1.) Thanks, I did not know about _vpath_builddir. Very useful! 2.) Unfortunately that installs the binary to /usr/feather, installs header files at random places, installs libraries that are only used internally, etc. 3.) Thanks, removed.
I updated the spec file (no updated SRPM yet).
Would you be willing to formally review?
https://bugzilla.redhat.com/show_bug.cgi?id=2053822
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |zbyszek@in.waw.pl Summary|Review Request: |Review Request: |featherwallet-1.0.1 - Free |featherwallet - Free and |and open-source Monero |open-source Monero desktop |desktop wallet |wallet
https://bugzilla.redhat.com/show_bug.cgi?id=2053822
--- Comment #3 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- "Free and open-source" applies to all packages in Fedora. So that part of the Summary is not useful. And it you don't know what "monero" is, and I don't, the rest won't tell you much either… Please try to write the Summary+description so that it is meaningful to someone who is completely new to the subject.
Also "written in C++ with the Qt framework" is not terribly interesting to users. You can mention Qt, but other details of implementation are unimportant.
sed -i 's/^(Icon|Exec)=feather$/&wallet/' %{buildroot}%{_datadir}/applications/featherwallet.desktop sed -i 's/^Name=Feather$/& Wallet/' %{buildroot}%{_datadir}/applications/featherwallet.desktop
Those commands should be in %prep. This is where we apply patches and do other modifications to sources.
mkdir -p %{buildroot}%{_bindir} %{buildroot}%{_datadir}/applications %{buildroot}%{_datadir}/pixmaps cp %{_vpath_builddir}/bin/feather %{buildroot}%{_bindir}/featherwallet cp src/assets/feather.desktop %{buildroot}%{_datadir}/applications/featherwallet.desktop
It's a bit nicer to say: install -D %{buildroot}%{_bindir}/featherwallet %{_vpath_builddir}/bin/feather install -m0644 -D %{buildroot}%{_datadir}/applications/featherwallet.desktop src/assets/feather.desktop etc.
I'd recommend dropping the manual Release and %changelog handling. See https://docs.pagure.org/fedora-infra.rpmautospec/index.html.
%global debug_package %{nil}
Why? This is usually a sign of other problem with the build systems, usually missing flags. Please drop this and instead figure out what is needed to for the -debuginfo package to generate properly.
Since this is a graphical application, it should have an appdata file: https://docs.fedoraproject.org/en-US/packaging-guidelines/AppData/ (with a pretty screenshot ;))
https://bugzilla.redhat.com/show_bug.cgi?id=2053822
Jonathan S. js-fedora@nil.im changed:
What |Removed |Added ---------------------------------------------------------------------------- Summary|Review Request: |Review Request: feather - |featherwallet - Free and |Monero desktop wallet |open-source Monero desktop | |wallet |
--- Comment #4 from Jonathan S. js-fedora@nil.im --- feather-2.3.0-1.fc37.src.rpm
https://bugzilla.redhat.com/show_bug.cgi?id=2053822
--- Comment #5 from Jonathan S. js-fedora@nil.im --- Sorry, last comment was in accident when changing the title.
I uploaded a new SRPM: https://nil.im/feather-2.3.0-1.fc37.src.rpm Updated spec file: https://nil.im/feather.spec
PTAL!
https://bugzilla.redhat.com/show_bug.cgi?id=2053822
Vitaly Zaitsev vitaly@easycoding.org changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |vitaly@easycoding.org
--- Comment #6 from Vitaly Zaitsev vitaly@easycoding.org ---
License: BSD
The License tag must be in SPDX format:
$ license-fedora2spdx 'BSD' Warning: more options on how to interpret BSD. Possible options: ['BSD-1-Clause', 'BSD-2-Clause-FreeBSD', 'BSD-2-Clause-Views', 'BSD-2-Clause', 'BSD-3-Clause-Modification', 'BSD-3-Clause'] {{pick BSD choice}}
ExcludeArch: armv7hl
Please use this: ExcludeArch: %{arm} %{ix86}
gpgv2 --quiet --keyring %{SOURCE2} %{SOURCE1} %{SOURCE0}
Please switch to %{gpgverify} macro: %{gpgverify} --keyring='%{SOURCE2}' --signature='%{SOURCE1}' --data='%{SOURCE0}'
-DCMAKE_BUILD_TYPE=RelWithDebInfo
You can use Release here, because `-g` flag is already present in Fedora build flags. RelWithDebInfo will just duplicate it.
# Their install target is not meant to be used. It installs the binary into # /usr (not /usr/bin) and installs a bunch of other useless files.
I think you should port their CMake build manifest to GNUInstallDirs and send a pull request instead of manually calling install binary.
https://bugzilla.redhat.com/show_bug.cgi?id=2053822
--- Comment #7 from Jonathan S. js-fedora@nil.im ---
The License tag must be in SPDX format:
Done
Please use this: ExcludeArch: %{arm} %{ix86}
Done
Please switch to %{gpgverify} macro:
Done
You can use Release here, because `-g` flag is already present in Fedora build flags. RelWithDebInfo will just duplicate it.
This doesn't work - with just Release (the default IIRC), it'll result in an empty debug package. RelWithDebInfo makes it work.
I think you should port their CMake build manifest to GNUInstallDirs and send a pull request instead of manually calling install binary.
That doesn't help with this release, though, so is something for later :).
https://bugzilla.redhat.com/show_bug.cgi?id=2053822
--- Comment #8 from Vitaly Zaitsev vitaly@easycoding.org ---
This doesn't work - with just Release (the default IIRC), it'll result in an empty debug package. RelWithDebInfo makes it work.
Then the package ignores the Fedora build flags. You need to check this.
https://bugzilla.redhat.com/show_bug.cgi?id=2053822
Jonathan S. js-fedora@nil.im changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|NOTABUG |--- Status|CLOSED |NEW Keywords| |Reopened
--- Comment #11 from Jonathan S. js-fedora@nil.im --- New spec file and SRPM for much newer version.
I checked and the Fedora flags *are* passed to GCC (both during compiling and linking), but RelWithDebInfo is still required as otherwise it seems the build process strips the binaries.
Spec URL: https://nil.im/feather.spec SRPM URL: https://nil.im/feather-2.6.8-1.fc40.src.rpm
https://bugzilla.redhat.com/show_bug.cgi?id=2053822
Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|nobody@fedoraproject.org |zbyszek@in.waw.pl Status|NEW |POST Flags| |fedora-review+
--- Comment #12 from Zbigniew Jędrzejewski-Szmek zbyszek@in.waw.pl --- + package name is OK + license is acceptable for Fedora (BSD-3-Clause) + license is specified correctly as SPDX + latest version + gpg signature is checked + builds and installs OK + builds flags are passed to the build commands + BR/P/R look OK + appdata file is present - %check is not present. Maybe add a test run to make sure that the executable works, e.g. just print '--help' output?
rpmlint: feather.src: E: spelling-error ('Monero', 'Summary(en_US) Monero -> Moreno, Monroe, Monera') feather.x86_64: E: spelling-error ('Monero', 'Summary(en_US) Monero -> Moreno, Monroe, Monera') Obviously bogus.
feather.x86_64: W: no-manual-page-for-binary feather feather.x86_64: W: no-documentation True, but not a big issue.
feather.x86_64: W: crypto-policy-non-compliance-openssl /usr/bin/feather SSL_CTX_set_cipher_list Hmm, this requires investigation.
feather-2.6.8/monero/contrib/epee/src/net_ssl.cpp 312: SSL_CTX_set_cipher_list(ssl_context.native_handle(), "ECDHE-ECDSA-CHACHA20-POLY1305-SHA256:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-RSA-AES128-GCM-SHA256");
https://docs.fedoraproject.org/en-US/packaging-guidelines/CryptoPolicies/ says:
OpenSSL applications: If the application doesn’t have a configuration file, ensure that there is no default cipher list specified, or that the default list is set as "PROFILE=SYSTEM". That is, check the source code for SSL_CTX_set_cipher_list(). If it is not present then nothing needs to be done (the default is used). Otherwise, if that call is present and provided a fixed string which does not contain PSK or SRP, replace the string with "PROFILE=SYSTEM", or remove the call.
But also:
Note however, that there are applications which intentionally set weaker, or custom settings on a purpose (e.g., postfix); those need not adhere to the policy. When in doubt, discuss with the Fedora crypto team.
Feather sets a *stronger* policy, clearly on purpose. I think this clearly falls into the exception quoted above and doesn't need to be discussed with the "Fedora crypto team".
Package is APPROVED.
https://bugzilla.redhat.com/show_bug.cgi?id=2053822
Fedora Admin user for bugzilla script actions fedora-admin-xmlrpc@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |RELEASE_PENDING
--- Comment #13 from Fedora Admin user for bugzilla script actions fedora-admin-xmlrpc@fedoraproject.org --- The Pagure repository was created at https://src.fedoraproject.org/rpms/feather
https://bugzilla.redhat.com/show_bug.cgi?id=2053822
--- Comment #14 from Jonathan S. js-fedora@nil.im --- Thanks for the review!
Re OpenSSL policy: There is another reason to never ever touch this in feather: It will make you immediately stand out among everybody else, ruining your privacy.
https://bugzilla.redhat.com/show_bug.cgi?id=2053822
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RELEASE_PENDING |MODIFIED
--- Comment #15 from Fedora Update System updates@fedoraproject.org --- FEDORA-2024-0a046f3b59 (feather-2.6.8-1.fc42) has been submitted as an update to Fedora 42. https://bodhi.fedoraproject.org/updates/FEDORA-2024-0a046f3b59
https://bugzilla.redhat.com/show_bug.cgi?id=2053822
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |CLOSED Resolution|--- |ERRATA Last Closed|2024-03-14 00:45:32 |2024-09-15 12:08:21
--- Comment #16 from Fedora Update System updates@fedoraproject.org --- FEDORA-2024-0a046f3b59 (feather-2.6.8-1.fc42) has been pushed to the Fedora 42 stable repository. If problem still persists, please make note of it in this bug report.
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com