Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 1884608 - Review Request: dosbox-staging - DOS/x86 emulator focusing on ease of use
Summary: Review Request: dosbox-staging - DOS/x86 emulator focusing on ease of use
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Hans de Goede
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks: FE-NEEDSPONSOR
TreeView+ depends on / blocked
 
Reported: 2020-10-02 12:38 UTC by Patryk Obara
Modified: 2021-03-04 13:41 UTC (History)
9 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2021-03-04 13:41:21 UTC
Type: ---
Embargoed:
hdegoede: fedora-review+


Attachments (Terms of Use)

Description Patryk Obara 2020-10-02 12:38:57 UTC
Spec URL: https://raw.githubusercontent.com/dosbox-staging/dosbox-staging/ca292836414b07ba501d71a7ffd5279eef0c6e75/contrib/fedora/dosbox-staging.spec
SRPM URL: https://github.com/dosbox-staging/dosbox-staging.github.io/releases/download/artifacts/dosbox-staging-0.75.1-1.fc32.src.rpm
Description: dosbox-staging is DOS/x86 emulator focusing on ease of use. https://dosbox-staging.github.io/about/
Fedora Account System Username: pbo

I am one of maintainers of dosbox-staging project. I'm not a Fedora packager and need a sponsor.

Comment 2 François Cami 2020-10-02 12:53:27 UTC
Taking. I'm the current DOSBox maintainer and am very interested in this package.

Comment 3 Patryk Obara 2020-10-27 13:18:33 UTC
We released 0.75.2 yesterday, I updated the spec file:

Spec URL: https://raw.githubusercontent.com/dosbox-staging/dosbox-staging/c280beeec011bacf96fa40cfe91260708cefec12/contrib/fedora/dosbox-staging.spec
SRPM URL: https://github.com/dosbox-staging/dosbox-staging.github.io/releases/download/artifacts/dosbox-staging-0.75.2-1.fc32.src.rpm

Koji build (f34): https://koji.fedoraproject.org/koji/taskinfo?taskID=54316364
Koji build (f33): https://koji.fedoraproject.org/koji/taskinfo?taskID=54315271

From changes: dosbox-staging includes metainfo.xml file now; diff on the spec file: https://github.com/dosbox-staging/dosbox-staging/commit/c280beeec011bacf96fa40cfe91260708cefec12

Other functional changes are listed in release announcement: https://dosbox-staging.github.io/v0-75-2/

Is there anything I can do to push this review forward?

Comment 4 Patryk Obara 2020-10-29 03:04:42 UTC
(In reply to François Cami from comment #2)
> Taking. I'm the current DOSBox maintainer and am very interested in this
> package.

Can we start the review process? I believe the spec is in quite good shape already.

@

Comment 5 Kamil Páral 2020-10-29 13:04:10 UTC
This happens if a user has 'dosbox' installed and tries to install 'dosbox-staging':

$ sudo dnf install ./dosbox-staging-0.75.2-1.fc33.x86_64.rpm 
...
Error: Transaction test error:
  file /usr/bin/dosbox from install of dosbox-staging-0.75.2-1.fc33.x86_64 conflicts with file from package dosbox-0.74.3-6.fc33.x86_64
  file /usr/share/man/man1/dosbox.1.gz from install of dosbox-staging-0.75.2-1.fc33.x86_64 conflicts with file from package dosbox-0.74.3-6.fc33.x86_64

I'm no packaging expert, but I believe either those files need to be renamed, or dosbox-staging needs to conflict with dosbox (explicitly in the rpm specfile).

Comment 6 Kamil Páral 2020-10-29 13:10:31 UTC
As a side note, many thanks for bringing dosbox-staging to Fedora, Patryk. Your project looks great (speaking as a retro gamer myself).

Comment 7 Patryk Obara 2020-10-29 17:49:37 UTC
@Kamil Páral
Thank you! :)

I wasn't even aware there is "Conflicts:" tag, that can be used.

URL with updated spec: https://raw.githubusercontent.com/dosbox-staging/dosbox-staging/bdb5d0685bd7d0d2da0b0f1f88006c526e8124bf/contrib/fedora/dosbox-staging.spec
diff: https://github.com/dosbox-staging/dosbox-staging/commit/bdb5d0685bd7d0d2da0b0f1f88006c526e8124bf


