diff mbox series

[bug#40267,1/2] gnu: Add unicorn.

Message ID 20200328005052.22846-1-kuba@kadziolka.net
State Under Review
Headers show
Series [bug#40267,1/2] gnu: Add unicorn. | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job

Commit Message

Maja Kądziołka March 28, 2020, 12:50 a.m. UTC
* gnu/packages/emulators.scm (unicorn-next): New variable.
---

If I package a -rc version, should it have a -next suffix in its name
even though the "stable" version isn't packaged?

Maybe I should also package the non-rc unicorn? The test suite for that
version fails to compile, so it's not entirely trivial.

I'd also like to bring these build phases to your attention - I feel
like they need a review the most.

 gnu/packages/emulators.scm | 110 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 109 insertions(+), 1 deletion(-)

Comments

Leo Famulari March 29, 2020, 3:48 a.m. UTC | #1
On Sat, Mar 28, 2020 at 01:50:52AM +0100, Jakub Kądziołka wrote:
> * gnu/packages/emulators.scm (unicorn-next): New variable.

Sounds like a cool package!

> If I package a -rc version, should it have a -next suffix in its name
> even though the "stable" version isn't packaged?

I think it's best to just call it unicorn. The version says -rc and we
mention it in the synopsis and description. And it's useful under the
hood for guix lint to match the upstream name.

> Maybe I should also package the non-rc unicorn? The test suite for that
> version fails to compile, so it's not entirely trivial.

Is the previous release useful? We normally don't package betas or
release candidates... it depends. Do you have an idea of the release
timeline? Do you think upstream would mind if we packaged the RC?

> +    ;; NOTE: unicorn ships a bundled QEMU, but with custom modifications.

Can you add more detail to this comment? Is it just a patch on a QEMU
tarball or is this not really QEMU anymore?

> +    ;; The main library is not written in Python, but the build process has
> +    ;; little in common with any defined build system, so we might as well
> +    ;; build on top of python-build-system and make use of all
> +    ;; the Python-specific phases that can be reused.

Okay

> +         (add-after 'unpack 'install-bindings-to-python-output
> +           (lambda* (#:key outputs #:allow-other-keys)
> +             ;; python-build-system will build the bindings and install them to
> +             ;; the "out" output, so change the build-internal names of the
> +             ;; outputs.
> +             (set-car! (assoc "out" outputs) "lib")
> +             (set-car! (assoc "python" outputs) "out")
> +             #t))

I would wait for advice here. The manual requests we write everything in
a functional style. But I don't know of another way to make
python-build-system install things to alternate outputs without changing
the build system or replacing the install phase. It would be nice to
have a parameter for this somewhere...

> +         (add-before 'check 'check-library
> +           (lambda* (#:key outputs #:allow-other-keys)
> +             ;; TODO: running the tests on non-x86 requires a cross-binutils
> +             ;; with x86 as target.
> +             ,@(if (member (%current-system) '("x86_64-linux" "i686-linux"))

I think the 'when' procedure is more clear than 'if' in cases where the
else branch is empty.
Maja Kądziołka March 29, 2020, 12:43 p.m. UTC | #2
On Sat, Mar 28, 2020 at 11:48:11PM -0400, Leo Famulari wrote:
> On Sat, Mar 28, 2020 at 01:50:52AM +0100, Jakub Kądziołka wrote:
> > If I package a -rc version, should it have a -next suffix in its name
> > even though the "stable" version isn't packaged?
> 
> I think it's best to just call it unicorn. The version says -rc and we
> mention it in the synopsis and description. And it's useful under the
> hood for guix lint to match the upstream name.

Fair enough.

> > Maybe I should also package the non-rc unicorn? The test suite for that
> > version fails to compile, so it's not entirely trivial.
> 
> Is the previous release useful?

As far as I am aware, the non-rc release is not useful beyond avoiding
any potential uneasyness about running -rc releases ;)

> We normally don't package betas or
> release candidates... it depends. Do you have an idea of the release
> timeline?

Sadly, I have no idea.

> Do you think upstream would mind if we packaged the RC?

I don't think so? As a datapoint, FreeBSD packages the -rc.

> > +    ;; NOTE: unicorn ships a bundled QEMU, but with custom modifications.
> 
> Can you add more detail to this comment? Is it just a patch on a QEMU
> tarball or is this not really QEMU anymore?

The documentation suggests the changes go quite deep:

| Internally, Unicorn reuses the CPU emulation component of QEMU as its
| core (with quite a lot of changes to adapt to our design).

What do you think about a comment like this?

;; NOTE: unicorn ships a bundled QEMU, but heavily modified.

> > +         (add-after 'unpack 'install-bindings-to-python-output
> > +           (lambda* (#:key outputs #:allow-other-keys)
> > +             ;; python-build-system will build the bindings and install them to
> > +             ;; the "out" output, so change the build-internal names of the
> > +             ;; outputs.
> > +             (set-car! (assoc "out" outputs) "lib")
> > +             (set-car! (assoc "python" outputs) "out")
> > +             #t))
> 
> I would wait for advice here. The manual requests we write everything in
> a functional style. But I don't know of another way to make
> python-build-system install things to alternate outputs without changing
> the build system or replacing the install phase. It would be nice to
> have a parameter for this somewhere...

Yeah, it's not the nicest thing. I think I'll submit a patch to c-u that
would add such a parameter (does #:python-output sound good?), and then come
back here when it lands. Would this imperative hack be ok as a temporary solution?

> > +         (add-before 'check 'check-library
> > +           (lambda* (#:key outputs #:allow-other-keys)
> > +             ;; TODO: running the tests on non-x86 requires a cross-binutils
> > +             ;; with x86 as target.
> > +             ,@(if (member (%current-system) '("x86_64-linux" "i686-linux"))
> 
> I think the 'when' procedure is more clear than 'if' in cases where the
> else branch is empty.

The issue is that the else branch contains '(), and is not itself empty.
Do you happen to know how to get the value of (%current-system)
build-side?

Thanks for your review!
diff mbox series

Patch

diff --git a/gnu/packages/emulators.scm b/gnu/packages/emulators.scm
index 3591b1740c..5b5e1359e3 100644
--- a/gnu/packages/emulators.scm
+++ b/gnu/packages/emulators.scm
@@ -44,6 +44,7 @@ 
   #:use-module (gnu packages boost)
   #:use-module (gnu packages backup)
   #:use-module (gnu packages cdrom)
+  #:use-module (gnu packages check)
   #:use-module (gnu packages compression)
   #:use-module (gnu packages curl)
   #:use-module (gnu packages elf)
@@ -88,7 +89,8 @@ 
   #:use-module (gnu packages web)
   #:use-module (guix build-system cmake)
   #:use-module (guix build-system glib-or-gtk)
-  #:use-module (guix build-system gnu))
+  #:use-module (guix build-system gnu)
+  #:use-module (guix build-system python))
 
 (define-public desmume
   (package
@@ -1628,3 +1630,109 @@  derived from Gens.  Project goals include clean source code, combined features
 from various forks of Gens, and improved platform portability.")
     (supported-systems '("i686-linux" "x86_64-linux"))
     (license license:gpl2+)))
+
+;; python-pwntools requires a -rc release of unicorn
+(define-public unicorn-next
+  (package
+    (name "unicorn-next")
+    (version "1.0.2-rc2")
+    ;; NOTE: unicorn ships a bundled QEMU, but with custom modifications.
+    (source
+     (origin
+       (method git-fetch)
+       (uri (git-reference
+             (url "https://github.com/unicorn-engine/unicorn")
+             (commit version)))
+       (file-name (git-file-name name version))
+       (sha256
+        (base32
+         "0v5sc8sniv2w0bki1f7n3pgsk17y7hggw55fvkjzk2sv8z8w4bsj"))))
+    (outputs '("out" "python"))
+    ;; The main library is not written in Python, but the build process has
+    ;; little in common with any defined build system, so we might as well
+    ;; build on top of python-build-system and make use of all
+    ;; the Python-specific phases that can be reused.
+    (build-system python-build-system)
+    (arguments
+     `(#:modules ((srfi srfi-26)
+                  (guix build python-build-system)
+                  (guix build utils))
+       #:phases
+       (modify-phases %standard-phases
+         (add-after 'unpack 'install-bindings-to-python-output
+           (lambda* (#:key outputs #:allow-other-keys)
+             ;; python-build-system will build the bindings and install them to
+             ;; the "out" output, so change the build-internal names of the
+             ;; outputs.
+             (set-car! (assoc "out" outputs) "lib")
+             (set-car! (assoc "python" outputs) "out")
+             #t))
+         (add-before 'build 'build-library
+           (lambda* (#:key inputs #:allow-other-keys)
+             (invoke "make"
+                     "-j" (number->string (parallel-job-count))
+                     (string-append
+                      "UNICORN_QEMU_FLAGS=--python="
+                      (assoc-ref inputs "python-for-qemu")
+                      "/bin/python2")
+                     "UNICORN_STATIC=no"
+                     "CC=gcc")))
+         (add-after 'build-library 'install-library
+           (lambda* (#:key outputs #:allow-other-keys)
+             (invoke "make" "install"
+                     "UNICORN_STATIC=no"
+                     (string-append
+                      "PREFIX="
+                      (assoc-ref outputs "lib")))))
+         (add-before 'build 'prepare-bindings
+           (lambda* (#:key outputs #:allow-other-keys)
+             (chdir "bindings/python")
+             ;; Set this environment variable so that the Python bindings
+             ;; don't build their own copy of the shared object, but use
+             ;; a dummy value such that the bindings test suite uses the
+             ;; same mechanism for loading the library as any other user.
+             (setenv "LIBUNICORN_PATH" "1")
+             (substitute* "unicorn/unicorn.py"
+               (("_path_list = \\[.*")
+                (string-append
+                 "_path_list = [\""
+                 (assoc-ref outputs "lib")
+                 ;; eat the rest of the list
+                 "/lib\"] + 0*[")))
+             #t))
+         (add-before 'check 'check-library
+           (lambda* (#:key outputs #:allow-other-keys)
+             ;; TODO: running the tests on non-x86 requires a cross-binutils
+             ;; with x86 as target.
+             ,@(if (member (%current-system) '("x86_64-linux" "i686-linux"))
+                   '((for-each
+                      (lambda (suite)
+                        (with-directory-excursion
+                            (string-append "../../tests/" suite)
+                          (invoke "make" "test" "CC=gcc")))
+                      '("unit" "regress")))
+                   '())
+             #t))
+         (add-after 'install 'install-samples
+           (lambda* (#:key outputs #:allow-other-keys)
+             (let* ((python-samples (find-files "." "sample_.*"))
+                    (c-samples (find-files "../../samples" ".*\\.c"))
+                    (python-docdir
+                      (string-append (assoc-ref outputs "out")
+                                     "/share/doc/unicorn/samples"))
+                    (c-docdir
+                      (string-append (assoc-ref outputs "lib")
+                                     "/share/doc/unicorn/samples")))
+               (for-each (cut install-file <> c-docdir) c-samples)
+               (for-each (cut install-file <> python-docdir) python-samples)
+               #t))))))
+    (native-inputs
+     `(("cmocka" ,cmocka)
+       ("python-for-qemu" ,python-2)
+       ("hexdump-for-tests" ,util-linux)))
+    (home-page "http://www.unicorn-engine.org")
+    (synopsis "Unicorn CPU emulator framework")
+    (description
+     "Unicorn is a lightweight, multi-platform, multi-architecture CPU emulator
+framework based on QEMU.")
+    (license license:gpl2+)))