[bug#34729,v2,core-updates] gnu: libgit2: Avoid Python.

Message ID 20190304114120.6987-1-dannym@scratchpost.org
State Accepted
Headers show
Series [bug#34729,v2,core-updates] gnu: libgit2: Avoid Python. | expand

Checks

Context Check Description
cbaines/comparison success View comparison
cbaines/applying patch fail Apply failed

Commit Message

Danny Milosavljevic March 4, 2019, 11:41 a.m. UTC
* gnu/packages/patches/libgit2-avoid-python.patch: New file.
* gnu/local.mk (dist_patch_DATA): Add it.
* gnu/packages/version-control.scm (libgit2)[source]: Use it.
[inputs]: Remove python.
[native-inputs]: Add guile-2.2.
---
 gnu/local.mk                                  |   1 +
 .../patches/libgit2-avoid-python.patch        | 304 ++++++++++++++++++
 gnu/packages/version-control.scm              |  10 +-
 3 files changed, 311 insertions(+), 4 deletions(-)
 create mode 100644 gnu/packages/patches/libgit2-avoid-python.patch

Comments

Ludovic Courtès March 11, 2019, 9:38 p.m. UTC | #1
Hi Danny,

AFAICS changes to libgit2 can go to master.

Danny Milosavljevic <dannym@scratchpost.org> skribis:

> * gnu/packages/patches/libgit2-avoid-python.patch: New file.
> * gnu/local.mk (dist_patch_DATA): Add it.
> * gnu/packages/version-control.scm (libgit2)[source]: Use it.
> [inputs]: Remove python.
> [native-inputs]: Add guile-2.2.

So the goal is to remove Python from libgit2’s *build-time*
dependencies, right?  (Python is not a run-time dependency for libgit2
according to ‘guix size libgit2’.)  Can you explain a bit more?  :-)

> +--- orig/libgit2-0.27.7/tests/generate.scm	1970-01-01 01:00:00.000000000 +0100
> ++++ libgit2-0.27.7/tests/generate.scm	2019-03-04 12:18:00.688040975 +0100
> +@@ -0,0 +1,277 @@
> ++;; -*- geiser-scheme-implementation: guile -*-
> ++
> ++;;; Implementation: Danny Milosavljevic <dannym@scratchpost.org>
> ++;;; Based on: Implementation in Python by Vicent Marti.
> ++;;; License: ISC, like the original generate.py in clar.

This is really cool, but at the same time I feel like we’d rather have a
compelling reason to start maintaining a parallel implementation of
their test suite machinery.

WDYT?

Thanks,
Ludo’.
Danny Milosavljevic March 11, 2019, 10:17 p.m. UTC | #2
Hi Ludo,

On Mon, 11 Mar 2019 22:38:07 +0100
Ludovic Courtès <ludo@gnu.org> wrote:

> Danny Milosavljevic <dannym@scratchpost.org> skribis:
> 
> > * gnu/packages/patches/libgit2-avoid-python.patch: New file.
> > * gnu/local.mk (dist_patch_DATA): Add it.
> > * gnu/packages/version-control.scm (libgit2)[source]: Use it.
> > [inputs]: Remove python.
> > [native-inputs]: Add guile-2.2.  
> 
> So the goal is to remove Python from libgit2’s *build-time*
> dependencies, right?  (Python is not a run-time dependency for libgit2
> according to ‘guix size libgit2’.)  Can you explain a bit more?  :-)

It's listed as regular input, though (for no reason).

We use git for quite basic things in Guix (especially since we usually
use git to fetch packages from github) and we had some problems
with Python 2 (since fixed).  So Andreas, Ricardo and me looked at what
on Earth libgit2 needs Python for and found that it's just one smallish
file used at build time.

Hence the replacement :)

> This is really cool, but at the same time I feel like we’d rather have a
> compelling reason to start maintaining a parallel implementation of
> their test suite machinery.

Sure, it makes sense to think about who's going to maintain it.

In this case it's just a test case collector which basically does

$ grep -r "void.*test_" .

so I think the risk of it diverging is low.  If it after all does diverge
(and we notice it), we can always replace it by the Python version again.

It's pretty much a 1:1 port of the original (it's so long because
I made both the structure and the output the same), so it should be
easy to track future changes.

libgit2 bundled it from an upstream project, "clar".  That also usually
means that the bundled copy gets updated less often.
 
We could also try upstreaming it.

btw: I also saw that Andy once had a Python frontend for Guile, but it
appears unfinished.  Obviously that would be even nicer.
Ricardo Wurmus March 11, 2019, 11:04 p.m. UTC | #3
Danny Milosavljevic <dannym@scratchpost.org> writes:

