Juan Hernandez has posted comments on this change.
Change subject: build: use maven deploy and move jar handling to Makefile
......................................................................
Patch Set 1: (6 inline comments)
> I prefer the brew method of handling maven output, using the maven deploy to
> create maven repository of the artifacts.
>
> For my opinion this is healthier to do so and not to grab files out of the
> target directory at mid build.
It is true, this relies less on maven internals, like the use of "target"
directories. However this is so common and so unlikely to change that I won't
say it is wrong. That said, I think that this solution is good.
> Also, the jar installation within the destination tree should be done by
> Makefile so it can be reused by other packagers.
No problem with that.
> I have not test this on live machine yet.
Yes, please, test it.
> Juan, You know the application far better than me... what are the
> implications of the NOTES? Maybe we should fix the maven build to produce the
> two missing packages and rename the root to parent.
The location/presence of POMs inside the packages are not really important at
the moment, as we don't have any consumers for them. This will probably change
in the future when third party "plugins" start to pop-up, but only if they want
to build using the distribution specific build systems.
We also need to check if the "%add_maven_depmap" macro in the .spec works
correctly with the changes in the POM files, as it is very picky.
> Is there anything you don't like in this patch?
I don't like the fact that the complexity of the Makefile is growing. However I
understand it is needed. I am starting to think that a python script or a shell
script would be a better tool for the job, as in fact we are seldom using make
capabilities and we are using more and more imperative programming constructs.
But this shouldn't block this patch.
....................................................
File Makefile
Line 28: javadir=/usr/share/java
This mix of UPPER CASE and lower case ...
Line 64: vdsbroker
One artifact per line simplifies reading and patching.
Line 68: $(BUILD_FILE):
I would name this "build" (or "build_mvn" as it was before), it reflects better
what it is actually doing.
Do we really need this intermediate build file? It makes the build process a
bit more difficult to understand.
Line 87: all \
It is a bit weird that "install" requires "all", shouldn't it require "build"
(or $(BUILD_FILE)) instead?
Line 175: done
I have to say that I don't like this convolute scripts, with so many escaping
and quoting, but I understand it is needed.
At this point is where I would say that the Makefile is no longer the right
tool for the job.
....................................................
File packaging/fedora/spec/ovirt-engine.spec.in
Line 312: make PREFIX=%{buildroot} javadir=%{_javadir}
mavenpomdir=%{_mavenpomdir} install
This mix of UPPER CASE and lower case variable names here hurts my eyes ;-)
--
To view, visit http://gerrit.ovirt.org/6171
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9eb8b8f60f2be71b2054582b5e898ae996f324ac
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com>
Gerrit-Reviewer: Ofer Schreiber <oschr...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
