From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50915) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1e56N4-0006vv-RQ for guix-patches@gnu.org; Thu, 19 Oct 2017 04:40:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1e56N0-0004ci-8M for guix-patches@gnu.org; Thu, 19 Oct 2017 04:40:06 -0400 Received: from debbugs.gnu.org ([208.118.235.43]:40400) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1e56N0-0004cU-10 for guix-patches@gnu.org; Thu, 19 Oct 2017 04:40:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1e56Mz-00044t-IT for guix-patches@gnu.org; Thu, 19 Oct 2017 04:40:01 -0400 Subject: [bug#28841] [PATCH 03/24] gnu: Add java-eclipse-jetty-test-helper. Resent-Message-ID: References: <20171014233216.49c852f7@lepiller.eu> <20171014222349.12902-1-julien@lepiller.eu> <20171014222349.12902-3-julien@lepiller.eu> From: Ricardo Wurmus In-reply-to: <20171014222349.12902-3-julien@lepiller.eu> Date: Wed, 18 Oct 2017 22:50:32 +0200 MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Message-ID: <87bml441uv.fsf@elephly.net> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+kyle=kyleam.com@gnu.org Sender: "Guix-patches" To: julien@lepiller.eu Cc: 28841@debbugs.gnu.org Hi Julien, > From: Julien Lepiller > > * gnu/packages/java.scm (java-eclipse-jetty-test-helper): New > variable. [=E2=80=A6] I wonder: is there a better place for this than java.scm? The same question applies to all other patches in this series. Let=E2=80=99s 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.tool= chain/" > + "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 =E2=80=9C#t=E2=80=9D. > + (add-before 'build 'fix-build-path > + (lambda _ > + ;; FIXME: > + ;; This file assumes that the build directory is named "tar= get" > + ;; but it is not the case with our ant-build-system. Once w= e have > + ;; maven though, we will have to rebuild this package becau= se this > + ;; assumption is correct with maven-build-system. > + (substitute* > + "src/main/java/org/eclipse/jetty/toolchain/test/MavenTest= ingUtils.java" > + (("\"target\"") "\"build\"") > + (("\"tests\"") "\"test-classes\"")))) Same here. FWIW: I don=E2=80=99t 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" "TestingDirTes= t.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 =E2=80=9Csubstitute*=E2=80=9D to return a boole= an 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 =E2=80=9Cfor testing the Jetty =E2=80=9D with an appropriate short description of Jetty substitu= ted 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