Bug 1924660 - Review Request: atari800 - An emulator of 8-bit Atari personal computers
Summary: Review Request: atari800 - An emulator of 8-bit Atari personal computers
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Lubomir Rintel
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2021-02-03 12:01 UTC by Jan ONDREJ
Modified: 2021-03-20 01:00 UTC (History)
2 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2021-03-12 20:29:49 UTC
Type: ---
lkundrak: fedora-review+


Attachments (Terms of Use)

Description Jan ONDREJ 2021-02-03 12:01:24 UTC
Spec URL: https://www.salstar.sk/pub/fedora/SPECS/atari800.spec
SRPM URL: https://www.salstar.sk/pub/fedora/33/SRPMS/atari800-4.2.0-1.fc33.src.rpm

Description:
Atari800 is an emulator for the 800, 800XL, 130XE and 5200 models of
the Atari personal computer. It can be used on console, FrameBuffer or X11.
It features excellent compatibility, HIFI sound support, artifacting
emulation, precise cycle-exact ANTIC/GTIA emulation and more.

Fedora Account System Username: ondrejj

This version doesn't require original ROM files, because Altirra ROM replacement is included.
For basic usage it is possible to legally use internal BASIC or own binaries for Atari computers.

Comment 1 Lubomir Rintel 2021-03-04 00:21:02 UTC
Thank you for packaging this. Here's my review:

* Package named correctly
* Latest version used
* SPEC file clean and legible
* Correct license tag used
* License good for Fedora
* Sane filelist
* Sane requires/provides
* No scriptlets shipped
* Correct compiler flags used
* Builds fine in mock

Below are some things I'd prefer you considered addressing before I approve
the package. All of them fairly minor:

1.) Please use the official dist tarball instead of just a git archive

> Source0:       https://github.com/%{name}/%{name}/archive/ATARI800_%{ver_}/%{name}-%{version}.tar.gz

The official source tarball seems to be autotools' make dist generated:
https://github.com/atari800/atari800/releases/download/ATARI800_4_2_0/atari800-4.2.0-src.tgz

> BuildRequires: gcc, autoconf, automake

You don't need to drag in autoconf & automake once you use the dist tarball.

> autoreconf -f -i

Likewise, you don't need this.

2.) Please consider removing the author reference from description:

> %description
...
> Authors:
> David Firth
> and Atari800 Development Team (see CREDITS for a full list)

While the credit might be due, %description servers a different purpose --
to inform the user what the package contains and no more than that.

I believe shipping the CREDITS file is a sufficient way of letting the
interested users know who wrote the software.

3.) Don't bother removing $RPM_BUILD_ROOT

> %install
> rm -rf $RPM_BUILD_ROOT

rpmbuild does that already:

  Executing(%install): /bin/sh -e /var/tmp/rpm-tmp.PRXywD
  + umask 022
  + cd /home/lkundrak/rpmbuild/BUILD
  + '[' /home/lkundrak/rpmbuild/BUILDROOT/atari800-4.2.0-1.fc33.x86_64 '!=' / ']'
  + rm -rf /home/lkundrak/rpmbuild/BUILDROOT/atari800-4.2.0-1.fc33.x86_64
  ++ dirname /home/lkundrak/rpmbuild/BUILDROOT/atari800-4.2.0-1.fc33.x86_64
  + mkdir -p /home/lkundrak/rpmbuild/BUILDROOT
  + mkdir /home/lkundrak/rpmbuild/BUILDROOT/atari800-4.2.0-1.fc33.x86_64
  + cd atari800-ATARI800_4_2_0
  + rm -rf /home/lkundrak/rpmbuild/BUILDROOT/atari800-4.2.0-1.fc33.x86_64
  + /usr/bin/make install DESTDIR=/home/lkundrak/rpmbuild/BUILDROOT/atari800-4.2.0-1.fc33.x86_64 'INSTALL=/usr/bin/install -p'

It may not have always does done, but modern versions do that, and
you already depend on new enough RPM by using macros such as
%make_install or %make_build.

> %make_install

4.) Please s/atari800.1.gz/atari800.1*/