I think in this case we're ending up in following rule from the guidelines:

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

> (…) as long as there are no clear cases for both packages to be installed simultaneously, explicit Conflicts are permitted at the packager’s discretion. Both packages must carry Conflicts in this case.

In this case, I don't think users are interested in having both dosbox and dosbox-staging installed at the same time. The only usecase for doing so is development/regression testing - and for that developers are likely to build their own version of DOSBox (either from SVN or our mirror branch in Git).

Once fcami will have a moment to review the spec file, we probably should add "Conflicts: dosbox-staging" to dosbox package as well.

---

I explained why we kept the binary name (and consequently the man page) in this comment:
https://github.com/dosbox-staging/dosbox-staging/issues/664#issuecomment-718856147

We could use "alternatives" mechanism, but I prefer to do it only if users actually request this...
https://docs.fedoraproject.org/en-US/packaging-guidelines/Alternatives/ (using this would require alternating binary, man page, and probably patching desktop entry)

Comment 8 Kamil Páral 2020-11-03 07:31:56 UTC
Let's try to poke François...

Comment 9 Patryk Obara 2020-11-12 04:37:17 UTC
I decided to explicitly mark SDL BuildRequires as >= 2.0.2. Also, added a comment to justify -O3 usage to comply with packaging guidelines: 

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

> Overriding these flags for performance optimizations (for instance, -O3 instead of -O2) is generally discouraged. If you can present benchmarks that show a significant speedup for this particular code, this could be revisited on a case-by-case basis. 
> Adding to and overriding or filtering parts of these flags is permitted if there’s a good reason to do so; the rationale for doing so must be documented in the specfile.

If -O3 is not acceptable, I am willing to drop it and go back to -O2 - recent benchmarks for upcoming dosbox-staging 0.76.0 show much smaller performance improvement than what I remember from benchmarking 0.75.0. For 0.76.0 using -O3 generally gives ~1-2% performance gain in average FPS and up to 5% improvement in minimum FPS. For benchmarking I use relevant subset of https://www.philscomputerlab.com/dos-benchmark-pack.html

URL with updated spec: https://raw.githubusercontent.com/dosbox-staging/dosbox-staging/a4ca6f5c45548fad2945a7e832e998777f2147af/contrib/fedora/dosbox-staging.spec
diff: https://github.com/dosbox-staging/dosbox-staging/commit/a4ca6f5c45548fad2945a7e832e998777f2147af
SRPM: https://github.com/dosbox-staging/dosbox-staging.github.io/releases/download/artifacts/dosbox-staging-0.75.2-2.fc33.src.rpm

Koji f33: https://koji.fedoraproject.org/koji/taskinfo?taskID=55428796
Koji rawhide: https://koji.fedoraproject.org/koji/taskinfo?taskID=55429208

Comment 10 Package Review 2020-12-04 00:45:15 UTC
This is an automatic action taken by review-stats script.

The ticket reviewer failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we reset the status and the assignee of this ticket.

Comment 11 François Cami 2020-12-04 08:12:38 UTC
Re-taking. Apologies for the delay, I started doing the review. Will be done soon.

Comment 12 Patryk Obara 2020-12-04 12:11:33 UTC
No problem, glad you're back on it :)

We did release 0.76.0 literally yesterday, but let's keep this review for 0.75.2 (in next version we added FluidSynth 2.x dependency, which is not available e.g. on Fedora 32).

Comment 13 Andy Mender 2020-12-05 12:54:38 UTC
François, I set the Status and flags for you :).

Comment 14 Patryk Obara 2020-12-13 22:03:21 UTC
Is there something I can do to push this review forward? It's only a simple autotools build, really nothing out of ordinary…

Comment 15 Kamil Páral 2020-12-30 06:41:49 UTC
Patryk, at this point I believe you should ask for a different package reviewer/sponsor on the devel list [1]. I'm sure François means well, but he clearly isn't responsive enough to move this forward :-/

[1] https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/

Comment 16 Package Review 2021-01-15 00:45:19 UTC
This is an automatic action taken by review-stats script.

The ticket reviewer failed to clear the NEEDINFO flag in a month.
As per https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews
we reset the status and the assignee of this ticket.