> btw: I also saw that Andy once had a Python frontend for Guile, but it
> appears unfinished.  Obviously that would be even nicer.

What about this?

https://gitlab.com/python-on-guile/python-on-guile
Ludovic Courtès March 18, 2019, 9:40 a.m. UTC | #4
Hello Danny,

Danny Milosavljevic <dannym@scratchpost.org> skribis:

> On Mon, 11 Mar 2019 22:38:07 +0100
> Ludovic Courtès <ludo@gnu.org> wrote:
>
>> Danny Milosavljevic <dannym@scratchpost.org> skribis:
>> 
>> > * gnu/packages/patches/libgit2-avoid-python.patch: New file.
>> > * gnu/local.mk (dist_patch_DATA): Add it.
>> > * gnu/packages/version-control.scm (libgit2)[source]: Use it.
>> > [inputs]: Remove python.
>> > [native-inputs]: Add guile-2.2.  
>> 
>> So the goal is to remove Python from libgit2’s *build-time*
>> dependencies, right?  (Python is not a run-time dependency for libgit2
>> according to ‘guix size libgit2’.)  Can you explain a bit more?  :-)
>
> It's listed as regular input, though (for no reason).
>
> We use git for quite basic things in Guix (especially since we usually
> use git to fetch packages from github) and we had some problems
> with Python 2 (since fixed).  So Andreas, Ricardo and me looked at what
> on Earth libgit2 needs Python for and found that it's just one smallish
> file used at build time.
>
> Hence the replacement :)

I see, that makes sense!

>> This is really cool, but at the same time I feel like we’d rather have a
>> compelling reason to start maintaining a parallel implementation of
>> their test suite machinery.
>
> Sure, it makes sense to think about who's going to maintain it.
>
> In this case it's just a test case collector which basically does
>
> $ grep -r "void.*test_" .
>
> so I think the risk of it diverging is low.  If it after all does diverge
> (and we notice it), we can always replace it by the Python version again.
>
> It's pretty much a 1:1 port of the original (it's so long because
> I made both the structure and the output the same), so it should be
> easy to track future changes.
>
> libgit2 bundled it from an upstream project, "clar".  That also usually
> means that the bundled copy gets updated less often.

OK, well let’s do that, then.  Please make sure to add comments in the
patch that explain the rationale, things we must pay attention to when
updating (if any), etc.

> We could also try upstreaming it.

Well, you could try, but I have a feeling that this wouldn’t be
fruitful.  :-)

Thank you!

Ludo’.
Danny Milosavljevic March 18, 2019, 9:31 p.m. UTC | #5
Hi Ludo,

which branch should it go to?
Marius Bakke March 21, 2019, 7:07 p.m. UTC | #6
Danny Milosavljevic <dannym@scratchpost.org> writes:

> which branch should it go to?

`guix refresh -l libgit2` suggests 'master' should be fine, no?
Ludovic Courtès March 22, 2019, 8:39 a.m. UTC | #7
Hello,

Thanks for pushing the patch, and apologies for the delay answering your
messages!

Ludovic Courtès <ludo@gnu.org> skribis:

> OK, well let’s do that, then.  Please make sure to add comments in the
> patch that explain the rationale, things we must pay attention to when
> updating (if any), etc.

I think you forgot this bit.  :-)

TIA,
Ludo’.

Patch

diff --git a/gnu/local.mk b/gnu/local.mk
index 52df157a9..6f7b207d7 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -969,6 +969,7 @@  dist_patch_DATA =						\
   %D%/packages/patches/libexif-CVE-2017-7544.patch		\
   %D%/packages/patches/libextractor-CVE-2018-20430.patch	\
   %D%/packages/patches/libextractor-CVE-2018-20431.patch	\
+  %D%/packages/patches/libgit2-avoid-python.patch		\
   %D%/packages/patches/libgit2-mtime-0.patch			\
   %D%/packages/patches/libgit2-oom-test.patch			\
   %D%/packages/patches/libgdata-fix-tests.patch			\
