GNU bug report logs - #28841
[PATCH] New java packages

Previous Next

Full log

Message #94 received at 28841 <at> debbugs.gnu.org (full text, mbox):

From: Ricardo Wurmus <rekado <at> elephly.net>
To: julien <at> lepiller.eu
Cc: 28841 <at> debbugs.gnu.org
Subject: Re: [bug#28841] [PATCH 03/24] gnu: Add java-eclipse-jetty-test-helper.
Date: Wed, 18 Oct 2017 22:50:32 +0200

Hi Julien,

> From: Julien Lepiller <julien <at> lepiller.eu>
>
> * 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.