Comment 17 Hans de Goede 2021-01-23 10:59:20 UTC
As discussed by email I'm taking over this review from François.

I've done a full review, below is the list of all checks
run (generated with the help of the fedora-review tool)

Note there are a few (trivial) [!] fail items in here,
I've put a summary of those with some extra explanation at the
end of this comment as a small TODO/FIXME list.


Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated

===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: If your application is a C or C++ application you must list a
     BuildRequires against gcc, gcc-c++ or clang.
[-]: Header files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license (GPLv2+)
     and meets other legal requirements as defined in the legal section
     of Packaging Guidelines.
[x]: License field in the package spec file matches the actual license.
[x]: License file installed when any subpackage combination is installed.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[!]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[!]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[!]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 81920 bytes in 3 files.
[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install or
     desktop-file-validate if there is such a file.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package must not depend on deprecated() packages.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane
[x]: Package functions as described.
[!]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Fully versioned dependency in subpackages if applicable.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on debuginfo package(s).
     Note: No rpmlint messages.
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Package should not use obsolete m4 macros
[x]: Spec file according to URL is the same as in SRPM.

Rpmlint
-------
Checking: dosbox-staging-0.75.2-2.fc34.x86_64.rpm
          dosbox-staging-debuginfo-0.75.2-2.fc34.x86_64.rpm
          dosbox-staging-debugsource-0.75.2-2.fc34.x86_64.rpm
          dosbox-staging-0.75.2-2.fc34.src.rpm
dosbox-staging.x86_64: W: spelling-error %description -l en_US chipsets -> chip sets, chip-sets, chips
dosbox-staging.x86_64: W: spelling-error %description -l en_US shaders -> shades, sharers, shavers
dosbox-staging.src: W: spelling-error %description -l en_US chipsets -> chip sets, chip-sets, chips
dosbox-staging.src: W: spelling-error %description -l en_US shaders -> shades, sharers, shavers



TODO / FIXME
============
[!]: Package consistently uses macros

  You use %{__make} in %build and just plain "make"
  in %install. Please be consistent. Note you can also choose to use:
  "%{make_build}" instead of "%{__make} %{_smp_mflags}" and
  "%{make_install}" instead of "%{__make} install DESTDIR=%{buildroot}"
  
  Also there is no need to pass " -n %{name}-%{version}" to
  "%autosetup" as that is the default, so please drop this.

[!]: Package does not generate any conflict.
[!]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
  ATM the package is using a Conflicts tag, but we generally do not allow Conflicts
  in Fedora. As discussed by email the plan is for this to replace the old dosbox
  package. Since it is a new upstream it is not really a rename but the way to handle
  this is the same, please replace the:
  
Conflicts: dosbox

  line in the spec-file with:
  
Provides:  dosbox = %{version}-%{release}
Obsoletes: dosbox = %{version}-%{release}

  This will switch users to the new dosbox-staging automatically during update
  installation.

[!]: Latest version is packaged.
  0.76.0 is available, which I'm sure you are aware of, please upgrade
  the package to 0.76.0 (or newer)

[!] Missing BuildRequires: make
  Starting with Fedora 34 make is no longer part of the default package
  set inside the build-root, please add a BuildRequires: make

[!] Missing BuildRequires: fluidsynth-devel
  Please add a BuildRequires: fluidsynth-devel so that the package
  gets build with fluidsynth support

[?] Missing BuildRequires: zstd
  The upstream docs mention that zstd should be present during the
  build but it is missing in the BuildRequires. Please explain
  why this is not necessary, or add the BuildRquires
  (maybe fix the upstreamd docs?)



Patryk, if you can post a new version with these items resolved
then I can approve the package and sponsor you to become a
packager. To sponsor you I also need to know your Fedora
Account System (FAS) username.

Comment 18 Hans de Goede 2021-01-23 11:04:00 UTC
Oops, I made a copy and paste error in the Provides/Obsoletes bits, the correct lines are:

Provides:  dosbox = %{version}-%{release}
Obsoletes: dosbox < %{version}-%{release}

Comment 19 Kamil Páral 2021-01-25 11:11:14 UTC
> As discussed by email I'm taking over this review from François.

Thanks!

> ATM the package is using a Conflicts tag, but we generally do not allow Conflicts
> in Fedora. As discussed by email the plan is for this to replace the old dosbox
> package.

Hans, just checking, has this been agreed on with the existing dosbox maintainer?

From [1] I see there is some (very small) functionality loss when replacing dosbox with dosbox-staging. If this case, the new package would only be suitable for Rawhide, so that users of stable releases don't suffer any regressions, I believe (and optionally it'd be also nice to see a Fedora Change proposal created to highlight the transition).

