Skip to content
  • Elliott Sales de Andrade @QuLogic ·

    Some thoughts:

    • Group is not used in Fedora, and Games/Other is not a valid group either.
    • Missing license on the server package.
    • Seeing this message: libudev development libraries not found, disabling udev support even though udev=yes, so probably a missing BR.
    • Why is the game runner not release_debug?
    • Would be nicer if upstream provided the desktop file instead, but you should use desktop-file-install for it in the meantime.
    • Relatedly, there will need to be an AppData file so that it shows up in GNOME Software, etc.
    • Add -p to install.
    • Are there some tests that can be run (maybe with xvfb)?
    • Is there a reason for the ExclusiveArch?
    Edited by Elliott Sales de Andrade
  • Rémi Verschelde @akien ·

    Thanks for the first review.

    • Group is not used in Fedora, and Games/Other is not a valid group either.
    • Missing license on the server package.
    • Add -p to install.

    Fixed, though for adding -p to install I don't really see the use case, as those are compiled binaries and it's not particularly relevant to the user at what time they were built before being moved to the buildroot a couple minutes later... Or do I miss something?

    • Seeing this message: libudev development libraries not found, disabling udev support even though udev=yes, so probably a missing BR.

    There's BuildRequires: pkgconfig(udev), and it's then checked with pkg-config --exists libudev. It works fine on Mageia - does the pkgconfig file differ on Fedora?

    Why is the game runner not release_debug?

    That's because release_debug is the debug mode (-DDEBUG) with some optimisations which is used by default for the tools (editor) build to provide debugging features (and the debug target is not optimised, and has more debug symbols).

    For the runner, we actually package the Linux X11 export template in release mode release so that it's optimized. Building the runner with release_debug would be the equivalent of the debug templates which are only meant for testing, not for use in production.

    Then the misnomer debug_release=yes (recently renamed to debug_symbols=yes in the master branch) adds the necessary debuginfo that RPM will strip after the build.

    • Would be nicer if upstream provided the desktop file instead, but you should use desktop-file-install for it in the meantime.
    • Relatedly, there will need to be an AppData file so that it shows up in GNOME Software, etc.

    Right, I guess I can add those to misc/dist/linux and we can install them from there.

    Fixed current spec to use desktop-file-install, though I tend to find it redundant when install does exactly the same thing and is more common. But since we BR desktop-file-utils for validation anyway, why not.

    Are there some tests that can be run (maybe with xvfb)?

    Not really. There are a few tests but they're not really maintained and cover only a very small subset of what should be tested to have some real test coverage. Not worth bothering for now IMO.

    Is there a reason for the ExclusiveArch?

    No strong one, apart from the fact that we only support those two arches upstream for Linux X11. But it might be able to build on armv7hl, I've read that someone built Godot 2.1.4 successfully on a RPi. So I guess we could try without the ExclusiveArch and see what fails.

    Edited by Rémi Verschelde
  • Rémi Verschelde @akien ·

    BTW, I'll cherry-pick your OpenSSL 1.1.0 compat patch to the 2.1 branch, so that we can use it for packaging.

  • Elliott Sales de Andrade @QuLogic ·

    There's BuildRequires: pkgconfig(udev), and it's then checked with pkg-config --exists libudev.

    Those are not the same, though?

  • Rémi Verschelde @akien ·

    There's BuildRequires: pkgconfig(udev), and it's then checked with pkg-config --exists libudev.

    Those are not the same, though?

    On Mageia they are:

    $ urpmq --provides lib64udev-devel
lib64udev-devel: devel(libudev(64bit))
lib64udev-devel: lib64udev-devel[== 230-12.mga6]
lib64udev-devel: lib64udev-devel(x86-64)[== 230-12.mga6]
lib64udev-devel: lib64udev0-devel[== 230-12.mga6]
lib64udev-devel: libudev-devel[== 230-12.mga6]
lib64udev-devel: pkgconfig(libudev)[== 230]
lib64udev-devel: pkgconfig(udev)[== 230]
lib64udev-devel: udev-devel[== 230-12.mga6]
lib64udev-devel: devel(libudev(64bit))
lib64udev-devel: lib64udev-devel[== 230-12.1.mga6]
lib64udev-devel: lib64udev-devel(x86-64)[== 230-12.1.mga6]
lib64udev-devel: lib64udev0-devel[== 230-12.1.mga6]
lib64udev-devel: libudev-devel[== 230-12.1.mga6]
lib64udev-devel: pkgconfig(libudev)[== 230]
lib64udev-devel: pkgconfig(udev)[== 230]
lib64udev-devel: udev-devel[== 230-12.1.mga6]

    But the file itself is /usr/lib64/pkgconfig/libudev.pc, so I guess we can use pkgconfig(libudev) in the spec.

    Edited by Rémi Verschelde
  • Elliott Sales de Andrade @QuLogic ·

    On Fedora:

    $ dnf repoquery --whatprovides 'pkgconfig(udev)'
