Subject: Re: bug#26454: [PATCH 2/2] gnu: add emacs-exwm

From: Arun Isaac <[email-masked]>
Date: Sun, 16 Apr 2017 02:43:48 +0530

Thanks for the updated patches! Could you please add a copyright header
for you at the top of the emacs.scm source file?

> +    ;; The following functions or variables is need by emacs-xelb,
> +    ;; which is not included in 'emacs-minimal'.

Make this comment as follows.

The following functions and variables needed by emacs-xelb are not
included in emacs-minimal:

> +    ;; x-display-screens x-keysym-table x-alt-keysym x-meta-keysym
> +    ;; x-hyper-keysym x-super-keysym libxml-parse-xml-region
> +    ;; x-display-pixel-width x-display-pixel-height

> +    (arguments
> +     `(#:emacs ,emacs
> +       #:phases
> +       ;; Generate .el files from the XML description of the XCB bindings.
> +       ;; This will be adjusted when xelb developer comment in issue page:
> +       ;; https://github.com/ch11ng/xelb/issues/16
> +       (modify-phases %standard-phases
> +         (add-after 'unpack 'regenerate-el-files
> +           (lambda _
> +             (let ((xcb-proto-xml-path (string-append (assoc-ref %build-inputs
> +                                                                 "xcb-proto")
> +                                                      "/share/xcb"))
> +                   (emacs-path         (string-append (assoc-ref %build-inputs
> +                                                                 "emacs")
> +                                                      "/bin/emacs")))
> +               (substitute* "Makefile"
> +                 (("PROTO_PATH := ../xcb-proto/src")
> +                  (string-append "PROTO_PATH := " xcb-proto-xml-path))
> +                 (("EMACS_BIN := emacs -Q")
> +                  (string-append "EMACS_BIN := " emacs-path "  -Q")))

You should be able to do this just using #:make-flags. See
https://www.gnu.org/software/make/manual/html_node/Overriding.html

> +    (inputs `(("xcb-proto" ,xcb-proto)))

xcb-proto is only required during build time, right? Therefore, it is a
native input.

> +    (propagated-inputs
> +     `(("emacs-xelb" ,emacs-xelb)
> +       ("xhost" ,xhost)
> +       ("dbus" ,dbus)))

Instead of making xhost and dbus propagated inputs, we should just
reference the required executables with their absolute paths. That way,
xhost and dbus would become normal inputs.

> +    ;; The following functions or variables is need by emacs-exwm,
> +    ;; which is not included in 'emacs-minimal'.

Same as mentioned earlier.

> +    ;; scroll-bar-mode, fringe-mode
> +    ;; x-display-pixel-width x-display-pixel-height

> +       (modify-phases %standard-phases
> +         (add-after 'build 'install-xsession
> +           (lambda* (#:key outputs #:allow-other-keys)
> +             (let* ((emacs-path (string-append (assoc-ref %build-inputs
> +                                                          "emacs")
> +                                               "/bin/emacs"))
> +                    (sh-path    (string-append (assoc-ref %build-inputs
> +                                                          "bash")
> +                                               "/bin/sh"))
> +                    (xhost-path (string-append (assoc-ref %build-inputs
> +                                                          "xhost")
> +                                               "/bin/xhost"))
> +                    (dbus-launch-path (string-append (assoc-ref %build-inputs
> +                                                                "dbus")
> +                                                     "/bin/dbus-launch"))

Use the keyword argument `inputs' here, instead of directly using `%build-inputs'.

> +                    (output (assoc-ref outputs "out"))
> +                    (xsessions-dir (string-append output "/share/xsessions"))
> +                    (desktop-file (string-append xsessions-dir "/exwm-demo.desktop"))
> +                    (bin-dir (string-append output "/bin"))
> +                    (exwm-demo-file (string-append bin-dir "/exwm-demo-session")))
> +               ;; Add a .desktop file to xsessions.
> +               (mkdir-p xsessions-dir)
> +               (mkdir-p bin-dir)
> +               (with-output-to-file desktop-file
> +                 (lambda _
> +                   (format #t
> +                    "[Desktop Entry]~@
> +                     Name=Exwm-Demo~@
> +                     Comment=Demo of Emacs X Window Manager ~@
> +                     Exec=~a/bin/exwm-demo-session~@
> +                     TryExec=~@*~a/bin/exwm-demo-session~@
> +                     Icon=~@
> +                     Type=Application~%"
> +                    output)))

Let's not call this a "Demo". Better to just use name, synopsis, etc. as
done for the xmonad package.

> +               ;; Add a shell wrap file to bin directory.
> +               ;; Fixup: Set DISPLAY variable for the reason:
> +               ;; https://github.com/ch11ng/exwm/issues/213
> +               (with-output-to-file exwm-demo-file
> +                 (lambda _
> +                   (format #t
> +                    "#!~a ~@
> +                     export DISPLAY=:0 ~@
> +                     ~a +SI:localuser:$USER ~@
> +                     exec ~a --exit-with-session ~a -q --eval \"(progn (require 'exwm)(require 'exwm-config)(exwm-config-default))\" ~%"
> +                    sh-path xhost-path dbus-launch-path emacs-path)))
> +               (chmod exwm-demo-file #o555)
> +               #t))))))

Similarly, remove any mention of the word "demo".