diff mbox

gnu: Add git-review.

Message ID 20160909150021.GD5507@macbook42.flashner.co.il
State New
Headers show

Commit Message

Efraim Flashner Sept. 9, 2016, 3 p.m. UTC
On Fri, Sep 09, 2016 at 10:23:20AM +0200, Clément Lassieur wrote:
> Efraim Flashner <efraim@flashner.co.il> writes:
> 
> > On Thu, Sep 08, 2016 at 10:28:37PM +1000, Ben Woodcroft wrote:
> 
> >> I think including git simply as an input is problematic because git-review
> >> calls git via 'subprocess', as evidenced by
> >> 
> >> $ ./pre-inst-env guix environment -C --ad-hoc git-review
> >> $ git-review
> >> [..]
> >>   File "/gnu/store/m4gc2wx4q9if1vrhgclpspdil7rqsn21-python-3.4.3/lib/python3.4/subprocess.py",
> >> line 1457, in _execute_child
> >>     raise child_exception_type(errno_num, err_msg)
> >> FileNotFoundError: [Errno 2] No such file or directory: 'git'
> >> 
> >> So, I think we need to patch the source code to call the full path to git,
> >> or otherwise wrap the 'git-review' executable.
> >> 
> >> > +    (home-page "http://docs.openstack.org/infra/git-review/")
> >> > +    (synopsis "Command-line tool for Gerrit")
> >> > +    (description
> >> > +     "Git-review is a command-line tool that helps submitting Git branches to
> >> > +Gerrit for review, or fetching existing ones.")
> >> > +    (license asl2.0)))
> >> Otherwise seems OK to me. Can you test with environment -C -N?
> >> ben
> >> 
> >
> > Try it with the attached patch
> 
> Hi, sorry for the mess and thanks for the reviews.
> 
> I did not know about the "guix environment -C -N" way of testing
> packages, and I'm very happy to learn this.
> 
> The attached patch seems to work for git, but the same error now happens
> with ssh.
> 
> -- 
> Clément

I found ssh and I found an instance of scp, so this should be better.

Comments

Clément Lassieur Sept. 9, 2016, 4:09 p.m. UTC | #1
Efraim Flashner <efraim@flashner.co.il> writes:

> I found ssh and I found an instance of scp, so this should be better.

--8<---------------cut here---------------start------------->8---
clement@lev ~/foo [env]# git-review -d 36373
Downloading refs/changes/73/36373/1 from gerrit
Cannot fetch patchset contents

Does specified change number belong to this project?

The following command failed with exit code 128
    "/gnu/store/...-git-2.10.0/bin/git fetch gerrit refs/changes/73/36373/1"
-----------------------
error: cannot run ssh: No such file or directory
fatal: unable to fork
-----------------------
clement@lev ~/foo [env]# /gnu/store/...-git-2.10.0/bin/git remote -v                           
gerrit  ssh://bar (fetch)
gerrit  ssh://bar (push)
--8<---------------cut here---------------end--------------->8---

It looks like ssh isn't hardcoded within git either. Is it worth doing
this for git-review then?
Alex Kost Sept. 9, 2016, 6:49 p.m. UTC | #2
Efraim Flashner (2016-09-09 18:00 +0300) wrote:

