https://bugzilla.redhat.com/show_bug.cgi?id=1919639
Bug ID: 1919639 Summary: Review Request: DOSBox-X - DOS/x86 emulator Product: Fedora Version: rawhide Status: NEW Component: Package Review Assignee: nobody@fedoraproject.org Reporter: robert.de.rooy@gmail.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Created attachment 1750230 --> https://bugzilla.redhat.com/attachment.cgi?id=1750230&action=edit DOSBox-X spec file
Website: https://dosbox-x.com github: https://github.com/joncampbell123/dosbox-x F33 build: https://koji.fedoraproject.org/koji/taskinfo?taskID=60355367
Fedora Account System Username: rob72
I am a contributor of the dosbox-x project, and have been requested by the team to submit this request. I am not a Fedora packager and need a sponsor.
Hans de Goede had expressed interest in us submitting this.
DOSBox-X provides a long list of enhancements compared to regular DOSBox (or DOSBox staging). You can find an overview of some of the enhancements on the flathub page: https://flathub.org/apps/details/com.dosbox_x.DOSBox-X where you can also see the metainfo.
One possible limitation of DOSBox-X, is that its dynamic recompiler only works on x86, x86_64, arm and aarch64, and will not compile on ppc or s390x. As such the spec file contains the statement: ExclusiveArch: %{ix86} %{arm} x86_64 aarch64
DOSBox-X is typically updated once a month with a new release.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
Hans de Goede hdegoede@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |hdegoede@redhat.com Doc Type|--- |If docs needed, set a value
--- Comment #2 from Hans de Goede hdegoede@redhat.com --- (In reply to Vladislav Kazakov from comment #1)
Note that make can be omitted.
That is no longer true, starting with Fedora 34 make will not be part of the default package set in the buildroot. So it needs to have an explicit BuildRequires, so please keep a "BuildRequires: make" for the next version.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #3 from Hans de Goede hdegoede@redhat.com --- Robert,
Please create a new version addressing Vladislav's requests.
For the next version please upload a srpm and spec file somewhere and provide links/URLs to the spec-file and srpm.
We must have a srpm for review so that we can verify that the sources tarbal which you will upload into the look-aside cache when importing the package matches the upstream tarbal.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #4 from Hans de Goede hdegoede@redhat.com --- Some more remarks:
"License: GPLv2"
The orginal dosbox is GPLv2 or later, which we write as GPLv2+, is this not the case for dosbox-x?
Please drop all the Requires: on libraries rpm will auto generate these based on the used sonames. Note you will want to keep the fluid-soundfont-gm Requires.
""" desktop-file-install \ --dir=%{buildroot}%{_datadir}/applications \ --set-icon=com.dosbox_x.DOSBox-X \ contrib/linux/com.dosbox_x.DOSBox-X.desktop
install -p -m 0644 contrib/icons/dosbox-x.svg %{buildroot}%{_datadir}/icons/hicolor/scalable/apps/com.dosbox_x.DOSBox-X.svg
%exclude %{_datadir}/icons/hicolor/scalable/apps/dosbox-x.svg """
Maybe just have the .desktop file used the already installed dosbox-x.svg ?
"install -p -m 0644 -Dt %{buildroot}%{_datadir}/%{name}/glshaders contrib/glshaders/*.glsl"
Why ? Please add a comment explaining why this is done
Please verify the installed appdata file: https://docs.fedoraproject.org/en-US/packaging-guidelines/AppData/
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #5 from Robert robert.de.rooy@gmail.com --- I think I have fixed all the issues mentioned, apart from changing ./configure to %configure
The moment I change that, it seems that the configure flags specified are thrown out of the window and the compile just fails as it ends up looking for SDL1 libraries instead of SDL2. If someone can tell me how to use the %configure macro while keeping the flags, I would appreciate it.
Also, I used [AT] instead of @ in the email to reduce spam, I thought that was acceptable.
spec file: https://github.com/rderooy/dosbox-x-rpm/blob/master/dosbox-x.spec srpm file: https://github.com/rderooy/dosbox-x-rpm/raw/master/dosbox-x-0.83.9-1.fc33.sr... f33 build: https://koji.fedoraproject.org/koji/taskinfo?taskID=60490574 f34 build: https://koji.fedoraproject.org/koji/taskinfo?taskID=60490134
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #6 from Robert robert.de.rooy@gmail.com --- A note about the GPL version. I changed it to GPL2+, since I thought I made a mistake. But looking at the DOSBox COPYING file, it seems to be GPL2 as the heading does not contain the "any later version" text.
https://sourceforge.net/p/dosbox/code-0/HEAD/tree/dosbox/trunk/COPYING
DOSBox-X use the same file. https://github.com/joncampbell123/dosbox-x/blob/master/COPYING
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #7 from Vladislav Kazakov vpackager@gmail.com ---
Also, I used [AT] instead of @ in the email to reduce spam, I thought that was acceptable.
Yes, this is acceptable. I just thought it was a copying error.
The moment I change that, it seems that the configure flags specified are thrown out of the window and the compile just fails as it ends up looking for SDL1 libraries instead of SDL2.
If someone can tell me how to use the %configure macro while keeping the flags, I would appreciate it.
You can keep flags just like that: """ %configure --enable-core-inline --enable-debug=heavy --enable-sdl2 """ But it falls anyway whille compiling sdlmain.cpp, because the %configure adds -Werror=format-security to g++: """ sdl_ttf.c:334:18: error: format not a string literal and no format arguments [-Werror=format-security] 334 | TTF_SetError(msg); | ^ """ It would be nice to fix this in upstream. Or you can just remove -Werror=format-security.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #8 from Robert robert.de.rooy@gmail.com --- Thanks for your review so far.
As regards the format-security error, that issue had apparently already been fixed upstream. Since a new release (0.83.10) should be out in the next day, I will wait until it is released, and update the spec file.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #9 from Robert robert.de.rooy@gmail.com --- DOSBox-X 0.83.10 was just released, and I have updated the spec and srpm. It now uses the %configure macro.
SPEC file: https://github.com/rderooy/dosbox-x-rpm/blob/master/dosbox-x.spec SRPM file: https://github.com/rderooy/dosbox-x-rpm/raw/master/dosbox-x-0.83.10-1.fc33.s...
I don't have any koji builds right, but will get some done later today when I'm back home.
The new version is also already up on flathub: https://flathub.org/apps/details/com.dosbox_x.DOSBox-X
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #10 from Robert robert.de.rooy@gmail.com --- In addition to the SPEC and SRPM files linked above, here are the koji builds:
F33: https://koji.fedoraproject.org/koji/taskinfo?taskID=61053742 F34: https://koji.fedoraproject.org/koji/taskinfo?taskID=61054003
Note: I had to disable building for 32bit ARM for now, I have created a bug report for this.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #11 from Robert robert.de.rooy@gmail.com --- I have not had time to provide updates. But I solved the issue of DOSBox-X not building on some architectures upstream. With this, I can build the RPM on koji for i686, x86_64, arm, aarch64, ppc and s390x.
A new version of DOSBox-X with this patch included should be out on or around the 1st of March, and I will send an updated review request then.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #12 from Hans de Goede hdegoede@redhat.com --- (In reply to Robert from comment #11)
I have not had time to provide updates. But I solved the issue of DOSBox-X not building on some architectures upstream. With this, I can build the RPM on koji for i686, x86_64, arm, aarch64, ppc and s390x.
A new version of DOSBox-X with this patch included should be out on or around the 1st of March, and I will send an updated review request then.
Sounds good, thank you.
I'll try to make some time to do a full review when you've posted the new version.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #13 from Robert robert.de.rooy@gmail.com --- As mentioned previously, I have updated the packages to the newly released 0.83.11 which fixes building on some architectures.
Here are the spec and srpm files https://github.com/rderooy/dosbox-x-rpm
F33 build: https://koji.fedoraproject.org/koji/taskinfo?taskID=62899097 F34 build: https://koji.fedoraproject.org/koji/taskinfo?taskID=62899544
Unfortunately a new issue is preventing it from building on s390x, so I had to exclude it. I will try to get this fixed upstream and have already opened an issue for it on the dosbox-x gitlab.
I would like to request another review at this point.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #14 from Robert robert.de.rooy@gmail.com --- We have a fix for the issue preventing building on s390x, but I'm not sure it is worth including a patch in the RPM. It should be fixed in the next monthly release.
It is not like people are likely to run a DOS emulator on a mainframe.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #15 from Hans de Goede hdegoede@redhat.com --- Please do add the fix for the s390x build-issue as a patch for now. Otherwise you need to add an ExcludeArch to the .spec file and we have a bunch of red-tape surrounding tracking ExcludeArch-s to avoid them being abused and to allow people who care about more niche architectures to check what is not available on their arches and why. See: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_architecture_bui...
Adding the patch (as a downstream patch for now) will avoid all the red-tape surrounding this which really is much easier.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #16 from Robert robert.de.rooy@gmail.com --- Ok, done. This includes a small patch to fix the s390x build issue. There are no ExclusiveArch or ExcludeArch statements in the spec file any longer.
SPEC and SRPM: https://github.com/rderooy/dosbox-x-rpm
F33 build: https://koji.fedoraproject.org/koji/taskinfo?taskID=63196099 F34 build: https://koji.fedoraproject.org/koji/taskinfo?taskID=63196166
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
Otto Urpelainen oturpe@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |oturpe@iki.fi Flags| |needinfo?(hdegoede@redhat.c | |om)
--- Comment #17 from Otto Urpelainen oturpe@iki.fi --- @hdegoede@redhat.com are you still planning to take this for full review? I could also take over if you do not intend to complete the review.
Regarding packaging, I think the upstream tarball is not suitable as a source entry in the specfile. Extracted, it is a > 200 MiB beast with lots of stuff that is not needed for Fedora packaging at all, at least (sub)directories called vs2015, windows and macos, probably others too. I have been told that Calibre package [1] performs similar actions, it could be used as a base for this.
[1]: https://src.fedoraproject.org/rpms/calibre
Licensing-wise, I doubt that the license really is GPLv2 or GPLv2+. fedora-review gave me this:
Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "GNU General Public License, Version 2", "*No copyright* GNU General Public License, Version 2", "GNU General Public License v2.0 or later", "GNU General Public License v2.0 or later [generated file]", "GNU General Public License v3.0 or later", "Expat License [generated file]", "GNU General Public License", "zlib/libpng license", "GNU Library General Public License v2 or later", "GNU Lesser General Public License v2.1 or later", "BSD 2-clause "Simplified" License", "*No copyright* Do What The Fuck Youither Want To Public License, Version 2", "Expat License", "ISC License", "BSD 3-clause "New" or "Revised" License", "*No copyright* GNU General Public License", "*No copyright* GNU General Public License v2.0 or later", "FreeType License", "Public domain", "[generated file]", "FSF Unlimited License (with Retention) [generated file]", "FSF Unlimited License [generated file]", "*No copyright* Public domain", "Libpng License", "BSD 4-clause "Original" or "Old" License", "GNU Lesser General Public License, Version 2.1", "*No copyright* GNU Library General Public License, Version 2.0", "*No copyright* GNU Lesser General Public License", "*No copyright* zlib/libpng license", "GNU Lesser General Public License v2.1 or later [obsolete FSF postal address (Temple Place)]", "the Unlicense", "GNU Library General Public License v2 or later [obsolete FSF postal address (Temple Place)]", "the Unlicense Expat License", "FreeType License [generated file]", "NTP License [generated file]", "FSF Unlimited License (with Retention) GNU General Public License, Version 2", "FSF Unlimited License (with Retention)", "FSF Unlimited License (with Retention) GNU General Public License v2.0 or later", "*No copyright* [generated file]", "zlib/libpng license Khronos License", "zlib/libpng license Expat License", "Khronos License", "*No copyright* SGI Free Software License B", "FSF Unlimited License (with Retention) GNU General Public License, Version 2 [generated file]", "BSD 4-clause "Original" or "Old" License GNU General Public License v2.0 or later", "Apache License 2.0", "NTP License", "GNU Lesser General Public License", "GNU Library General Public License, Version 2.0", "Public domain GNU Lesser General Public License v2.1 or later", "zlib/libpng license [generated file]", "zlib/libpng license Public domain", "zlib/libpng license NTP License", "*No copyright* Boost Software License 1.0", "Boost Software License 1.0", "Apple MIT License GNU Library General Public License v2 or later", "Expat License GNU Lesser General Public License v2.1 or later", "GNU General Public License, Version 3", "*No copyright* Common Public License 1.0". 3105 files have unknown license.
Not all of them end up in the binary rpm, some are only found inside vs2015 directory, for example, but the situation should be checked. It also seems that some licenses that require attaching the full license text are not respected in that manner (note that this needs to be handled for source rpm even if the file is not used for binary rpm, because the source rpm is distributed by Fedora, too). Probably it would be the best to first create a smaller source tarball as suggested above, so that there would be less files and licenses to check.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
Hans de Goede hdegoede@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(hdegoede@redhat.c | |om) |
--- Comment #19 from Hans de Goede hdegoede@redhat.com ---
@hdegoede@redhat.com are you still planning to take this for full review? I could also take over if you do not intend to complete the review.
I have my hands full with upstream kernel maintenance work, so if you can take over this review that would be great, go for it.
Note that Robert does need a sponsor. If you cannot sponsor, then I can take-over once you are happy with this, do a final review and then sponsor Robert. If you can sponsor yourself, that would be even better :)
As for your remarks about shrinking the source-tarbal, yes that would probably be good. I also agree that it would be good to double check the license situation, although I don't expect any issues to come up, you never know.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
Otto Urpelainen oturpe@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|nobody@fedoraproject.org |oturpe@iki.fi Flags| |fedora-review? | |needinfo?(robert.de.rooy@gm | |ail.com)
--- Comment #20 from Otto Urpelainen oturpe@iki.fi --- Ok, I took this review. Unfortunately I am not able to sponsor Robert, as I am quite recent contributor myself. We will get back to you when the review is otherwise complete.
@robert.de.rooy@gmail.com there seems to be agreement that creating a slimmed down source tarball is a good idea. Please see what you can do. Otherwise, I will continue the review as soon as possible.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
Robert robert.de.rooy@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(robert.de.rooy@gm | |ail.com) |
--- Comment #21 from Robert robert.de.rooy@gmail.com --- @oturpe@iki.fi
Thanks for your review so far.
Just to clarify. I am a occasional contributor of the DOSBox-X project, but am not one of the main contributors/developers. I was asked if I could do this review request as I am a Fedora user, and have some packaging experience. I also did the flatpak.
As to the issues raised, I will relay them back to the main developers.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #22 from Otto Urpelainen oturpe@iki.fi --- Note that the source stripping can be done without changes upstream. If you check the correct old commit of Calibre package [1] (incorrectly linked above, my mistake, I am sorry for that) I linked before, you will find the following scheme:
1. The spec file refers to a source tarball that is a stripped down version of the upstream tarball 2. The is a comment in spec file explaining which script needs to be run the create the stripped down tarball 3. Said script in included in the repository
Similar scheme could be used here to transform upstream sources to smaller source to be used as a starting point for Fedora packaging. Of course, it would be great for Fedora if upstream provided a tarball without bundled dependencies or material specific to other OSes or build systems Fedora does not use — but there is also the method described above.
I can also suggest yet another, perhaps simpler way to get started with stripping: If unnecessary files can be removed like this in the %prep section:
%prep rm -rf vs2015 # more removals
After that, the removed files do not exist for the rest of processing, and fedora-review is also smart enough to not consider when checking for licenses. That way, you could find a suitably simple base. Afterwards, the source stripping script described above could be developed to make the source rpm smaller and simplify the licensing situation there also.
I hope this explanation makes sense. Feel free to ask further questions if anything is unclear.
[1]: https://src.fedoraproject.org/rpms/calibre/tree/fea70390a1a085be6155894425c3...
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
Otto Urpelainen oturpe@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED
--- Comment #23 from Otto Urpelainen oturpe@iki.fi --- Some findings from review. This list is not exhaustive, since I did not have time to go through everything yet, and there were also some items from fedora-review that I still have to look up (everybody is learning something here):
- Patch should have link to upstream fix, or comment explaining why it is required but cannot be upstreamed. Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/PatchUpstreamStatu...
Patch: s390x-mem.patch
- Customarily, changelog entries have a blank line between them (not required as such, but that is what is commonly done). Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/#changelogs
- For the .desktop file, either desktop-file-install (in %install) OR desktop-file-validate (in %check or %install) must be run. Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_desktop_files
- AppStream .metainfo.xml must be validated with appstream-util validate-relax (in %check or %install). Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/AppData/
- Description and summary should be available in all supported languages. I see some translations in the .desktop file, do those count as "supported languages"? Reference: Unclear where this comes from, I cannot find this is Packaging Guidelines, still fedora-review lists this as a SHOULD item.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #24 from Hans de Goede hdegoede@redhat.com ---
- Description and summary should be available in all supported languages. I see some translations in the .desktop file, do those count as "supported languages"? Reference: Unclear where this comes from, I cannot find this is Packaging Guidelines, still fedora-review lists this as a SHOULD item.
I've always interpreted this as "don't remove translations" (when using say an upstream provided specfile as base) there is really no need to add translations to spec-files. Esp. since now a days all the GUI tools don't care about the spec-file Summary / description anyways. In e.g. gnome-software the summary comes from the .desktop and/or the appdata file and the description from the appdata.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #25 from Otto Urpelainen oturpe@iki.fi --- (In reply to Hans de Goede from comment #24)
- Description and summary should be available in all supported languages. I see some translations in the .desktop file, do those count as "supported languages"? Reference: Unclear where this comes from, I cannot find this is Packaging Guidelines, still fedora-review lists this as a SHOULD item.
I've always interpreted this as "don't remove translations" (when using say an upstream provided specfile as base) there is really no need to add translations to spec-files. Esp. since now a days all the GUI tools don't care about the spec-file Summary / description anyways. In e.g. gnome-software the summary comes from the .desktop and/or the appdata file and the description from the appdata.
I get the feeling that the Review Guidelines are outdated regarding this topic. Robert, do not bother with spec file translations. I will see later if something can be done to clarify the guidelines regarding this.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #26 from Hans de Goede hdegoede@redhat.com --- (In reply to Otto Urpelainen from comment #25)
I get the feeling that the Review Guidelines are outdated regarding this topic.
I agree.
Robert, do not bother with spec file translations.
Ack.
I will see later if something can be done to clarify the guidelines regarding this.
Great, thank you for looking into this.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #27 from Robert robert.de.rooy@gmail.com --- A few answers...
Regarding the patch, I will keep it in mind for the future. Yes the patch was submitted upstream and merged.
For the .desktop and .metainfo.xml files, I have updated the spec file to do the validation and have submitted the changes upstream.
A new DOSBox-X release is due in a few days, and I will update the SRPM then. In the meantime, I have placed an updated spec file online with the changes.
https://github.com/rderooy/dosbox-x-rpm/blob/master/dosbox-x.spec
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
Otto Urpelainen oturpe@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(robert.de.rooy@gm | |ail.com)
--- Comment #28 from Otto Urpelainen oturpe@iki.fi --- Great, the items from previous batch are now all resolved. Here are two more:
- This would be better with %{_sbindir} macro here instead of hard coded /usr/sbin. Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_macros
if [ -x /usr/sbin/setcap ]; then
- Man page should be installed. I see that you have added it upstream, is it just accidentally missing from %install? Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_manpages
I only have a couple of items remaining, the big two being:
1. Licenses, already discussed before. I will attach the list of licenses fedora-review found; there is more than GPLv2+ even if vs2015 is not counted. Could you check that and find out, what actually should be written to License field, following https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuideline... This will be easier if the source rpm minification is done first, because there will be less clutter from unused files.
2. Bundled dependencies. To me, src/libs looks like collection of bundled libraries. Could you check the content of src/libs against guidelines at https://docs.fedoraproject.org/en-US/packaging-guidelines/#bundling and comment?
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #29 from Otto Urpelainen oturpe@iki.fi --- Created attachment 1767183 --> https://bugzilla.redhat.com/attachment.cgi?id=1767183&action=edit licensecheck.txt
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
Robert robert.de.rooy@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(robert.de.rooy@gm | |ail.com) |
--- Comment #30 from Robert robert.de.rooy@gmail.com --- The man page is not in the linked spec file, because it is a spec file for 0.83.11. The man page was added after, and will be part of 0.83.12, which should be out in the next few days.
I will make the update for %{_sbindir}, and will ask the main developers to comment on the two big items.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #31 from Robert robert.de.rooy@gmail.com --- Here is a comment on the licenses from one of the developers (Wengier):
About licensing - some libraries are released under the zlib/libpng license, Apache license, BSD license, LGPL license, Expat license, etc. Most (if not all) of them appear to be compatible with either GNU GPL v2 or v3. The vs2015 directory may not need to be counted by the way.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #32 from Otto Urpelainen oturpe@iki.fi --- The process is regretfully long, but we are making progress, which is great. I found two more items:
- Because the binary is given a capability, it must be compiled to use position independent by adding '%global _hardened_build 1'. Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_pie
- Directory hierarchy for the icon starting at /usr/share/icons/hicolor must be explicitly imported from the package that owns in by adding 'Requires: hicolor-icon-theme'. Reference: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_file_and_directo... which I found unclear, so asked in the mailing list, too: https://lists.fedoraproject.org/archives/list/packaging@lists.fedoraproject....
In other news, I actually installed the package on F34 system and can confirm that it works. From the 4 games I tried, only Settlers 2 was unplayable due to mouse not working. So in general it works, great!
Regarding licenses, it is really good that upstream is involved with getting it right for Fedora. GPL compatibility is also an absolute must if dosbox-x comes with a GPL license, so it is good that it has been asserted already — but it is not the only condition that is found in various licenses. All the conditions the author has put on their work have to be fulfilled, otherwise including the software in Fedora would be a copyright violation. I will give an example, reference for this are the Licensing Guidelines https://docs.fedoraproject.org/en-US/packaging-guidelines/LicensingGuideline...
Original work from dosbox-x is licensed under GPLv2+, so the specfile license field will contain at least that.
I picked one item from attachment licensecheck.txt that is listed as having something else than GPLv2+:
*No copyright* Do What The Fuck You Want To Public License, Version 2 --------------------------------------------------------------------- dosbox-x-dosbox-x-v0.83.11/include/whereami.h dosbox-x-dosbox-x-v0.83.11/src/gui/whereami.c
Checking those files, I find out that it is a bundled library with a copyright notice saying it is originally from https://github.com/gpakosz/whereami and is dual licensed under WTFPL and MIT licenses. So now License is at least (GPLv2+ and (WTFPL or MIT)).
Then to licensing conditions. The only condition of MIT is this: "The above copyright notice and this permission notice shall be included in all copies or substantial portions of the Software." This is fulfilled by adding the MIT license to the package through the %license field. Checking the Licensing Guidelines, there is a discouraged alternative path, but basically the rule is that the license has to be copied from upstream. So, upstream has to be contacted and asked to add the full license text (which can be copied from the whereami repo).
On the other hand, WTFPL does not have any restrictions at all, so including the license text is not required. Still, the Licensing Guidelines have a SHOULD item about asking upstream to add it, just to remove any doubt regarding what the license really is. Probably it can be added when the MIT case is resolved without any extra effort.
This procedure has to be repeated for all the content that gets compiled into or otherwise included in the rpm. Details way depending on the exact licenses.
(Note that in the particular case of gpakosz/whereami, upstream could leverage the great freedom of WTFPL and simply relicense the whole thing under GPLv2+ and their own name. Complication removed. This may not be an option for other files with different licenses. In theory Fedora could also do the relicensing, the guidelines do not say anything about it, we could ask in the mailing lists — but my hunch is that the answer would be to always use the same license upstream has and fix possible hurdles upstream.)
I am also not sure how the resulting combined license for dosbox-x executable should be packaged. Can multiple license files be packaged? Can the multiple licenses be concatenated with a script in the specfile? Or does Fedora only accept a single file that is maintained upstream and copied verbatim? Most probably we have to find an answer to that question before this review is complete. but before doing that, I would really like to first follow the two paths that are available for simplyfying the situation: source minification (remove clutter so it is easier to understand what content is really needed and how it is licensed) and library unbundling (breaking the monolith to multiple small and simple packages — many of which hopefully already exist in Fedora).
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #33 from Robert robert.de.rooy@gmail.com --- A new dosbox-x version was released, and I have just found a moment to package it up.
You can find the updated spec and srpm file here: https://github.com/rderooy/dosbox-x-rpm
F34 build: https://koji.fedoraproject.org/koji/taskinfo?taskID=65172729
Spec file changes: - Removed patch as the issue was fixed upstream - Includes the MAN page - adds "BuildRequires: libslirp-devel" (needed for a new optional NE2000 back-end) - adds "Requires: hicolor-icon-theme" - adds "%global _hardened_build 1"
I'm not sure about the last one being necessary, based on this: https://fedoraproject.org/wiki/Changes/Harden_All_Packages
Regarding some games perhaps not working, this is likely because some games require special settings to get working. That or perhaps something broke in dosbox-x...
Regarding the licenses, I reported it, but I don't have any update at this point.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
Gregory PAKOSZ gregory.pakosz+redhat@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |gregory.pakosz+redhat@gmail | |.com
--- Comment #34 from Gregory PAKOSZ gregory.pakosz+redhat@gmail.com --- Stumbled upon this ticket: does it help if I add a license to whereami?
Best, Gregory
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #35 from Gregory PAKOSZ gregory.pakosz+redhat@gmail.com --- Though it shouldn't be necessary: IANAL but WTFPL v2 is supposed to be compatible with GPL v2 and v3. It's at least listed as such by Fedora: https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#Good_Licenses
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #36 from Robert robert.de.rooy@gmail.com --- A breakage was found in the latest release version which effects DOS4GW games, and possibly others. So I included a patch, and have bumped the release to 0.83.12-2 The patch is already merged upstream.
You can find the updated spec and srpm file here: https://github.com/rderooy/dosbox-x-rpm
F34 build: https://koji.fedoraproject.org/koji/taskinfo?taskID=65216496
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #37 from Otto Urpelainen oturpe@iki.fi --- (In reply to Gregory PAKOSZ from comment #35)
Though it shouldn't be necessary: IANAL but WTFPL v2 is supposed to be compatible with GPL v2 and v3. It's at least listed as such by Fedora: https://fedoraproject.org/wiki/Licensing:Main?rd=Licensing#Good_Licenses
Hi Gregory,
That is right, whereami's license is ok for Fedora, either of the given options would work actually. The problem I have with the licenses is not that dosbox-x could not be included in Fedora. It is just that the licenses should be listed correctly in specfile Licenses field, and the conditions for each license fulfilled.
A very common case is that popular permissive licenses like MIT, BSD, APL2 all require distributing the original copyright and permissions notices with the source or compiled program. When a project bundles it dependencies like dosbox-x does, this results in requirement to include a lot of those notices. As an example, consider the Visual Studio Code notices file (from some old version, the current one is much longer): https://gist.github.com/dm/e5581d6c37b408c819ec
Another example would be this Oracle HTTP Server docs page, fulfilling the license conditions of open source software they have used: https://docs.oracle.com/cd/B14117_01/server.101/b12255/license.htm
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
Otto Urpelainen oturpe@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(robert.de.rooy@gm | |ail.com)
--- Comment #38 from Otto Urpelainen oturpe@iki.fi --- Thank you for the new specfile. I went through it. Findings that I missed earlier:
- You are right about hardened build being the default. I apologize, my mistake. Adding the line about hardening should not be required. I sent a pull request for the Guidelines about this topic: https://pagure.io/packaging-committee/pull-request/1064
- I think these files are actually documentation and it would be better to install them with %doc so they would go to documentation directory instead of data (assuming that there are not actually needed at runtime):
/usr/share/dosbox-x/CHANGELOG /usr/share/dosbox-x/dosbox-x.reference.conf /usr/share/dosbox-x/dosbox-x.reference.full.conf
- I see the following in build.log, perhaps you want to check if the capability setting script actually achieved its purpose?
test -x /usr/sbin/setcap && setcap cap_net_raw=ep /builddir/build/BUILDROOT/dosbox-x-0.83.12-2.fc35.x86_64/usr/bin/dosbox-x unable to set CAP_SETFCAP effective capability: Operation not permitted make: [Makefile:906: install] Error 1 (ignored)
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
Robert robert.de.rooy@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(robert.de.rooy@gm | |ail.com) |
--- Comment #39 from Robert robert.de.rooy@gmail.com --- Thanks Otto,
I will in the next spin (might not be until the weekend...) remove the hardening and classify those files as doc.
As to setcap, yes I know it gives that ignored error message during build, but it absolutely does do it during the RPM install.
$ getcap /usr/bin/dosbox-x /usr/bin/dosbox-x cap_net_raw=ep
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #40 from Robert robert.de.rooy@gmail.com --- I have done a new build with the suggested %doc packaging changes.
You can find the updated spec and srpm file here: https://github.com/rderooy/dosbox-x-rpm F34 build: https://koji.fedoraproject.org/koji/taskinfo?taskID=65817372
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #41 from Robert robert.de.rooy@gmail.com --- I made another minor change, I replaced the setcap command with the %caps macro.
You can find the updated spec and srpm file here: https://github.com/rderooy/dosbox-x-rpm F34 build: https://koji.fedoraproject.org/koji/taskinfo?taskID=65836560
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #42 from Otto Urpelainen oturpe@iki.fi --- Thank you for %doc and %caps changes, much better. I did not even know about %caps before this.
The files that now go to /usr/share/doc/dosbox-x still go to /usr/share/dosbox-x, though. It is not serious, but worth noting here anyhow.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #43 from Robert robert.de.rooy@gmail.com --- %caps is poorly documented in my opinion. I just thought to check how ping was packaged, since I knew it also used caps.
In any case, a new release (0.83.13) is out and I just build it.
You can find the updated spec and srpm file here: https://github.com/rderooy/dosbox-x-rpm F34 build: https://koji.fedoraproject.org/koji/taskinfo?taskID=67020063
No real packaging changes though.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #44 from Hans de Goede hdegoede@redhat.com --- Robert, thank you for all your work on this.
Otto, thank you for all your input on this. Do you think this is ready for me to review now (and sponsor Robert assuming the review goes well) or do you still see issues which need addressing ?
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
--- Comment #45 from Otto Urpelainen oturpe@iki.fi --- Blocking items are:
1. Marking all bundled dependencies as such and 2. Setting the License field and installed license file(s) to match actual source content and to comply with Licensing Guidelines
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
Otto Urpelainen oturpe@iki.fi changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |NEW Assignee|oturpe@iki.fi |nobody@fedoraproject.org Flags|fedora-review? |
--- Comment #46 from Otto Urpelainen oturpe@iki.fi --- There has been no progress with this package for a long time. Since I still see serious problems with bundled dependencies and licensing, I will drop this review on my part. Sorry, and thank you for all the effort that was put into this.
Hans, fell free to continue if you wish.
https://bugzilla.redhat.com/show_bug.cgi?id=1919639
Robert robert.de.rooy@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |DEFERRED Status|NEW |CLOSED Last Closed| |2022-03-09 12:57:08
--- Comment #47 from Robert robert.de.rooy@gmail.com --- For anyone finding this issue, there is a copr repo available here: https://copr.fedorainfracloud.org/coprs/rob72/DOSBox-X/
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com