[1] https://github.com/dosbox-staging/dosbox-staging#summary-of-differences-compared-to-upstream

Comment 20 Hans de Goede 2021-01-25 11:33:06 UTC
(In reply to Kamil Páral from comment #19)
> > ATM the package is using a Conflicts tag, but we generally do not allow Conflicts
> > in Fedora. As discussed by email the plan is for this to replace the old dosbox
> > package.
> 
> Hans, just checking, has this been agreed on with the existing dosbox
> maintainer?

Yes.

> From [1] I see there is some (very small) functionality loss when replacing
> dosbox with dosbox-staging. If this case, the new package would only be
> suitable for Rawhide, so that users of stable releases don't suffer any
> regressions, I believe (and optionally it'd be also nice to see a Fedora
> Change proposal created to highlight the transition).

The feature loss is limited to playing audio from physical game CDs, which I doubt has a lot of users.

The current Fedora dosbox package OTOH is pretty broken, e.g. in Fedora 33 the cursor-keys do not even work (I don't know when they broke, but atm they are definitely broken). I did not check, but I guess that this is caused by the original dosbox shipping with a patched SDL1 and our packages using the system SDL1. But I think that it is safe to say that if something as essential as the cursor keys are not working that switching to dosbox-staging is going to be a big improvement overall.

Comment 21 Kamil Páral 2021-01-25 11:37:55 UTC
> The feature loss is limited to playing audio from physical game CDs, which I doubt has a lot of users.

I also see that "windowresolution=X%" option is no longer supported in the config file. But yes, judging just from that table, the differences seem very small.

> The current Fedora dosbox package OTOH is pretty broken, e.g. in Fedora 33 the cursor-keys do not even work

Alright, I haven't had time to play with dosbox for a long time, but if this is the case, replacing it with dosbox-staging sounds like a clear-cut improvement.

Comment 22 Patryk Obara 2021-01-25 23:20:31 UTC
Spec URL: https://raw.githubusercontent.com/dosbox-staging/dosbox-staging/master/contrib/fedora/dosbox-staging.spec
Spec diff: https://github.com/dosbox-staging/dosbox-staging/commit/a827baa1b2cddd6cea166cafd6b597c568eac58d
SRPM URL: https://github.com/dosbox-staging/dosbox-staging.github.io/releases/download/artifacts/dosbox-staging-0.76.0-1.fc33.src.rpm
Koji f33 build: https://koji.fedoraproject.org/koji/taskinfo?taskID=60490850
Koji f34 build: https://koji.fedoraproject.org/koji/taskinfo?taskID=60491350

(In reply to Hans de Goede from comment #17)
> TODO / FIXME
> ============
> [!]: Package consistently uses macros
> 
>   You use %{__make} in %build and just plain "make"
>   in %install. Please be consistent. Note you can also choose to use:
>   "%{make_build}" instead of "%{__make} %{_smp_mflags}" and
>   "%{make_install}" instead of "%{__make} install DESTDIR=%{buildroot}"
>   
>   Also there is no need to pass " -n %{name}-%{version}" to
>   "%autosetup" as that is the default, so please drop this.

Done and done :)

> [!]: If the package is a rename of another package, proper Obsoletes and
>      Provides are present.
>   
> Provides:  dosbox = %{version}-%{release}
> Obsoletes: dosbox < %{version}-%{release}

Done. I also tested if dosbox-staging is correctly picked by Wine and it works exactly as it should :) When Wine is instructed to run DOS executable, it delegates the job to dosbox binary, picking dosbox-staging. Wine package in Fedora does not depend on dosbox explicitly.

>   This will switch users to the new dosbox-staging automatically during
> update
>   installation.

Tested it and indeed, dnf replaced the package automatically now instead of failing due to the conflict as before.

> [!]: Latest version is packaged.

Done.

> [!] Missing BuildRequires: make

Done, and Koji build on f34 finished without a problem.

> [!] Missing BuildRequires: fluidsynth-devel

This is a new dependency for 0.76.0, that's why it was omitted earlier. One thing I forgot to add in spec 0.76.0-1 - we actually depend on FluidSynth >= 2.0 (therefore this spec file won't work for Fedora 32 which ships FluidSynth 1.1). Should I specify the package version in here? There's also a similar story for SDL2 - we specifically depend on >= 2.0.2 (but all Fedora versions provide it).

