config/debug.log
1 ↗	(On Diff #33865)	I assume you included this whole file on accident
package.yml
8	Seems like there is also - GPL-2.0-or-later WITH Font-exception-2.0 #GNU Unifont - MIT #getpost/gulrak - OFL-1.1-RFN #Terminus Font - Zlib #PLF List/Colony From: https://github.com/CleverRaven/Cataclysm-DDA/blob/master/LICENSE.txt (leaving out the debug and test stuff that we don't use)
12	I think `games.rpg` would be more fitting for the `-common` package as well
21	Note that A-Z before a-z, so the `SDL2-` stuff comes first.
43	We can clean this up by putting those patches in a `series` file and using `%apply_patches` (see https://getsol.us/articles/packaging/packaging-practices/en/#handling-multiple-patches)
46	You can use our own variable `$workdir` for this (so you don't have to export anything, just use `cp -R $workdir` and so on. [Note: only if we can't find a way to do this whole thing without having two copies of the source, see below]
52	It'd be cleaner to instead move this to a `environment : \|` key before `setup` export COMMON_OPTIONS="CLANG=1 CCACHE=1 PREFIX=/usr RELEASE=1 USE_HOME_DIR=1 BACKTRACE=0 RUNTESTS=0 ZLEVELS=1 LOCALIZE=1 LANGUAGES=all" and then simply use `%make $COMMON_OPTIONS` plus any additional options for the step Can't we just skip the copying of the sources in `setup`, and the folder switching, and instead simply run the following?: %make $COMMON_OPTIONS %make $COMMON_OPTIONS TILES=1 SOUND=1 (same for the install step)

This revision now requires changes to proceed.Dec 20 2022, 1:26 AM

In D13877#238087, @Staudey wrote:

Started reviewing this but then got a little sidetracked, so I will just post my preliminary observations before I go to bed

Thanks for all the feedback! Not sure when I'll get to this with Christmas coming up but I'll work on it when I get a chance.

Properly alphabetized deps, cleaned up patches, build, and install

Harbormaster completed remote builds in B4777: Diff 33915.Dec 28 2022, 8:30 PM

Icosahunter marked 7 inline comments as done.Dec 28 2022, 8:32 PM

Nice work! Two more little questions:
(otherwise LGTM)

files/0002-Update-catch.hpp-to-2.13.7.patch
1 ↗	(On Diff #33915)	Do we actually need this patch when we're not running the tests (yet)? Compilation seems to run fine without it.
package.yml
48	From looking at their repo it seems like at least `USE_HOME_DIR` and `LOCALIZE` are set to "ON" by default. Can we remove these options, or does something during the setup/build override those defaults?

Icosahunter added inline comments.Dec 29 2022, 1:32 AM

files/0002-Update-catch.hpp-to-2.13.7.patch
1 ↗	(On Diff #33915)	I think I had tests on in earlier testing and then didn't check if some of the patches were still needed when I changed the build options, I'll try removing it.
package.yml
48	I copied the build options from the Cataclysm build on AUR, very possible they just opted to specify everything, I'll try without and check if everything is good.

Made requested changes (except removing HOME_DIR=1 as that does not appear to be set by default)

Harbormaster completed remote builds in B4778: Diff 33916.Dec 29 2022, 4:11 AM

Icosahunter marked 2 inline comments as done.Dec 29 2022, 4:15 AM

Icosahunter added inline comments.

files/0002-Update-catch.hpp-to-2.13.7.patch
1 ↗	(On Diff #33915)	Indeed that patch was not required to build anymore.
package.yml
48	I tried without USE_HOME_DIR and it didn't find my world so looks like it is needed, and I didn't see it set anywhere in the MAKEFILE so makes sense. LOCALIZE does appear to be set to 1 by default and after building with it removed and switching languages it did change the language in game, so we should be good to go here.