Subject: [bug#40035] Add widelands game

From: Nicolas Goaziou <[email-masked]>
Date: Thu, 12 Mar 2020 15:31:20 +0100

Hello,

Alberto EFG <[email-masked]> writes:

> This is my first patch.

Thank you, and congratulations!

> +(define-public widelands
> +  (let ((commit "d9513d413f2558f9ef6f033a7685bf9881fbdbb3")
> +	(revision "1"))

I suggest to add a comment explaining why we rely on a commit, and, if
that makes sense, why this particular one, e.g., "No official release."

> +    (package
> +     (name "widelands")
> +     (version (git-version "20" revision commit))

Where is this "20" coming from?

> +     (source (origin

Nitpick: I suggest to move `origin' to a line on its own.

> +     (arguments
> +      `(#:tests? #f

Why are the tests disabled? We usually provide a comment when disabling
tests.

> +	#:configure-flags
> +	(let* ((out (assoc-ref %outputs "out"))
> +	       (share (string-append out "/share")))
> +	  (list    "-DCMAKE_BUILD_TYPE=Release"
> +		   (string-append "-DCMAKE_INSTALL_PREFIX=" out "/bin")
> +		   (string-append "-DWL_INSTALL_BASEDIR=" share "/widelands")
> +		   (string-append "-DWL_INSTALL_DATADIR=" share "/widelands")
> +		   "-DOPTION_BUILD_WEBSITE_TOOLS=OFF"))))
> +     (inputs
> +      `(("sdl" ,(sdl-union (list sdl2
> +				 sdl2-image
> +				 sdl2-mixer
> +				 sdl2-ttf)))

Nitpick: all can go into a single line.

> +	("gettext" ,gettext-minimal)

This probably belongs to `native-inputs' not `inputs'.

> +        ("icu4c" ,icu4c)

Indentation is off here.

> +	("libpng" ,libpng)
> +	("zlib" ,zlib)
> +	("boost" ,boost)
> +	("python" ,python)

This one may also be a native input. Could you check?

> +	("glew" ,glew)))

Could you re-order inputs alphabetically?

> +     (synopsis "Real-time strategy game")

It is a bit terse. Debian uses the slightly more accurate:

  "Fantasy real-time strategy game"

Maybe it is worth mentioning. Or better, something like:

   "Fantasy real-time strategy game with singleplayer campaigns and multiplayer mode"

It could make sense since in the description I suggest below, there is
no reference to campaigns nor multiplayer.

> +     (description
> +      "Widelands is a free, open source real-time strategy game with

You can remove "free" and "open source". All is free in Guix!

> + singleplayer campaigns and a multiplayer mode.  The game was inspired
> + by Settlers II but has significantly more variety and depth to it.  ")

Mind the spurious spaces at the end.

Again, Debian uses:

    Widelands is a strategy game aiming for gameplay similar to Settlers II by
    BlueByte.

    In this game, you start out on a small piece of land with nothing more than
    a few of useful resources.  Using those, you can build yourself an empire
    with many thousands of inhabitants.  On your way towards this goal, you will
    have to build up an economic infrastructure, explore the lands around you
    and face enemies who are trying to rule the world just like you do.

Would it be better to use it?

> +     (home-page "https://www.widelands.org/")

Nitpick: home-page is usually located above synopsis. I don't know if
there's a strong rule about it, tho.

> +     (license license:gpl2+))))

You are missing out some licenses (CC-based) from the assets in the
game. Could you add them too?

Could you send an updated patch?

Regards,

-- 
Nicolas Goaziou