Subject: Re: [bug#34325] A better patch

From: Ricardo Wurmus <[email-masked]>
Date: Tue, 05 Feb 2019 10:15:05 +0100

Hi Andrew,

> OK. Managed to build `pre-inst-env` and run `guix lint`, fixed couple
> minor issues.
>
> From 2267b26f8d38fb40e58d62e3e6fcdaff01001384 Mon Sep 17 00:00:00 2001
> From: Andrew Miloradovsky <[email-masked]>
> Date: Mon, 4 Feb 2019 23:28:23 +0000
> Subject: [PATCH] gnu: Add xmagnify
>
> Tiny screen magnifier for X11
>
> * gnu/packages/accessibility.scm (xmagnify): New variable.

Thank you very much for your patch.  For a first patch this is near
perfect, congratulations!

> +(define-public xmagnify
> +  (let ((commit "56da280173e9d0bd7b3769e07ba485cb4db35869")
> +        (revision "0.1.0"))

The “revision” variable is used internally to ensure that the version
string increases monotonically.  Git commit hashes are random, so a
future commit could end up lower in a sorted list that an older commit.
That’s why we put a “revision” string before the commit hash.  The
revision usually starts at 1 and is incremented each time the commit
hash is updated.

In this case we can do without this because the last commit is also the
tag “0.1.0”, which we can use directly.

> +    (package
> +     (name "xmagnify")
> +     (version "0.1.0")
> +     (source (origin
> +              (method git-fetch)
> +              (uri (git-reference
> +                    (url "https://gitlab.com/amiloradovsky/magnify.git")
> +                    (commit commit)))
> +              (file-name (git-file-name name version))
> +              (sha256
> +               (base32
> +                "1ngnp5f5zl3v35vhbdyjpymy6mwrs0476fm5nd7dzkba7n841jdh"))))
> +     (inputs `(("libX11" ,libx11)
> +               ("xproto" ,xproto)))

xproto does not seem to be needed.

> +     (build-system gnu-build-system)
> +     (arguments
> +      `(#:tests? #f
> +        #:make-flags '("CC=gcc")
> +        #:phases
> +        (modify-phases %standard-phases
> +                       (replace 'configure
> +                                (lambda _
> +                                  (substitute*
> +                                   '("Makefile") (("/usr")
> +                                                  (assoc-ref %outputs "out")))
> +                                  #t)))))

You can specify the “prefix” variable instead of patching the Makefile.

> +     (home-page "https://gitlab.com/amiloradovsky/magnify")
> +     (synopsis "Tiny screen magnifier for X11")
> +     (description
> +      "Magnifies a screen region by an integer positive factor and
> +draws the result on a window.  Useful as an accessibility tool, which
> +works with every X Window System based GUI (depends only on libX11);
> +or as an assistant for graphic designers, who need to select
> +individual pixels.")

Here we should use complete sentences.

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

This is a dual license, which should be noted in a comment.

I pushed the patch to the master branch with commit 16bb27f28 after
performing these changes and adding a copyright line for you.

Thanks again!

--
Ricardo