Re: [PATCH] Add junit.

[Ricardo Wurmus]

Eric Bavier <address@hidden> writes:

> On 2016-04-22 09:16, Ricardo Wurmus wrote:
>> Hi Guix,
>> 
>> here’s a first batch of patches for Java libraries.  Many Java packages
>> depend on JUnit, so that’s where I started.  “hamcrest-core” is just a
>> small part of the whole hamcrest library, but it’s enough to build
>> JUnit.
> [...]
>> +         (add-before 'configure 'patch-build.xml
>> +           (lambda _
>> +             (substitute* "build.xml"
>> +               (("unit-test, ") "")
>> +               (("\\$\\{build.timestamp\\}") "guix"))
>
> Is using "guix" here as a timestamp safe?  Will nothing read the 
> manifest and expect an actualy timestamp?  (I've become unfamiliar with 
> most of java-land lately).

hamcrest-core is the only package I’ve encountered so far that adds a
timestamp to the manifest.  Anything can be put inside manifests, so I
don’t think it matters much.  We could replace the timestamp with the
commit date of the package expression, just so that we have an actual
date (and one that isn’t 30+ years in the past).

>> +         ;; Java's "getMethods()" returns methods in an unpredictable 
>> order.
>> +         ;; To make the output of the generated code deterministic we 
>> must
>> +         ;; sort the array of methods.
>> +         (add-after 'unpack 'make-method-order-deterministic
>
> Should we patch our java package instead?  Perhaps that can be saved as 
> a future exercise.

The official documentation states that the order of methods returned is
undefined.  I wouldn’t like to force sorting on all users of the
reflection API.  In this case it’s important as the method names are
written to a file in whatever order they are returned.  In general the
order is of no importance.

>> +              (snippet
>> +               '(begin
>> +                  ;; Delete bundled jar archives.
>> +                  (delete-file-recursively "lib")
>> +                  #t))))
>
> Is this very common in java packages?

I’d say it’s somewhat more common than in packages written in other
languages, but I wouldn’t yet feel comfortable doing this by default for
all Java packages.

> Otherwise LGTM.

Thanks for the review!

~~ Ricardo