On Sun, Oct 29, 2017 at 09:54:38PM +0100, Julien Lepiller wrote:
> On Sun, 29 Oct 2017 21:47:09 +0100
> Julien Lepiller <julien@lepiller.eu> wrote:
> 
> > Hi, here are 22 new java patches that get us a bit closer to maven :)
> > 
> > 
> > 
> 
> Somehow I can't use git send-email, so here are the patches.

Thanks for all this!

This is a cursory review. I trust that, in general, these packages are
the latest upstream versions (or there is a code comment explaining why
not), the licenses are free and correctly listed, and that everything
seems to work.

Beyond that, I think your work on Java packaging falls under the commit
policy mentioned in HACKING, about "allowing individuals to commit
directly on non-controversial changes on parts they’re familiar with":

https://git.savannah.gnu.org/cgit/guix.git/tree/HACKING?id=faffd821f35dc34944226a992033df5a4be60cb1#n63

> From 10c1801922abbc82e48360b22d5d8681449715b9 Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <julien@lepiller.eu>
> Date: Tue, 24 Oct 2017 22:05:04 +0200
> Subject: [PATCH 01/22] gnu: Add java-bsh.
> 
> * gnu/packages/java.scm (java-bsh): New variable.

> From f02f8c081e1627180e9f78393c3ab1fd8ad8b60b Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <julien@lepiller.eu>
> Date: Tue, 24 Oct 2017 22:05:04 +0200
> Subject: [PATCH 02/22] gnu: Add java-bsh.
> 
> * gnu/packages/java.scm (java-bsh): New variable.

These two patches can be squashed into one, right?

> From 016af8aad804f488e530ae680855f175e8d8cfd1 Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <julien@lepiller.eu>
> Date: Tue, 24 Oct 2017 22:08:38 +0200
> Subject: [PATCH 03/22] gnu: Add java-jmock.
> 
> * gnu/packages/java.scm (java-jmock): New variable.

This patch looks a bit weird, as if there are two copies of the package
or something.

> From ff6b73b44424949796b18689e4c5aba156573b41 Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <julien@lepiller.eu>
> Date: Tue, 24 Oct 2017 22:30:48 +0200
> Subject: [PATCH 10/22] gnu: Add java-mvel2.
> 
> * gnu/packages/java.scm (java-mvel2): New variable.

This patch also adds java-jnacl. It should be added in its own patch,
right?

> From 3ea3acbd6cc141687107b1ad2a14bad7cefb9ebe Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <julien@lepiller.eu>
> Date: Tue, 24 Oct 2017 22:36:11 +0200
> Subject: [PATCH 12/22] gnu: Add java-bouncycastle-bcprov.
> 
> * gnu/packages/java.scm (java-bouncycastle-bcprov): New variable.

[...]

> +       #:phases
> +       (modify-phases %standard-phases
> +         (add-before 'configure 'unzip-src
> +           (lambda _
> +             (mkdir-p "src")
> +             (with-directory-excursion "src"
> +               (zero? (system* "unzip" "../src.zip"))))))))

Not a blocker for this patch series — I notice this custom unzip
phase in a few packages. Should it be added to ant-build-system?

> From d300f5ec70007f7554f14031b2af2def5ad3f439 Mon Sep 17 00:00:00 2001
> From: Julien Lepiller <julien@lepiller.eu>
> Date: Wed, 25 Oct 2017 23:05:10 +0200
> Subject: [PATCH 16/22] gnu: Add java-powermock-reflect.
> 
> * gnu/packages/java.scm (java-powermock-reflect): New variable.
> ---
>  gnu/local.mk                                       |   1 +
>  gnu/packages/java.scm                              |  38 +++++
>  .../patches/java-powermock-fix-java-files.patch    | 178 +++++++++++++++++++++

Please mention the new patch file and its addition to gnu/local.mk in
the commit message.

Also, do you think the patch should be submitted upstream?