Fixes issue #789 where brpJavaRepack was negated #932
Conversation
…nstead of 'true')
YuvalItzchakov
changed the title
YuvalItzchakov
changed the title
| @@ -121,7 +121,7 @@ object RpmPlugin extends AutoPlugin { | |||
| (rpmProvides, rpmRequirements, rpmPrerequisites, rpmObsoletes, rpmConflicts) apply RpmDependencies, | |||
| maintainerScripts in Rpm := { | |||
| val scripts = (maintainerScripts in Rpm).value | |||
| if (rpmBrpJavaRepackJars.value) { | |||
| if (!rpmBrpJavaRepackJars.value) { | |||
There was a problem hiding this comment.
Yep. This is the actual bug fix. Thanks :)
| @@ -81,7 +81,7 @@ object RpmPlugin extends AutoPlugin { | |||
| rpmConflicts := Seq.empty, | |||
| rpmSetarch := None, | |||
| rpmChangelogFile := None, | |||
| rpmBrpJavaRepackJars := false, | |||
| rpmBrpJavaRepackJars := true, | |||
There was a problem hiding this comment.
Together with the fix this doesn't change the default behaviour, right?
I'm not building rpm's on a daily basis, so I'm not sure what impact this has. When we introduced
this it was because rpm:packageBin took a long time to extract and repack all the jar files, which
is IMHO wasted time. I would prefer false as a default.
There was a problem hiding this comment.
@muuki88 Right, I negated the default because this is basically the behavior people had without actually knowing it so far. The time impact is pretty major if you're doing daily builds. I was waiting for one of you guys to set the tone on this, i'll revert the default to false.
| %{_rpmconfigdir}/brp-strip-static-archive %{__strip} \ | ||
| %{_rpmconfigdir}/brp-strip-comment-note %{__strip} %{__objdump} \ | ||
| %{nil} | ||
| %global __os_install_post %(echo '%{__os_install_post}' | sed -e 's!/usr/lib[^[:space:]]*/brp-java-repack-jars[[:space:]].*$!!g') |
There was a problem hiding this comment.
There was a problem hiding this comment.
Going to try it now, I saw:
%define __jar_repack %nil
Yesterday. Will try.
2. Changed the default of repack from true to false, which means repack will not run by default.
| @@ -81,7 +81,7 @@ object RpmPlugin extends AutoPlugin { | |||
| rpmConflicts := Seq.empty, | |||
| rpmSetarch := None, | |||
| rpmChangelogFile := None, | |||
| rpmBrpJavaRepackJars := true, | |||
| rpmBrpJavaRepackJars := false, | |||
There was a problem hiding this comment.
The documentation regarding jar repackaging is now going to be outdated (and generally it's pointing to a blog post that no longer exists). Where is the documentation maintained?
2. Added a test to make sure java repack is ran when set to `true`.
|
Sorry for the delay @YuvalItzchakov I'm still getting used the new review tools by Github :) Changes look pretty good! Thanks for the effort :) |
Fixes #789 where the value of
rpmBrpJavaRepackJarswas required to be set tofalsein order for repacking not to occur.I've done three things:
Created a shorter pre script which utilizes
sedin a single liner, since there are versions which don't support multi line macros (when running the test I kept getting errors about macro having an empty body, see this for more). I also changed the macro prefix from %define to %global, as that's the recommended approach by Fedora.Negated the default flag from
falsetotrue. My assumption was people might not want to disable repackaging out of the box, but feel free to tell me otherwiseAdded a test to validate that the removal of brpRepack works when the flag is set to
false.