diff --git a/gnu/packages/patches/libgit2-avoid-python.patch b/gnu/packages/patches/libgit2-avoid-python.patch
new file mode 100644
index 000000000..c85097440
--- /dev/null
+++ b/gnu/packages/patches/libgit2-avoid-python.patch
@@ -0,0 +1,304 @@ 
+diff -ruN orig/libgit2-0.27.7/tests/CMakeLists.txt libgit2-0.27.7/tests/CMakeLists.txt
+--- orig/libgit2-0.27.7/tests/CMakeLists.txt	1970-01-01 01:00:00.000000000 +0100
++++ libgit2-0.27.7/tests/CMakeLists.txt	2019-03-04 11:13:06.640118979 +0100
+@@ -1,10 +1,3 @@
+-FIND_PACKAGE(PythonInterp)
+-
+-IF(NOT PYTHONINTERP_FOUND)
+-  MESSAGE(FATAL_ERROR "Could not find a python interpeter, which is needed to build the tests. "
+-    "Make sure python is available, or pass -DBUILD_CLAR=OFF to skip building the tests")
+-ENDIF()
+-
+ SET(CLAR_FIXTURES "${CMAKE_CURRENT_SOURCE_DIR}/resources/")
+ SET(CLAR_PATH "${CMAKE_CURRENT_SOURCE_DIR}")
+ ADD_DEFINITIONS(-DCLAR_FIXTURE_PATH=\"${CLAR_FIXTURES}\")
+@@ -21,7 +14,7 @@
+ 
+ ADD_CUSTOM_COMMAND(
+ 	OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/clar.suite
+-	COMMAND ${PYTHON_EXECUTABLE} generate.py -o "${CMAKE_CURRENT_BINARY_DIR}" -f -xonline -xstress -xperf .
++	COMMAND guile generate.scm -o "${CMAKE_CURRENT_BINARY_DIR}" -f -x online -x stress -x perf .
+ 	DEPENDS ${SRC_TEST}
+ 	WORKING_DIRECTORY ${CLAR_PATH}
+ )
+diff -ruN orig/libgit2-0.27.7/tests/generate.scm libgit2-0.27.7/tests/generate.scm
+--- orig/libgit2-0.27.7/tests/generate.scm	1970-01-01 01:00:00.000000000 +0100
++++ libgit2-0.27.7/tests/generate.scm	2019-03-04 12:18:00.688040975 +0100
+@@ -0,0 +1,277 @@
++;; -*- geiser-scheme-implementation: guile -*-
++
++;;; Implementation: Danny Milosavljevic <dannym@scratchpost.org>
++;;; Based on: Implementation in Python by Vicent Marti.
++;;; License: ISC, like the original generate.py in clar.
++
++(use-modules (ice-9 ftw))
++(use-modules (ice-9 regex))
++(use-modules (ice-9 getopt-long))
++(use-modules (ice-9 rdelim))
++(use-modules (ice-9 match))
++(use-modules (ice-9 textual-ports))
++(use-modules (srfi srfi-1))
++
++(define (render-callback cb)
++  (if cb
++      (string-append "    { \"" (assoc-ref cb "short-name") "\", &"
++                     (assoc-ref cb "symbol") " }")
++      "    { NULL, NULL }"))
++
++(define (replace needle replacement haystack)
++  "Replace all occurences of NEEDLE in HAYSTACK by REPLACEMENT.
++NEEDLE is a regular expression."
++  (regexp-substitute/global #f needle haystack 'pre replacement 'post))
++
++(define (skip-comments* text)
++  (call-with-input-string
++   text
++   (lambda (port)
++     (let loop ((result '())
++                (section #f))
++       (define (consume-char)
++         (cons (read-char port) result))
++       (define (skip-char)
++         (read-char port)
++         result)
++       (match section
++        (#f
++         (match (peek-char port)
++          (#\/ (loop (consume-char) 'almost-in-block-comment))
++          (#\" (loop (consume-char) 'in-string-literal))
++          (#\' (loop (consume-char) 'in-character-literal))
++          ((? eof-object?) result)
++          (_ (loop (consume-char) section))))
++        ('almost-in-block-comment
++         (match (peek-char port)
++          (#\* (loop (consume-char) 'in-block-comment))
++          (#\/ (loop (consume-char) 'in-line-comment))
++          ((? eof-object?) result)
++          (_ (loop (consume-char) #f))))
++        ('in-line-comment
++         (match (peek-char port)
++          (#\newline (loop (consume-char) #f))
++          ((? eof-object?) result)
++          (_ (loop (skip-char) section))))
++        ('in-block-comment
++         (match (peek-char port)
++           (#\* (loop (skip-char) 'almost-out-of-block-comment))
++           ((? eof-object?) result)
++           (_ (loop (skip-char) section))))
++        ('almost-out-of-block-comment
++         (match (peek-char port)
++           (#\/ (loop (cons (read-char port) (cons #\* result)) #f))
++           (#\* (loop (skip-char) 'almost-out-of-block-comment))
++           ((? eof-object?) result)
++           (_ (loop (skip-char) 'in-block-comment))))
++        ('in-string-literal
++         (match (peek-char port)
++           (#\\ (loop (consume-char) 'in-string-literal-escape))
++           (#\" (loop (consume-char) #f))
++           ((? eof-object?) result)
++           (_ (loop (consume-char) section))))
++        ('in-string-literal-escape
++         (match (peek-char port)
++           ((? eof-object?) result)
++           (_ (loop (consume-char) 'in-string-literal))))
++        ('in-character-literal
++         (match (peek-char port)
++           (#\\ (loop (consume-char) 'in-character-literal-escape))
++           (#\' (loop (consume-char) #f))
++           ((? eof-object?) result)
++           (_ (loop (consume-char) section))))
++        ('in-character-literal-escape
++         (match (peek-char port)
++           ((? eof-object?) result)
++           (_ (loop (consume-char) 'in-character-literal)))))))))
++
++(define (skip-comments text)
++  (list->string (reverse (skip-comments* text))))
++
++(define (maybe-only items)
++  (match items
++   ((a) a)
++   (_ #f)))
++
++(define (Module name path excludes)
++  (let* ((clean-name (replace "_" "::" name))
++         (enabled (not (any (lambda (exclude)
++                              (string-prefix? exclude clean-name))
++                            excludes))))
++    (define (parse contents)
++      (define (cons-match match prev)
++        (cons
++         `(("declaration" . ,(match:substring match 1))
++           ("symbol" . ,(match:substring match 2))
++           ("short-name" . ,(match:substring match 3)))
++         prev))
++      (let* ((contents (skip-comments contents))
++             (entries (fold-matches (make-regexp
++                                     (string-append "^(void\\s+(test_"
++                                                    name
++                                                    "__(\\w+))\\s*\\(\\s*void\\s*\\))\\s*\\{")
++                                     regexp/newline)
++                                    contents
++                                    '()
++                                    cons-match))
++             (entries (reverse entries))
++             (callbacks (filter (lambda (entry)
++                                   (match (assoc-ref entry "short-name")
++                                    ("initialize" #f)
++                                    ("cleanup" #f)
++                                    (_ #t)))
++                                entries)))
++        (if (> (length callbacks) 0)
++            `(("name" . ,name)
++              ("enabled" . ,(if enabled "1" "0"))
++              ("clean-name" . ,clean-name)
++              ("initialize" . ,(maybe-only (filter-map (lambda (entry)
++                                                      (match (assoc-ref entry "short-name")
++                                                       ("initialize" entry)
++                                                       (_ #f)))
++                                                     entries)))
++              ("cleanup" . ,(maybe-only (filter-map (lambda (entry)
++                                                   (match (assoc-ref entry "short-name")
++                                                    ("cleanup" entry)
++                                                    (_ #f)))
++                                                  entries)))
++              ("callbacks" . ,callbacks))
++            #f)))
++
++    (define (refresh path)
++      (and (file-exists? path)
++           (parse (call-with-input-file path get-string-all))))
++    (refresh path)))
++
++(define (generate-TestSuite path output excludes)
++    (define (load)
++        (define enter? (const #t))
++        (define (leaf file stat result)
++          (let* ((module-root (string-drop (dirname file)
++                                           (string-length path)))
++                 (module-root (filter-map (match-lambda
++                                           ("" #f)
++                                           (a a))
++                                          (string-split module-root #\/))))
++            (define (make-module path)
++              (let* ((name (string-join (append module-root (list (string-drop-right (basename path) (string-length ".c")))) "_"))
++                     (name (replace "-" "_" name)))
++                (Module name path excludes)))
++            (if (string-suffix? ".c" file)
++                (let ((module (make-module file)))
++                  (if module
++                      (cons module result)
++                      result))
++                result)))
++        (define (down dir stat result)
++          result)
++        (define (up file state result)
++          result)
++        (define skip (const #f))
++        (file-system-fold enter? leaf down up skip error '() path))
++
++    (define (CallbacksTemplate module)
++      (string-append "static const struct clar_func _clar_cb_"
++                     (assoc-ref module "name") "[] = {\n"
++                     (string-join (map render-callback
++                                       (assoc-ref module "callbacks"))
++                                  ",\n")
++                     "\n};\n"))
++
++    (define (DeclarationTemplate module)
++      (string-append (string-join (map (lambda (cb)
++                                         (string-append "extern "
++                                                        (assoc-ref cb "declaration")
++                                                        ";"))
++                                       (assoc-ref module "callbacks"))
++                                  "\n")
++                     "\n"
++                     (if (assoc-ref module "initialize")
++                         (string-append "extern " (assoc-ref (assoc-ref module "initialize") "declaration") ";\n")
++                         "")
++                     (if (assoc-ref module "cleanup")
++                         (string-append "extern " (assoc-ref (assoc-ref module "cleanup") "declaration") ";\n")
++                         "")))
++
++    (define (InfoTemplate module)
++      (string-append "
++    {
++        \"" (assoc-ref module "clean-name") "\",
++    " (render-callback (assoc-ref module "initialize")) ",
++    " (render-callback (assoc-ref module "cleanup")) ",
++        _clar_cb_" (assoc-ref module "name") ", "
++        (number->string (length (assoc-ref module "callbacks")))
++        ", " (assoc-ref module "enabled") "
++    }"))
++
++    (define (Write data)
++      (define (name< module-a module-b)
++        (string<? (assoc-ref module-a "name")
++                  (assoc-ref module-b "name")))
++      (define modules (sort (load) name<))
++
++      (define (suite-count)
++        (length modules))
++
++      (define (callback-count)
++        (fold + 0 (map (lambda (entry)
++                         (length (assoc-ref entry "callbacks")))
++                         modules)))
++
++      (define (display-x value)
++        (display value data))
++
++      (for-each (compose display-x DeclarationTemplate) modules)
++      (for-each (compose display-x CallbacksTemplate) modules)
++
++      (display-x "static struct clar_suite _clar_suites[] = {")
++      (display-x (string-join (map InfoTemplate modules) ","))
++      (display-x "\n};\n")
++
++      (let ((suite-count-str (number->string (suite-count)))
++            (callback-count-str (number->string (callback-count))))
++        (display-x "static const size_t _clar_suite_count = ")
++        (display-x suite-count-str)
++        (display-x ";\n")
++
++        (display-x "static const size_t _clar_callback_count = ")
++        (display-x callback-count-str)
++        (display-x ";\n")
++
++        (display (string-append "Written `clar.suite` ("
++                                callback-count-str
++                                " tests in "
++                                suite-count-str
++                                " suites)"))
++        (newline))
++      #t)
++
++    (call-with-output-file (string-append output "/clar.suite") Write))
++
++;;; main
++
++(define (main)
++  (define option-spec
++    '((force (single-char #\f) (value #f))
++      (exclude (single-char #\x) (value #t))
++      (output (single-char #\o) (value #t))
++      (help  (single-char #\h) (value #f))))
++
++  (define options (getopt-long (command-line) option-spec #:stop-at-first-non-option #t))
++  (define args (reverse (option-ref options '() '())))
++  (when (> (length args) 1)
++    (display "More than one path given\n")
++    (exit 1))
++
++  (if (< (length args) 1)
++      (set! args '(".")))
++
++  (let* ((path (car args))
++         (output (option-ref options 'output path))
++         (excluded (filter-map (match-lambda
++                                (('exclude . value) value)
++                                (_ #f))
++                               options)))
++    (generate-TestSuite path output excluded)))
++
++(main)
diff --git a/gnu/packages/version-control.scm b/gnu/packages/version-control.scm
index baf49e64f..4278f9311 100644
--- a/gnu/packages/version-control.scm
+++ b/gnu/packages/version-control.scm
@@ -68,6 +68,7 @@ 
   #:use-module (gnu packages gettext)
   #:use-module (gnu packages gl)
   #:use-module (gnu packages groff)
+  #:use-module (gnu packages guile)
   #:use-module (gnu packages haskell)
   #:use-module (gnu packages haskell-check)
   #:use-module (gnu packages haskell-crypto)
@@ -536,7 +537,8 @@  everything from small to very large projects with speed and efficiency.")
                (base32
                 "0c95pbv7hwclwmn51nqnh1lb0cajpcdb24pbdzcir6vmhfj3am0s"))
               (patches (search-patches "libgit2-mtime-0.patch"
-                                       "libgit2-oom-test.patch"))
+                                       "libgit2-oom-test.patch"
+                                       "libgit2-avoid-python.patch"))
 
               ;; Remove bundled software.
               (snippet '(begin
@@ -562,10 +564,10 @@  everything from small to very large projects with speed and efficiency.")
            (lambda _ (invoke "./libgit2_clar" "-v" "-Q"))))))
     (inputs
      `(("libssh2" ,libssh2)
-       ("http-parser" ,http-parser)
-       ("python" ,python-wrapper)))
+       ("http-parser" ,http-parser)))
     (native-inputs
-     `(("pkg-config" ,pkg-config)))
+     `(("guile" ,guile-2.2)
+       ("pkg-config" ,pkg-config)))
     (propagated-inputs
      ;; These two libraries are in 'Requires.private' in libgit2.pc.
      `(("openssl" ,openssl)