[bug#28841] [PATCH 03/24] gnu: Add java-eclipse-jetty-test-helper.

[Ricardo Wurmus]

Hi Julien,

> From: Julien Lepiller <address@hidden>
>
> * gnu/packages/java.scm (java-eclipse-jetty-test-helper): New
> variable.

[…]

I wonder: is there a better place for this than java.scm?  The same
question applies to all other patches in this series.  Let’s try to
avoid a second python.scm situation :)

> +
> +(define-public java-eclipse-jetty-test-helper
> +  (package
> +    (name "java-eclipse-jetty-test-helper")
> +    (version "4.2")
> +    (source (origin
> +              (method url-fetch)
> +              (uri (string-append 
> "https://github.com/eclipse/jetty.toolchain/";
> +                                  "archive/jetty-test-helper-" version 
> ".tar.gz"))
> +              (sha256
> +               (base32
> +                "1jd6r9wc26fa11si4rn2gvy8ml8q4zw1nr6v04mjp8wvwpgvzwx5"))))
> +    (build-system ant-build-system)
> +    (arguments
> +     `(#:jar-name "eclipse-jetty-test-helper.jar"
> +       #:source-dir "src/main/java"
> +       #:test-dir "src/test"
> +       #:jdk ,icedtea-8
> +       #:phases
> +       (modify-phases %standard-phases
> +         (add-before 'configure 'chdir
> +           (lambda _
> +             (chdir "jetty-test-helper")))

Please end the phase with “#t”.

> +         (add-before 'build 'fix-build-path
> +           (lambda _
> +             ;; FIXME:
> +             ;; This file assumes that the build directory is named "target"
> +             ;; but it is not the case with our ant-build-system. Once we 
> have
> +             ;; maven though, we will have to rebuild this package because 
> this
> +             ;; assumption is correct with maven-build-system.
> +             (substitute*
> +               
> "src/main/java/org/eclipse/jetty/toolchain/test/MavenTestingUtils.java"
> +               (("\"target\"") "\"build\"")
> +               (("\"tests\"") "\"test-classes\""))))

Same here.
FWIW: I don’t think all Java packages will need to be repackaged to use
the maven-build-system.  We should make sure that we clearly separate
the packages needed to bootstrap maven from all the rest.

> +         (add-before 'check 'fix-paths
> +           (lambda _
> +             (with-directory-excursion 
> "src/test/java/org/eclipse/jetty/toolchain/test"
> +               (substitute* '("FSTest.java" "OSTest.java" 
> "TestingDirTest.java"
> +                              "MavenTestingUtilsTest.java")
> +                 (("target/tests") "build/test-classes")
> +                 (("\"target") "\"build"))))))))

Same as above: please end all phases with a boolean.

I would merge these two phases and add a comment between the different
substitutions.  In my opinion, all substitutions should happen as early
as possible in the build process.

In the future we would like “substitute*” to return a boolean value
depending on success or failure to substitute, and then it would be good
to fail early.

> +    (description "This packages contains helper classes for testing
> jetty.")

Could you rewrite this to “for testing the Jetty <flower watering
framework>” with an appropriate short description of Jetty substituted
for the placeholder?  :)

> +    (license (list license:epl1.0 license:asl2.0))))

What does this list mean?  Could you please add a comment above this
field?

Thanks!

--
Ricardo

GPG: BCA6 89B6 3655 3801 C3C6  2150 197A 5888 235F ACAC
https://elephly.net