Created attachment 210282 [details] Removed ugly hack and added icon installation I had an ugly hack in the CMakeLists.txt patch for a problem I've fixed after being pointed this out by a very helpful DragonflyBSD committer. On Linux DevilutionX also requires use of a certain font. This has been added as well. On top of this I've added installation of the desktop file as it's used in Linux and the necessary icons.
Created attachment 210283 [details] Removed ugly hack and added icon installation Correct patch, the previous one contained a maintainer script.
Created attachment 210375 [details] Patch to upgrade devilutionX to version 1.0.0 and fix issues Upstream has just released 1.0.0, so the patch has been updated to take this into account just as well.
Thanks, overall this looks good. A few minor comments on the patch: LOCALBASE is where other ports/packages installed things, PREFIX is where *this* port/package is going to install things. I think CMAKE_INSTALL_SHAREDIR should reference PREFIX. CMAKE_BUILD_TYPE is already set based on WITH_DEBUG, perhaps you just need to duplicate the logic from here: https://svnweb.freebsd.org/ports/head/Mk/Uses/cmake.mk?revision=488341&view=markup#l65 since I guess they don't use the standard build type names. Setting __BSD_VISIBLE and such is usually not advisable, instead it may be that you need to specify the compiler standard. Also, hopefully you would be able to use standard cmake flags instead of patching the CMakeLists.txt for that. (Checking CMAKE_SYSTEM_NAME does make sense tho, please keep that and send it upstream.) Probably better to just remove the OS name from devilutionx.desktop entirely, it's not really required, people know what OS they're on. (And Dragonfly wouldn't have to re-patch it.)
*** Bug 243013 has been marked as a duplicate of this bug. ***
Created attachment 210391 [details] Patch to upgrade devilutionX to version 1.0.0 and fix issues post review Hi Steve, thanks for the review, I really appreciate it. I've uploaded a modified patch and here are my 2 cents on your review points. > LOCALBASE (...) Replaced with PREFIX in the CMAKE_INSTALL_SHAREDIR variable > CMAKE_BUILD_TYPE is already set based on WITH_DEBUG (...) I'm not sure what you mean with this one. Do you mean the NIGHTLY/RELEASE option? These are an upstream thing. What I could do is set the NIGHTLY=ON option when WITH_DEBUG is on. Any ideas if this would be a good idea? > (...) __BSD_VISIBLE (...) Yes, that was the hack I had before. I've had some comments on this before. The patch removes these hacks from the port's patches. > (...) remove the OS name from devilutionx.desktop (...) Done. It is indeed better like this.
(In reply to malavon from comment #5) Hi, > I'm not sure what you mean with this one. Do you mean the NIGHTLY/RELEASE option? These are an upstream thing. What I could do is set the NIGHTLY=ON option when WITH_DEBUG is on. Any ideas if this would be a good idea? I mean that you should set the build type based *only* on WITH_DEBUG and not have an OPTION at all. > Yes, that was the hack I had before. I've had some comments on this before. The patch removes these hacks from the port's patches. Ah, sorry, misread the patch, good job then. The patch is much better, just fix the one issue then it should be ready. Steve
Created attachment 210393 [details] Patch to upgrade devilutionX to version 1.0.0 and fix issues post review 2
And there's that last point tackled as well now. Thanks for the improvements, Steve.
A commit references this bug: Author: swills Date: Thu Jan 2 16:53:36 UTC 2020 New revision: 521859 URL: https://svnweb.freebsd.org/changeset/ports/521859 Log: games/devilutionX 1.0.0: version update and fixed some issues PR: 242940 Submitted by: malavon <benny.goemans@gmail.com> (maintainer, with slight changes) Changes: head/games/devilutionX/Makefile head/games/devilutionX/distinfo head/games/devilutionX/files/patch-CMakeLists.txt head/games/devilutionX/files/patch-Packaging_fedora_devilutionx.desktop
Made some more changes I noticed on final review and after testing. (In particular, the CXXFLAGS looks ugly, but it's the only way I found to make it work for me.) Anyway, committed, thanks!