I also added another dependency (runtime only, not a build dependency): fluid-soundfont-gm - it's the package providing FluidR3 soundfont and setting up default.sf2 symlink - thanks to this, users who want to use FluidSynth MIDI feature have high-quality default soundfont installed out of the box.

> [?] Missing BuildRequires: zstd

This is a bug in our documentation/scripts - at the moment we do not depend on zstd. We use it for compressing certain artifacts on CI and the dependency made its way to some scripts. To be fixed on our master branch.

> Patryk, if you can post a new version with these items resolved
> then I can approve the package and sponsor you to become a
> packager. To sponsor you I also need to know your Fedora
> Account System (FAS) username.

My FAS username is: pbo

(In reply to Hans de Goede from comment #20)
> 
> The feature loss is limited to playing audio from physical game CDs, which I
> doubt has a lot of users.

Yes, it's not really used any more. Over last year we had 3 users complain about it. 1 user misunderstood the scope of the change, 2 users were not using physical CDs anyway, but rather were mounting CD images using some Windows virtual drive software (as opposed to using built-in functionality: imgmount command). The feature was removed because SDL2 dropped support for library SDL_cdrom (also due to lack of interest).

> The current Fedora dosbox package OTOH is pretty broken, e.g. in Fedora 33
> the cursor-keys do not even work (I don't know when they broke, but atm they
> are definitely broken). I did not check, but I guess that this is caused by
> the original dosbox shipping with a patched SDL1 and our packages using the
> system SDL1

Yes, in vanilla DOSBox cursor keys are broken when using Wayland. There are other critical issues as well: input breaks in certain games when starting in fullscreen (both X11 and Wayland), multi-display setups break badly (both X11 and Wayland), CD-DA emulation for some formats simply does not work (due to obsolete SDL_sound 1.2 library), etc… dosbox-staging fixes all of these problems, but we do have some important known issues to fix still.

(In reply to Kamil Páral from comment #21)
> I also see that "windowresolution=X%" option is no longer supported in the
> config file. But yes, judging just from that table, the differences seem
> very small.

This feature was never released by vanilla DOSBox - it exists only on SVN trunk. We removed it because it's not well-thought-out (it's unclear what % is supposed to refer to - might be screen resolution/desktop area/original game resolution, etc), and it conflicts conceptually with support for a resizable window.

Originally I intended dosbox-staging to be installable as an alternative to dosbox, and suggest replacing maybe for f35 (if everything worked well), but I am ok with having faster schedule :) Especially with François on board with this change (as indicated in mail discussion).

Comment 23 Kamil Páral 2021-01-26 08:58:25 UTC
(In reply to Hans de Goede from comment #18)
> Oops, I made a copy and paste error in the Provides/Obsoletes bits, the
> correct lines are:
> 
> Provides:  dosbox = %{version}-%{release}
> Obsoletes: dosbox < %{version}-%{release}

I believe this is not correct. According to the documentation [1], and given that dosbox's highest NVR is dosbox-0.74.3-6.fc34 [2], I believe it should be:

> Provides:  dosbox = %{version}-%{release}
> Obsoletes: dosbox < 0.74.4

I chose to bump the version field instead of release, because there's a mass rebuild happening in Rawhide and the release field might be bumped automatically. Of course this assumes that the dosbox developer will not package and release 0.74.4 or higher (which is, I assume, agreed on, and the dosbox package can be removed from Fedora once dosbox-staging is approved and built).

[1] https://docs.fedoraproject.org/en-US/packaging-guidelines/#renaming-or-replacing-existing-packages
[2] https://src.fedoraproject.org/rpms/dosbox

> This is a new dependency for 0.76.0, that's why it was omitted earlier. One thing I forgot to add in spec 0.76.0-1 - we actually depend on FluidSynth >= 2.0 (therefore this spec file won't work for Fedora 32 which ships FluidSynth 1.1). Should I specify the package version in here? There's also a similar story for SDL2 - we specifically depend on >= 2.0.2 (but all Fedora versions provide it).