> %files
...
> %{_mandir}/man1/atari800.1.gz

In theory, RPM might be configured not to compress manuals. It's a good
practice to use a wildcard here.

5. ) Please use %_pkgdocdir in place of /usr/share/doc/atari800

> %files
...
> %doc /usr/share/doc/atari800/README.TXT
> %doc /usr/share/doc/atari800/README

Or %{_docdir}/atari800; whichever you prefer.

Comment 2 Jan ONDREJ 2021-03-04 09:27:17 UTC
Thank you for review. Your suggestions have been applied. Changelog:

* Thu Mar 04 2021 Ján ONDREJ (SAL) <ondrejj(at)salstar.sk> - 4.2.0-2
- Updated description, source URL.
- Removed autoconf/automake and cleaning of RPM_BUILD_ROOT.
- Updated package doc directory to macro.
- Added configure --docdir parameter to allow build for epel7 too.

Updated spec and SRPM:

Spec URL: https://www.salstar.sk/pub/fedora/SPECS/atari800.spec
SRPM URL: https://www.salstar.sk/pub/fedora/33/SRPMS/atari800-4.2.0-2.fc33.src.rpm

Comment 3 Lubomir Rintel 2021-03-04 10:41:39 UTC
Looks very good now. Thank you.

APPROVED

Comment 4 Tomas Hrcka 2021-03-04 11:44:24 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/atari800

Comment 5 Fedora Update System 2021-03-05 10:13:41 UTC
FEDORA-2021-2cbcf38c7a has been submitted as an update to Fedora 33. https://bodhi.fedoraproject.org/updates/FEDORA-2021-2cbcf38c7a

Comment 6 Fedora Update System 2021-03-05 10:34:39 UTC
FEDORA-2021-77bf5a7792 has been submitted as an update to Fedora 32. https://bodhi.fedoraproject.org/updates/FEDORA-2021-77bf5a7792

Comment 7 Fedora Update System 2021-03-05 10:42:09 UTC
FEDORA-EPEL-2021-a5eae8e39d has been submitted as an update to Fedora EPEL 8. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2021-a5eae8e39d

Comment 8 Fedora Update System 2021-03-05 10:53:39 UTC
FEDORA-EPEL-2021-26b967230c has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2021-26b967230c

Comment 9 Fedora Update System 2021-03-05 18:57:01 UTC
FEDORA-2021-2cbcf38c7a has been pushed to the Fedora 33 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2021-2cbcf38c7a \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2021-2cbcf38c7a

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 10 Fedora Update System 2021-03-05 19:11:15 UTC
FEDORA-EPEL-2021-a5eae8e39d has been pushed to the Fedora EPEL 8 testing repository.

You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2021-a5eae8e39d

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 11 Fedora Update System 2021-03-05 19:31:02 UTC
FEDORA-EPEL-2021-26b967230c has been pushed to the Fedora EPEL 7 testing repository.

You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2021-26b967230c

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 12 Fedora Update System 2021-03-05 20:03:11 UTC
FEDORA-2021-77bf5a7792 has been pushed to the Fedora 32 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2021-77bf5a7792 \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2021-77bf5a7792

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 13 Fedora Update System 2021-03-12 20:29:49 UTC
FEDORA-2021-2cbcf38c7a has been pushed to the Fedora 33 stable repository.
If problem still persists, please make note of it in this bug report.

Comment 14 Fedora Update System 2021-03-13 20:53:32 UTC
FEDORA-2021-77bf5a7792 has been pushed to the Fedora 32 stable repository.
If problem still persists, please make note of it in this bug report.

Comment 15 Fedora Update System 2021-03-20 00:40:20 UTC
FEDORA-EPEL-2021-a5eae8e39d has been pushed to the Fedora EPEL 8 stable repository.
If problem still persists, please make note of it in this bug report.

Comment 16 Fedora Update System 2021-03-20 01:00:03 UTC
FEDORA-EPEL-2021-26b967230c has been pushed to the Fedora EPEL 7 stable repository.
If problem still persists, please make note of it in this bug report.


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