GNU bug report logs - #69918
[PATCH] gnu: smalltalk: add pharo-vm 10.1.1.

Previous Next

Full log

Message #8 received at 69918 <at> debbugs.gnu.org (full text, mbox):

From: Nicolas Graves <ngraves <at> ngraves.fr>
To: 69918 <at> debbugs.gnu.org
Cc: dziltener <at> lyrion.ch
Subject: Re: [bug#69918] [PATCH] gnu: smalltalk: add pharo-vm 10.1.1.
Date: Mon, 20 May 2024 14:36:13 +0200

Hi, thanks for this patch!

Some comments below.

On 2024-03-20 16:36, Daniel Ziltener via Guix-patches via wrote:

> ---
>  gnu/packages/smalltalk.scm | 66 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 65 insertions(+), 1 deletion(-)
>
> diff --git a/gnu/packages/smalltalk.scm b/gnu/packages/smalltalk.scm
> index 64146813d1..60e0cc5778 100644
> --- a/gnu/packages/smalltalk.scm
> +++ b/gnu/packages/smalltalk.scm
> @@ -5,6 +5,7 @@
>  ;;; Copyright © 2016 Ludovic Courtès <ludo <at> gnu.org>
>  ;;; Copyright © 2018 Tobias Geerinckx-Rice <me <at> tobias.gr>
>  ;;; Copyright © 2021 Maxime Devos <maximedevos <at> telenet.be>
> +;;; Copyright © 2024 Daniel Ziltener <dziltener <at> lyrion.ch>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -36,13 +37,16 @@ (define-module (gnu packages smalltalk)
>    #:use-module (gnu packages fontutils)
>    #:use-module (gnu packages gl)
>    #:use-module (gnu packages glib)
> +  #:use-module (gnu packages image)
>    #:use-module (gnu packages libffi)
>    #:use-module (gnu packages libsigsegv)
>    #:use-module (gnu packages linux)
>    #:use-module (gnu packages multiprecision)
>    #:use-module (gnu packages pkg-config)
>    #:use-module (gnu packages pulseaudio)
> -  #:use-module (gnu packages xorg))
> +  #:use-module (gnu packages version-control)
> +  #:use-module (gnu packages xorg)
> +  #:use-module (gnu packages xdisorg))

You're missing a few imports : (guix gexp) (gnu packages gtk).

> 
>  (define-public smalltalk
>    (package
> @@ -184,3 +188,63 @@ (define-public squeak-vm
>  interactively extensible Web sites.")
>      (home-page "http://squeakvm.org/")
>      (license license:x11)))
> +
> +(define-public pharo-vm
> +  (package
> +   (name "pharo-vm")
> +   (version "10.1.1")
> +   (source
> +    (origin
> +     (method url-fetch)
> +     (uri (string-append "http://files.pharo.org/vm/pharo-spur64-headless/Linux-x86_64/source/PharoVM-" version "-32b2be5-Linux-x86_64-c-src.tar.gz"))

You should keep the lines under 79 characters. If you need, you can
split a line with a \ character before the end of the line. I'm also not
sure we wouldn't want the github code directly, since we want to
cross-compile when possible, and this seems specific to x86_64
architectures.

> +     (sha256
> +      (base32
> +       "1hbkvfrw57sz5nw48z64789yjcry9l1am4hmkndy9dd6i06n2c2n"))))
> +   (build-system cmake-build-system)
> +   (arguments
> +    (list
> +     #:configure-flags
> +      #~(list
> +         (string-append "-DPHARO_LIBRARY_PATH="
> +                        (assoc-ref %outputs "out") "/lib")
> +         "-DGENERATED_SOURCE_DIR=."
> +         "-DALWAYS_INTERACTIVE=on"
> +         "-DBUILD_IS_RELEASE=on"
> +         "-DGENERATE_SOURCES=off"

We should try when possible to generate sources and vm instances. Here
it seems to be a classic bootstrap chain: each major version of pharo is
used to build the next. Let's hope there's not a lot more dependencies
along the chain. Do you feel confident enough with guix and is your
computer beefy enough to tackle this?

> +         "-DBUILD_BUNDLE=off")
> +      #:phases
> +      #~(modify-phases %standard-phases
> +          (delete 'check)

You should rather use #:tests? #false and explain in a comment why tests
are not present.

> +          (delete 'validate-runpath)
> +          (add-after 'install 'really-install
> +            (lambda _
> +              (let ((bin (string-append #$output "/bin"))
> +                    (lib (string-append #$output "/lib")))
> +                (mkdir-p bin)
> +                (mkdir-p lib)
> +                (copy-recursively "./build/vm/pharo"
> +                                  (string-append bin "/pharo"))
> +                (for-each (lambda (file)
> +                            (let ((inode (string-append "./build/vm/" file)))
> +                              (copy-recursively
> +                               inode
> +                               (string-append lib "/" file))))
> +                          (with-directory-excursion
> +                              "./build/vm"
> +                            (find-files "."
> +                                        (lambda (file stat)
> +                                          (string-contains file ".so")))))
> +                (wrap-program (string-append bin "/pharo")
> +                  `("LD_LIBRARY_PATH" prefix (,lib)))))))))

This phase can be reduced to:

(add-after 'install 'really-install
  (lambda _
    (install-file "./build/vm/pharo"
                  (string-append #$output "/bin"))
    (for-each
      (lambda (file)
        (install-file file (string-append #$output "/lib")))
      (find-files "./build/vm" "\\.so"))))

Isn't there a way to replace this last phase by carefully chosen Cmake
flags? I couldn't, but would be great if possible.

I've fixed the part about rpath, will be adding a patch over that,
you may use it as a basis for a v3.

> +   (inputs
> +    (list libffi
> +          libgit2
> +          cairo
> +          freetype
> +          pixman
> +          libpng
> +          util-linux))
> +   (synopsis "This is the VM used by Pharo")

I like to write my synopses as action statements, answering the question 
What can you do with this package?

> +   (home-page "https://www.pharo.org")
> +   (description "This is the VM used by Pharo.")

Be more specific about description : what is Pharo?  What can you do
with this package? You may also add the PharoVM features from Github
repo wrapped in a @itemize / @end itemize texinfo snippet.

> +   (license license:expat)))

-- 
Best regards,
Nicolas Graves

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.