[PATCH] gnu: Add maven-shared-invoker

  • Open
  • quality assurance status badge
Details
3 participants
  • Julien Lepiller
  • Ludovic Courtès
  • Artyom V. Poptsov
Owner
unassigned
Submitted by
Artyom V. Poptsov
Severity
normal
A
A
Artyom V. Poptsov wrote on 18 Jun 2022 16:58
(address . guix-patches@gnu.org)
87a6aavx17.fsf@gmail.com
Hello,

this patch adds Apache Maven Invoker[1] under the name
'maven-shared-invoker'.
- Artyom

References:

--
Artyom "avp" Poptsov <poptsov.artyom@gmail.com>
CADR Hackerspace co-founder: https://cadrspace.ru/
GPG: D0C2 EAC1 3310 822D 98DE B57C E9C5 A2D9 0898 A02F
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCgAdFiEE0MLqwTMQgi2Y3rV86cWi2QiYoC8FAmKt6AQACgkQ6cWi2QiY
oC9cEgf/UpCxSPeoPv+/adw8fiFQmMk27zVPpAGeMN3MIIhF0jhSDZ8umkzd35uC
+jIXr7j3yc4uySmN2GORBCcVY8+HzO0XBsohQze9o649MNerDp5J5lsCt/wBSFNQ
N2xZR9BL2qoJGXoo8R/HEX1QCllBVa1J3lTMMtgAV0AUK4iuvQOyRxFcHPB3IDsc
q3DTgCZlaR2nM2T/kkZkeEQrzb29D/ALYS5az+Roej/+teEU5BFKZXvWNhr0pfL5
g+LzHxE4Q5FRM8/8v4+uQ1SbnwEfY/27tjk9W6dg60ktOLJGV0lfzFwPww7cjBFk
oK00u/zhd0B/52Oy0FLK2ui3E0rkMA==
=nzEV
-----END PGP SIGNATURE-----

J
J
Julien Lepiller wrote on 18 Jun 2022 21:56
(name . Artyom V. Poptsov)(address . poptsov.artyom@gmail.com)(address . 56054@debbugs.gnu.org)
20220618215647.4e25fd30@sybil.lepiller.eu
Thanks for the patch! It's mostly good, but I have some comments below
:)

Le Sat, 18 Jun 2022 17:58:12 +0300,
"Artyom V. Poptsov" <poptsov.artyom@gmail.com> a écrit :

