Bug 2254109 - Review Request: batstat - CLI battery status for Linux
Summary: Review Request: batstat - CLI battery status for Linux
Keywords:
Status: ASSIGNED
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Jonathan Wright
QA Contact: Fedora Extras Quality Assurance
URL: https://github.com/Juve45/%{name}
Whiteboard:
Depends On:
Blocks: FE-NEEDSPONSOR
TreeView+ depends on / blocked
 
Reported: 2023-12-12 00:33 UTC by Cody Robertson
Modified: 2024-03-01 17:27 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed:
Type: ---
Embargoed:
xavier: fedora-review?


Attachments (Terms of Use)

Comment 1 Fedora Review Service 2023-12-12 00:39:16 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6742431
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2254109-batstat/fedora-rawhide-x86_64/06742431-batstat/fedora-review/review.txt

Found issues:

- Not a valid SPDX expression 'None'.
  Read more: https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1

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.

Comment 2 Carl George 🤠 2023-12-20 21:30:52 UTC
Hey Cody, I can give this review a first pass to correct a few things, until Jonathan has time to complete the full review.

================================================================================

The most important problem I see is the lack of a license.  I can see that upstream hasn't specified a license, which means they retain all rights and no one is allowed to redistribute the code.  Fedora also requires that packages be licensed under open source licenses.  I suggest filing an issue upstream to ask them what license the software is available under.

Once upstream does specify a license, they'll likely add the license text file to the repository, at which point you need to include it in the package by adding this line to the %files section.

    %license LICENSE

You'll also update the License: field to the appropriate SPDX expression.

https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/licensing-a-repository#choosing-the-right-license
https://docs.fedoraproject.org/en-US/packaging-guidelines/what-can-be-packaged/
https://docs.fedoraproject.org/en-US/legal/license-field/
https://docs.fedoraproject.org/en-US/legal/allowed-licenses/

================================================================================

Your debug_packages macro definition is disabling debuginfo packages.  The guidelines state that packages should produce useful debuginfo packages, and only disable them when it's not possible to generate a useful one.  I can see that the upstream makefile as-is doesn't create the debug symbols, which is likely why you added that line in the first place.  It's better to resolve the lack of debug symbols rather than disabling the debuginfo packages.  It's also required to honor the Fedora system-wide compiler flags.  We can solve both of these issues at once with a small change.

    %make_build CCFLAGS="%{optflags}"

https://docs.fedoraproject.org/en-US/packaging-guidelines/#_debuginfo_packages
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_compiler_flags

================================================================================

Unfortunately this upstream has never tagged a version, so the versioning will be a bit complex.  The guidelines mention using version 0 in this scenario, but also have seperate instructions for packaging snapshots.  We need both in this case, but the guidelines don't have instructions for combining them.  I'm in the process of writing this up to improve the docs, but this is what it will look like for this package in the end.  Note that this also adjust the Source0 line, not just for the snapshot bits but to use a complete URL to indicate where the tarball comes from, which is also part of the guidelines.

    %global commit      e107193e99fb8d461050358f05aa8343e2fd5bc9
    %global shortcommit %(c=%{commit}; echo ${c:0:7})

    Version:        0~^1.%{shortcommit}

    Source0:        %{url}/archive/%{commit}/batstat-%{shortcommit}.tar.gz

    %autosetup -n batstat-%{commit}

https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_upstream_has_never_chosen_a_version
https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_snapshots
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_sections

================================================================================

It is recommended to use %autochange, especially since you are already using %autorelease.

https://docs.fedoraproject.org/en-US/packaging-guidelines/#changelogs

================================================================================

You can remove the gcc build requirement, since this is a c++ program.

================================================================================

You can remove the %check section since there are no tests to run.

================================================================================

The manual specification of /usr/bin in %install should be switched to %{_bindir} for consistency.

    mkdir -p %{buildroot}%{_bindir}


Note You need to log in before you can comment on or make changes to this bug.