Last metadata expiration check: 0:00:13 ago on Sun 24 Sep 2017 03:48:49 AM EDT.
systemd-0:233-6.fc26.i686
systemd-0:233-6.fc26.x86_64

$ dnf repoquery --whatprovides 'pkgconfig(libudev)'
Last metadata expiration check: 0:00:25 ago on Sun 24 Sep 2017 03:48:49 AM EDT.
systemd-devel-0:233-6.fc26.i686
systemd-devel-0:233-6.fc26.x86_64

    Not sure why the difference, but they do seem to be in separate packages.

    Edited by Elliott Sales de Andrade
  • Rémi Verschelde @akien ·

    I've added the desktop file upstream and wrote an AppStream file too: https://github.com/godotengine/godot/commit/893ebc54fceae13fe7a0427be5a11601ef30e601

    I've edited the spec file to cherry-pick this commit and the OpenSSL 1.0 one (git format-patch -2 on current 2.1 branch).

  • Rémi Verschelde @akien ·

    I now wrote a man page too.

    Patches:

  • Rémi Verschelde @akien ·

    I'm building a variation of this spec for Mageia (without all the Fedora-specific checks and cache updates, etc., since Mageia has filetriggers handling that). If it works I'll try this spec in a COPR for Fedora and RHEL 7.

  • Rémi Verschelde @akien ·

    Some further fixes:

    • Add conditional support for Mageia to play with on COPR - for the official Fedora/RHEL package we can remove it though, to reduce clutter
    • Fix support for RHEL whose clang breaks on -fstack-protector-strong (rhbz#1099282). Another alternative would be to build with gcc instead but I would have had to make the .llvm suffix conditional and I didn't want to bother :p
    • Add --nonet to the appstream-util call, otherwise it fails
  • Rémi Verschelde @akien ·

    The current version seems to build fine: https://copr.fedorainfracloud.org/coprs/akien/godot/build/607393/

    (Don't mind the failed Mageia Cauldron build, it's a mirror issue).

  • Rémi Verschelde @akien ·

    Last thing to pre-review IMO before making an official review request on Bugzilla would be this:

    License: MIT

    Is it find to define it this way as "that's the main license of the project, see licenses files for details", or should we list explicitly every single license of the non-unbundled thirdparty libraries and code snippet?

  • Rémi Verschelde @akien ·

    should we list explicitly every single license of the non-unbundled thirdparty libraries and code snippet?

    If so, I guess that would be:

    License: MIT and CC-BY and ASL 2.0 and BSD and zlib and OFL and FTL and ISC

    There's also a snippet from libcurl which has its own identifier in SPDX, but I don't find what it should be in the Fedora Licensing policy.

    Edit: Well Fedora's own curl package claims to be MIT, while the curl license is a kind of custom "MIT with advertising" mix... I won't pick up a fight with fedora-legal, if they say curl is MIT we don't need to bother about the license of our small hostcheck.c snippet.

    Also this one which I can't find in the Fedora page:

    Files: ./thirdparty/misc/md5.cpp
 ./thirdparty/misc/md5.h
Comment: MD5 Message Digest Algorithm
Copyright: 1990, RSA Data Security, Inc.
License: RSA-MD

    But it seems we don't need to worry about this one: https://fedoraproject.org/wiki/User:Toshio/Licensing_FAQ#What_about_the_RSA_license_on_their_MD5_implementation.3F_Isn.27t_that_GPL-incompatible.3F

    Edited by Rémi Verschelde
  • Rémi Verschelde @akien ·

    I think the current spec file is ready for a formal Review Request, WDYT?

  • Neal Gompa (ニール・ゴンパ) @Conan_Kudo ·

    @akien If the vendored third-party libraries aren't in use, you shouldn't need to worry too much. Any vendored libraries need to be declared with Provides: bundled(), per https://fedoraproject.org/wiki/Bundled_Libraries?rd=Packaging:Bundled_Libraries#Requirement_if_you_bundle

    And while the Group tag is considered obsolete in Fedora, you are allowed to define them if you want. It certainly doesn't hurt anything. :)

    I would also suggest using gcc-c++ on (0%{?rhel} && 0%{?rhel} < 8) || (0%{?mageia} && 0%{?mageia} < 7), as the GCC version is compatible there.

  • Rémi Verschelde @akien ·

    @Conan_Kudo Well some of the vendored libraries stay bundled:

    $ ls -l thirdparty/
b2d_convexdecomp/  certs/  fonts/  jpeg-compressor/  libmpcdec/  minizip/  misc/  pvrtccompressor/  README.md  rg-etc1/  rtaudio/  speex/  squish/

    (The unbundled ones for now are

    • b2d_convexdecomp, jpeg-compressor, pvrtcompressor, rg-etc1 don't exist as distro packages, and also wouldn't make much sense as shared libraries (provided they could be packaged as such).
    • certs is a certificate bundle, presumably taken from Ubuntu's rootcerts a while ago... It can and should be unbundled (as well as updated in git, with its provenance better documented), but I haven't had time to work on it.
    • fonts is DroidSans{,Arabic,Fallback,Hebrew,Japanese,Thai}.ttf + source_code_pro.otf, albeit (IIRC) with some custom modifications to cut their size down while keeping the glyphs we want to support - they eventually get compiled into the binary and not installed as duplicated fonts in /usr/share, so I'm not sure it's worth the pain to unbundle them. But could be doable if there's interest.
    • libmpcdec could be unbundled, but that would require more upstream work as the current version is an ancient SVN revision, and the current libmpcdec releases have a completely different API. As it's dropped in master, I haven't spent time on it. But it could be done if there's interest, it would just take quite a while.
    • minizip has sadly custom changes: https://github.com/godotengine/godot/blob/2.1/thirdparty/minizip/godot-zlib-1.2.4-minizip-seek.patch https://github.com/godotengine/godot/blob/2.1/thirdparty/minizip/godot-zlib-1.2.4-minizip-unbreak-gentoo.patch
    • rtaudio is for Windows. I guess I could also rm -rf it.
    • speex could be unbundled I guess, not sure why I haven't done it. Probably because it was already dropped in the master branch when I worked on it and later cherry-picked changes to the 2.1 branch. I'll have another look.
    • squish is not packaged in Fedora nor Mageia

    Actually for libmpcdec and speex, an easy way out is to just remove them and disable the eponymous modules that use them.

    As for misc, it's:

    $ ls thirdparty/misc/
aes256.cpp  base64.c  curl_hostcheck.c  fastlz.c  hq2x.cpp  md5.cpp  mikktspace.c  sha256.c  smaz.c  stb_truetype.h    triangulator.h
aes256.h    base64.h  curl_hostcheck.h  fastlz.h  hq2x.h    md5.h    mikktspace.h  sha256.h  smaz.h  triangulator.cpp  yuv2rgb.h

    There's not much here that would be usually packaged in distros, most projects bundle such minimal implementations of aes256, base64, md5 or sha256 as needed. Adding the whole libcurl as a dependency to unbundle the very small snippet which was extracted from its source would also be overkill.

    So yeah, there's still some stuff that could be done, but I'll admit that there's a limit to how much work I'm ready to do for it to be fully perfect. I've already spent weeks of work sanitizing all this, unbundling stuff and documenting licensing :)

    As for using GCC on RHEL7, why not, but unless there's a strong reason for using it I prefer to stay consistent with Fedora.

  • Rémi Verschelde @akien ·

    Alright, did some changes:

    • Removed rtaudio source files in %prep
    • Added some bundled() provides
    • Dropped Mageia compat code, I don't want the spec to become needlessly bloated (and Godot 2.1.4 is already packaged in Mageia)
    • Made clang build conditional, reverting to gcc-c++ for RHEL 7
  • Neal Gompa (ニール・ゴンパ) @Conan_Kudo ·

    @akien Bundling isn't a problem, just they need to be documented as bundled() Provides. It shouldn't be that difficult. :)

  • Neal Gompa (ニール・ゴンパ) @Conan_Kudo ·

    @akien: LGTM.

  • Rémi Verschelde @akien ·

    Fixed a couple C++-induced typos (s/%ifdef/%if/g) and removed the -fstack-protector-strong hack on RHEL7 since we're building with GCC < 6.

    COPR build: https://copr.fedorainfracloud.org/coprs/akien/godot/build/607541/

  • Elliott Sales de Andrade @QuLogic ·

    The Provides: bundled() may need to go on a couple of the subpackages as well.

    • b2d_convexdecomp, jpeg-compressor, pvrtcompressor, rg-etc1 don't exist as distro packages, and also wouldn't make much sense as shared libraries (provided they could be packaged as such).

    I'm not sure what it's used for, so I could be wrong, but would libjpeg(-turbo) be useful there? Both as more standard and perhaps more optimized (maybe).

    • certs is a certificate bundle, presumably taken from Ubuntu's rootcerts a while ago... It can and should be unbundled (as well as updated in git, with its provenance better documented), but I haven't had time to work on it.

    Having worked on Pidgin, I know you don't really want to be in the business of maintaining certificate authority lists...

    • fonts is DroidSans{,Arabic,Fallback,Hebrew,Japanese,Thai}.ttf + source_code_pro.otf, albeit (IIRC) with some custom modifications to cut their size down while keeping the glyphs we want to support > - they eventually get compiled into the binary and not installed as duplicated fonts in /usr/share, so I'm not sure it's worth the pain to unbundle them. But could be doable if there's interest.

    Maybe still needs a bundled() provide; not sure about fonts.

    • libmpcdec could be unbundled, but ... dropped in master
    • speex could be unbundled, ... because it was already dropped

    If it's going to be dropped, why bother.

    As for misc, it's:

    $ ls thirdparty/misc/
aes256.cpp  base64.c  curl_hostcheck.c  fastlz.c  hq2x.cpp  md5.cpp  mikktspace.c  sha256.c  smaz.c  stb_truetype.h    triangulator.h
aes256.h    base64.h  curl_hostcheck.h  fastlz.h  hq2x.h    md5.h    mikktspace.h  sha256.h  smaz.h  triangulator.cpp  yuv2rgb.h

    Some of these are provided by OpenSSL, I think? But I see you've worked out the licensing stuff already.

  • Rémi Verschelde @akien ·
    • b2d_convexdecomp, jpeg-compressor, pvrtcompressor, rg-etc1 don't exist as distro packages, and also wouldn't make much sense as shared libraries (provided they could be packaged as such).

    I'm not sure what it's used for, so I could be wrong, but would libjpeg(-turbo) be useful there? Both as more standard and perhaps more optimized (maybe).

    No, most of those are not particularly related to the scope of libjpeg. b2d_convexdecomp is used to decompose complex convex polygons in optimized triangles for the physics engine. pvrtc and etc1 are GPU texture compression formats. As for jpeg-compressor (actually it's only the decompressor class), it could maybe be replaced, but since we vendor libraries anyway for all !Linux, I prefer to keep the lightweight variant unless there's a strong reason to swap libraries.

    • certs is a certificate bundle, presumably taken from Ubuntu's rootcerts a while ago... It can and should be unbundled (as well as updated in git, with its provenance better documented), but I haven't had time to work on it.

    Having worked on Pidgin, I know you don't really want to be in the business of maintaining certificate authority lists...

    Well we're definitely not maintaining it, but we still need a bundle to be able to resolve certificates from Godot (both for Godot's own needs with the Asset Library, and for games which need to communicate with websites). So the vendored bundle will stay, but it's indeed not meant to be modified, just updated every now and then.

    It seems to be a PITA to generate a proper cert bundle from the Mozilla rootcerts.txt, so I think I'll just grab the latest version from Fedora's rootcerts to update the current vendored one with undocumented provenance. (Mozilla's rootcerts.txt is MPL, which could be problematic for Godot, but Fedora deems its compiled version "public domain", so I'll just happily trust fedora-legal and go with that).

    $ ls thirdparty/misc/
aes256.cpp  base64.c  curl_hostcheck.c  fastlz.c  hq2x.cpp  md5.cpp  mikktspace.c  sha256.c  smaz.c  stb_truetype.h    triangulator.h
aes256.h    base64.h  curl_hostcheck.h  fastlz.h  hq2x.h    md5.h    mikktspace.h  sha256.h  smaz.h  triangulator.cpp  yuv2rgb.h

    Some of these are provided by OpenSSL, I think? But I see you've worked out the licensing stuff already.

    Yeah aes256, md5 and sha256 could likely be replaced by OpenSSL - though tbh the less we depend on OpenSSL, the better. We only need it for TLS support and I'd gladly get rid of it to replace it by a more lightweight LTS library.

  • Rémi Verschelde @akien ·

    FYI, I've sent an update to Debian's WNPP for Godot with some general advice for packaging Godot, and the results of the discussion we had here so far: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=793057#45

0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Please register or to comment