Toggle quote (24 lines)
> +(define-public maven-shared-invoker
> + (package
> + (name "maven-shared-invoker")
> + (version "3.2.0")
> + (source (origin
> + (method url-fetch)
> + (uri (string-append "mirror://apache/maven/shared/"
> + "maven-invoker-" version
> "-source-release.zip"))
> + (sha256
> + (base32
> +
> "0yhgxvwpmyfhqaksdfmj9c4ml4pj60gnin8bq1a92ximf1dyyjyc"))
> + (patches
> + (search-patches
> + "maven-shared-invoker-exception-handler-fix.patch"
> + "maven-shared-invoker-rename-test-classes.patch"))))
> + (build-system ant-build-system)
> + (arguments
> + `(#:jar-name "maven-shared-invoker.jar"
> + #:source-dir "src/main/java"
> + #:tests? #f)) ; Tests require Maven
> itself

How so? Tests are usually just junit tests and it's easy to run them.
How are they so different from the usual tests?

If it really requires maven, have you tried building it with the
maven-build-system? There's a way to remove plugins from the pom file,
so maven doesn't complain. The pom file doesn't look too complex, so I
think it could work.

Toggle quote (3 lines)
> + (propagated-inputs
> + (list maven-parent-pom-35))

Yes you should propagate the parent, but that's only because maven
needs it when it reads this package's pom file. So, please keep it and
install this package from its pom file, like the others :)

Toggle quote (6 lines)
> + (native-inputs
> + (list unzip
> + maven-surefire-plugin
> + java-javax-inject
> + java-junit))

I'm surprised here you need maven-surefire-plugin. What is it used for
exactly? From my understanding it can't be called outside of maven, and
we don't use maven to install this package.

The pom file lists java-javax-inject as a normal dependency, so it
should be propagated instead. The pom file also lists
maven-shared-utils. Is it needed? If so please add it to the propagated
inputs, otherwise fix the pom file (with a patch to upstream I guess).

Toggle quote (19 lines)
> + (home-page
> "https://maven.apache.org/shared/maven-invoker/index.html")
> + (synopsis "Invoke Maven programmatically")

> Sep 17 00:00:00 2001 +From: "Artyom V. Poptsov"
> <poptsov.artyom@gmail.com> +Date: Tue, 14 Jun 2022 23:53:13 +0300
> +Subject: [PATCH 1/2] MavenCommandLineBuilder: Fix exception handling
> +
> +*
> src/main/java/org/apache/maven/shared/invoker/MavenCommandLineBuilder.java
> + (setGoals): Catch 'Exception' instead of 'CommandLineException' as
> + 'CommandLineException' is never thrown in the "try" block.
> +---
> + .../apache/maven/shared/invoker/MavenCommandLineBuilder.java | 2
> +-
> + 1 file changed, 1 insertion(+), 1 deletion(-)
> +
> +diff --git

This looks like a simple patch, but I don't understand why it's needed.
Is this a fix from upstream? Did you create it? For what reason?

Toggle quote (7 lines)
> @@ -0,0 +1,126 @@ +From 4bce3183b25c44ab406c2f4d8541a0a520b15a3d Mon
> Sep 17 00:00:00 2001 +From: "Artyom V. Poptsov"
> <poptsov.artyom@gmail.com> +Date: Wed, 15 Jun 2022 07:09:29 +0300
> +Subject: [PATCH 2/2] test: Rename some classes to avoid name
> conflicts +
> +*

I'm lost. What's happening in this patch? Why do you need it
(especially since you couldn't run the tests anyway)? Is this a problem
with upstream, or some issue you encountered because of what Guix is
doing?
L
L
Ludovic Courtès wrote on 4 Aug 2022 11:11
Re: bug#56054: [PATCH] gnu: Add maven-shared-invoker
(name . Julien Lepiller)(address . julien@lepiller.eu)
87y1w4nzje.fsf_-_@gnu.org
Hey Artyom,

Did you have a chance to look at Julien’s comments below?

Thanks,
Ludo’.

Julien Lepiller <julien@lepiller.eu> skribis:

Toggle quote (93 lines)
> Thanks for the patch! It's mostly good, but I have some comments below
> :)
>
> Le Sat, 18 Jun 2022 17:58:12 +0300,
> "Artyom V. Poptsov" <poptsov.artyom@gmail.com> a écrit :
>
>> +(define-public maven-shared-invoker
>> + (package
>> + (name "maven-shared-invoker")
>> + (version "3.2.0")
>> + (source (origin
>> + (method url-fetch)
>> + (uri (string-append "mirror://apache/maven/shared/"
>> + "maven-invoker-" version
>> "-source-release.zip"))
>> + (sha256
>> + (base32
>> +
>> "0yhgxvwpmyfhqaksdfmj9c4ml4pj60gnin8bq1a92ximf1dyyjyc"))
>> + (patches
>> + (search-patches
>> + "maven-shared-invoker-exception-handler-fix.patch"
>> + "maven-shared-invoker-rename-test-classes.patch"))))
>> + (build-system ant-build-system)
>> + (arguments
>> + `(#:jar-name "maven-shared-invoker.jar"
>> + #:source-dir "src/main/java"
>> + #:tests? #f)) ; Tests require Maven
>> itself
>
> How so? Tests are usually just junit tests and it's easy to run them.
> How are they so different from the usual tests?
>
> If it really requires maven, have you tried building it with the
> maven-build-system? There's a way to remove plugins from the pom file,
> so maven doesn't complain. The pom file doesn't look too complex, so I
> think it could work.
>
>> + (propagated-inputs
>> + (list maven-parent-pom-35))
>
> Yes you should propagate the parent, but that's only because maven
> needs it when it reads this package's pom file. So, please keep it and
> install this package from its pom file, like the others :)
>
>> + (native-inputs
>> + (list unzip
>> + maven-surefire-plugin
>> + java-javax-inject
>> + java-junit))
>
> I'm surprised here you need maven-surefire-plugin. What is it used for
> exactly? From my understanding it can't be called outside of maven, and
> we don't use maven to install this package.
>
> The pom file lists java-javax-inject as a normal dependency, so it
> should be propagated instead. The pom file also lists
> maven-shared-utils. Is it needed? If so please add it to the propagated
> inputs, otherwise fix the pom file (with a patch to upstream I guess).
>
>> + (home-page
>> "https://maven.apache.org/shared/maven-invoker/index.html")
>> + (synopsis "Invoke Maven programmatically")
>
>> Sep 17 00:00:00 2001 +From: "Artyom V. Poptsov"
>> <poptsov.artyom@gmail.com> +Date: Tue, 14 Jun 2022 23:53:13 +0300
>> +Subject: [PATCH 1/2] MavenCommandLineBuilder: Fix exception handling
>> +
>> +*
>> src/main/java/org/apache/maven/shared/invoker/MavenCommandLineBuilder.java
>> + (setGoals): Catch 'Exception' instead of 'CommandLineException' as
>> + 'CommandLineException' is never thrown in the "try" block.
>> +---
>> + .../apache/maven/shared/invoker/MavenCommandLineBuilder.java | 2
>> +-
>> + 1 file changed, 1 insertion(+), 1 deletion(-)
>> +
>> +diff --git
>
> This looks like a simple patch, but I don't understand why it's needed.
> Is this a fix from upstream? Did you create it? For what reason?
>
>> @@ -0,0 +1,126 @@ +From 4bce3183b25c44ab406c2f4d8541a0a520b15a3d Mon
>> Sep 17 00:00:00 2001 +From: "Artyom V. Poptsov"
>> <poptsov.artyom@gmail.com> +Date: Wed, 15 Jun 2022 07:09:29 +0300
>> +Subject: [PATCH 2/2] test: Rename some classes to avoid name
>> conflicts +
>> +*
>
> I'm lost. What's happening in this patch? Why do you need it
> (especially since you couldn't run the tests anyway)? Is this a problem
> with upstream, or some issue you encountered because of what Guix is
> doing?
?