Subject: Re: [bug#33876] [PATCH]:gnu: Add flat assembler

From: Kei Kebreau <[email-masked]>
Date: Fri, 04 Jan 2019 19:10:21 -0500

Hi Guy,

Guy fleury <[email-masked]> writes:

> From 061c6b7fd399a304d0a0fc089f0020b4f9fa368c Mon Sep 17 00:00:00 2001
> From: guy fleury iteriteka <[email-masked]>
> Date: Wed, 26 Dec 2018 09:39:23 +0100
> Subject: [PATCH] gnu:Add flat assembler
>
> ---
>  gnu/packages/assembly.scm | 56 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/gnu/packages/assembly.scm b/gnu/packages/assembly.scm
> index 763d183cf..34832bad2 100644
> --- a/gnu/packages/assembly.scm
> +++ b/gnu/packages/assembly.scm
> @@ -30,7 +30,9 @@
>    #:use-module (gnu packages perl)
>    #:use-module (gnu packages texinfo)
>    #:use-module (gnu packages python)
> -  #:use-module (gnu packages xml))
> +  #:use-module (gnu packages xml)
> +  #:use-module ((guix utils)
> +                #:select (%current-system)))
>  
>  (define-public nasm
>    (package
> @@ -122,3 +124,55 @@ abstracts over the target CPU by exposing a standardized RISC instruction set
>  to the clients.")
>      (home-page "https://www.gnu.org/software/lightning/")
>      (license license:gpl3+)))
> +
> +(define-public fasm
> +  (package
> +    (name "fasm")
> +    (version "1.73.04")

Since you've submitted this patch, the newest version has become
1.73.06. This is not a difficult change at all.

> +    (source
> +     (origin
> +       (method url-fetch)
> +       (uri (string-append "https://flatassembler.net/fasm-"
> +                           version ".tgz"))
> +       (sha256
> +        (base32
> +         "0y0xkf9fzcm5gklhdi61wjpd1p8islpbcnkv5k16aqci3qsd0ia1"))))
> +    (build-system gnu-build-system)
> +    (arguments
> +     `(#:phases
> +       (modify-phases %standard-phases
> +         (delete 'configure) ;;no configure script used
> +         (replace 'build
> +           (lambda _
> +             ;;source code are in this directory
> +             (chdir "source/Linux/")
> +             (if (string=? ,(%current-system) "x86_64-linux") ;;if x86_86

I believe you meant "if x86_64" in this comment? Anyway, it is not
necessary, as it is easy to see what is going on here.

> +                 ;;use pre-binarie in /root directory to build itself

Instead of "/root directory", I'd say "top-level directory", and
instead of "pre-binarie" we typically refer to these as "pre-compiled
binaries".

Also, we try to avoid relying on pre-compiled binaries to build
software, but it seems that fasm is strictly self-building.

> +                 (invoke "../../fasm.x64" "fasm.asm")
> +
> +                 (invoke "../../fasm" "fasm.asm")))) ;;the same with i686
> +
> +         (replace 'install
> +           (lambda _
> +             (let ((out (assoc-ref %outputs "out")))
> +               (begin
> +                 ;;delete pre-binaries
> +                 ;;this make sure not copy them
> +                 ;;instead of the one recentently build
> +                 (for-each delete-file '("../../fasm" "../../fasm.x64"))

You shouldn't need to remove these because the recently built fasm is
clearly in the current directory by itself.

> +                 (mkdir-p (string-append out "/bin"))
> +                 ;;copy the excutable recentently build in /source/Linux/fasm
> +                 ;;the excutable name is fasm
> +                 (copy-file "fasm" (string-append out "/bin/fasm"))))))

You can use the 'install-file' procedure here instead of
'copy-file'. This way you don't have to worry about naming the
file, the directory you install to is automatically created and your
intentions are clearer. See the difference:

(mkdir-p (string-append out "/bin"))
(copy-file "fasm" (string-append out "/bin/fasm"))

vs.

(install-file "fasm" (string-append out "/bin"))

Also please make sure that the install phase returns #t so Guix doesn't
show its "phase returned `#<unspecified>'" warning.

> +
> +         (delete 'check)))) ;;no test

Instead of deleting the check phase manually, you can use #f as an
argument to the keyword #:tests?. While you do that, also use #f as an
argument to the keyword #:strip-binaries? because strip reports that
fasm's binary doesn't have sections in it, so stripping the debug
section from the binary would have no effect.

(arguments
 `(#:tests? #f ; no tests
   #:strip-binaries? #f ; fasm has no sections.
   #:phases ...

> +    ;;support only intel x86 family processors
> +    (supported-systems '("x86_64-linux" "i686-linux"))
> +    (synopsis "Assembler for x86 processors")
> +    (description
> +     "FASM is a assembler that supports x86, and IA-64 Intel architectures.It
> +does multiple passes to optimize machine code.It have macro abilities and focus on

We like to keep our line lengths below 80 characters, and it is easily
fixed in this case by moving "on" to the next line.

> +operating system portability.")
> +    (home-page "https://flatassembler.net/")
> +    (license license:bsd-2)))
> +

Please reply with a patch with the above corrections. Thanks for
packaging this!