[...]
> +    (arguments
> +     `(#:tests? #f ; tests require a running Gerrit server
> +       #:phases
> +       (modify-phases %standard-phases
> +         (add-after 'unpack 'hardcode-outside-programs
> +           (lambda _

A side note: I think in phases it is better to use 'inputs' argument
instead of %build-inputs:

              (lambda* (#:key inputs #:allow-other-keys)

> +             (let* ((git  (string-append
> +                            (assoc-ref %build-inputs "git") "/bin/git"))
> +                    (base (assoc-ref %build-inputs "openssh"))

(assoc-ref inputs "git") and (assoc-ref inputs "openssh") here

> +                    (ssh  (string-append base "/bin/ssh"))
> +                    (scp  (string-append base "/bin/scp")))
> +               (substitute* '("git_review/cmd.py" "git_review/tests/test_git_review.py"
> +                              "git_review/tests/test_unit.py" "git_review/tests/utils.py")
> +                            (("\"git ") (string-append "\"" git " "))
> +                            (("\"git\"") (string-append "\"" git "\""))
> +                            (("'git'") (string-append "'" git "'")))
> +               (substitute* "git_review/cmd.py"
> +                            (("\"ssh\"") (string-append "\"" ssh "\""))
> +                            (("'ssh'") (string-append "'" ssh "'"))
> +                            (("\"scp\"") (string-append "\"" scp "\""))
> +                            (("'scp") (string-append "'" scp)))
> +               #t))))))
>      (native-inputs
>       `(("python-pbr" ,python-pbr)))
>      (inputs
>       `(("python-requests" ,python-requests)
> +       ("openssh" ,openssh)
>         ("git" ,git)))
>      (home-page "http://docs.openstack.org/infra/git-review/")
>      (synopsis "Command-line tool for Gerrit")
Efraim Flashner Sept. 10, 2016, 7:05 p.m. UTC | #3
On Fri, Sep 09, 2016 at 06:09:40PM +0200, Clément Lassieur wrote:
> Efraim Flashner <efraim@flashner.co.il> writes:
> 
> > I found ssh and I found an instance of scp, so this should be better.
> 
> --8<---------------cut here---------------start------------->8---
> clement@lev ~/foo [env]# git-review -d 36373
> Downloading refs/changes/73/36373/1 from gerrit
> Cannot fetch patchset contents
> 
> Does specified change number belong to this project?
> 
> The following command failed with exit code 128
>     "/gnu/store/...-git-2.10.0/bin/git fetch gerrit refs/changes/73/36373/1"
> -----------------------
> error: cannot run ssh: No such file or directory
> fatal: unable to fork
> -----------------------
> clement@lev ~/foo [env]# /gnu/store/...-git-2.10.0/bin/git remote -v                           
> gerrit  ssh://bar (fetch)
> gerrit  ssh://bar (push)
> --8<---------------cut here---------------end--------------->8---
> 
> It looks like ssh isn't hardcoded within git either. Is it worth doing
> this for git-review then?

This looks like you are using ssh for talking to the remote server, in
which case it would exist outside of git-review. Or did I misunderstand
it?

It does seem that wrapping the program would be easier than trying to
fing regexes that work to patching only certain instances of other
programs.
Clément Lassieur Sept. 10, 2016, 9:25 p.m. UTC | #4
Efraim Flashner <efraim@flashner.co.il> writes:

>> It looks like ssh isn't hardcoded within git either. Is it worth doing
>> this for git-review then?
>
> This looks like you are using ssh for talking to the remote server, in
> which case it would exist outside of git-review. Or did I misunderstand
> it?

This error was a git error, when it tries to fetch a remote through ssh
in a container. It is kind of the same thing that happens with
git-review. Except that git-review is unusable without ssh. That's why
ssh has to be an input of git-review.

> It does seem that wrapping the program would be easier than trying to
> fing regexes that work to patching only certain instances of other
> programs.

I sent a patch that does the wrapping.
diff mbox

Patch

diff --git a/gnu/packages/openstack.scm b/gnu/packages/openstack.scm
index 4cb38a9..65bc296 100644
--- a/gnu/packages/openstack.scm
+++ b/gnu/packages/openstack.scm
@@ -20,6 +20,7 @@ 
 
 (define-module (gnu packages openstack)
   #:use-module (gnu packages python)
+  #:use-module (gnu packages ssh)
   #:use-module (gnu packages tls)
   #:use-module (gnu packages version-control)
   #:use-module (guix build-system python)
@@ -796,11 +797,33 @@  permanence.")
         (base32
          "07d1jn9ryff5j5ic6qj5pbk10m1ccmpllj0wyalrcms1q9yhlzh8"))))
     (build-system python-build-system)
-    (arguments `(#:tests? #f)) ; tests require a running Gerrit server
+    (arguments
+     `(#:tests? #f ; tests require a running Gerrit server
+       #:phases
+       (modify-phases %standard-phases
+         (add-after 'unpack 'hardcode-outside-programs
+           (lambda _
+             (let* ((git  (string-append
+                            (assoc-ref %build-inputs "git") "/bin/git"))
+                    (base (assoc-ref %build-inputs "openssh"))
+                    (ssh  (string-append base "/bin/ssh"))
+                    (scp  (string-append base "/bin/scp")))
+               (substitute* '("git_review/cmd.py" "git_review/tests/test_git_review.py"
+                              "git_review/tests/test_unit.py" "git_review/tests/utils.py")
+                            (("\"git ") (string-append "\"" git " "))
+                            (("\"git\"") (string-append "\"" git "\""))
+                            (("'git'") (string-append "'" git "'")))
+               (substitute* "git_review/cmd.py"
+                            (("\"ssh\"") (string-append "\"" ssh "\""))
+                            (("'ssh'") (string-append "'" ssh "'"))
+                            (("\"scp\"") (string-append "\"" scp "\""))
+                            (("'scp") (string-append "'" scp)))
+               #t))))))
     (native-inputs
      `(("python-pbr" ,python-pbr)))
     (inputs
      `(("python-requests" ,python-requests)
+       ("openssh" ,openssh)
        ("git" ,git)))
     (home-page "http://docs.openstack.org/infra/git-review/")
     (synopsis "Command-line tool for Gerrit")