If you know specific minimum version, I believe it should be explicitly listed, because it prevents errors. That is nicely illustrated with the Fedora 32 situation - the package requirements should stop an incorrect situation from happening - like a failed build due to insufficient library versions, or a completed build but a broken functionality in the final binary. In this case, either dosbox-staging can't be submitted to Fedora 32, or the FluidSynth maintainer needs to be asked to update the F32 version to >= 2.0 (which might or might not be appropriate for a stable Fedora release, that's their area of expertise). 

Hans, please correct me if I'm wrong about some of that :-)

> Yes, in vanilla DOSBox cursor keys are broken when using Wayland
> ...

Thanks for clarifications about the feature set.

Comment 24 Patryk Obara 2021-01-26 18:05:14 UTC
(In reply to Kamil Páral from comment #23)
> (In reply to Hans de Goede from comment #18)
> > Oops, I made a copy and paste error in the Provides/Obsoletes bits, the
> > correct lines are:
> > 
> > Provides:  dosbox = %{version}-%{release}
> > Obsoletes: dosbox < %{version}-%{release}
> 
> I believe this is not correct. According to the documentation [1], and given
> that dosbox's highest NVR is dosbox-0.74.3-6.fc34 [2], I believe it should
> be:
> 
> > Provides:  dosbox = %{version}-%{release}
> > Obsoletes: dosbox < 0.74.4


Good point. I updated the spec file to 0.76.0-2:

Spec URL: https://raw.githubusercontent.com/dosbox-staging/dosbox-staging/e131d5624021e6afe088507f7c7526d404f60be9/contrib/fedora/dosbox-staging.spec
Spec Diff: https://github.com/dosbox-staging/dosbox-staging/commit/e131d5624021e6afe088507f7c7526d404f60be9
SRPM URL: https://github.com/dosbox-staging/dosbox-staging.github.io/releases/download/artifacts/dosbox-staging-0.76.0-2.fc33.src.rpm

Comment 25 Patryk Obara 2021-02-07 12:09:37 UTC
Are there any other issues with the spec file that need to be addressed?

Comment 26 Hans de Goede 2021-02-07 14:04:52 UTC
(In reply to Patryk Obara from comment #25)
> Are there any other issues with the spec file that need to be addressed?

No, I its just that I've been somewhat swamped with other stuff. I can make some time now, so let me check the latest version right away.

Comment 27 Hans de Goede 2021-02-07 14:16:05 UTC
Ok, with the 0.76.0-2 version everything looks good / everything from the TODO list I posted earlier has been addressed.

So this package is now approved and I've just sponsored you into the Fedora packager group in the account system.

So now you can proceed with the next steps, request a distgit repo to be created for the pkg and then import and build it:
https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner

Comment 28 Hans de Goede 2021-02-07 14:18:26 UTC
p.s. Please let me know (send me an email) if you hit any issues, or if you are unsure about how to do something. As your sponsor I'm here to answer any questions about processes / tooling. Please don't hesitate to ask questions if you are afraid that you might do something wrong.

Comment 29 Mohan Boddu 2021-02-10 15:47:27 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/dosbox-staging

Comment 30 Patryk Obara 2021-03-04 13:34:40 UTC
Thanks everyone for the review; dosbox-staging package is available in F34 for few weeks now and I just pushed the update to Bodhi for F33 users as well: https://bodhi.fedoraproject.org/updates/FEDORA-2021-c96edcd678 (waiting for karma to raise).

Comment 31 Hans de Goede 2021-03-04 13:41:21 UTC
(In reply to Patryk Obara from comment #30)
> Thanks everyone for the review; dosbox-staging package is available in F34
> for few weeks now and I just pushed the update to Bodhi for F33 users as
> well: https://bodhi.fedoraproject.org/updates/FEDORA-2021-c96edcd678
> (waiting for karma to raise).

Thank you. Since this is available in F34 and rawhide now, this bug can be closed, so let me do that right away.


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