> On Aug. 4, 2014, 4:35 p.m., Santhosh Edukulla wrote:
> > tools/marvin/setup.py, line 35
> > <https://reviews.apache.org/r/24238/diff/1/?file=650483#file650483line35>
> >
> > Please tying like this to as a dependency on pom.xml, may unnecessarily
> > lead to tight coupling with mvn logic inside of setup.py. Currently, only
> > cloudstackAPI is the one which marvin relies upon while building cs through
> > mvn, otherwise we can remove that dependency as well, its in plan. Any
> > upload of marvin to external package download system will tie this
> > dependency on pom.xml.
>
> Leo Simons wrote:
> Yep, this change requires that there is a file named pom.xml which has in
> it <parent xmlns="...maven..."><version>...</version></parent>. That's why
> MANIFEST.in now has to include that file. It doesn't depend on maven being
> installed or available. It's pretty common practice with distutils to
> externalize the version into a VERSION file or similar, especially on
> multi-module projects. But, maven can't externalize _its_ versions, they MUST
> be in the pom. I guess you can choose:
>
> * have an ugly pom.xml file shipped in your sdist, but maven release
> plugin works for bumping _all_ versions, marvin versions stay synced with
> management server, you don't have multiple places to update a string (DRY),
> etc
> * don't have the pom.xml file in your sdist, and have the same version
> number repeated in 3+ places, leading to bugs like the one that just
> happened, ever-lasting confusing errors when bumping versions, difficulty
> configuring CI servers to inject different versions, and other such fun
> * add an ugly non-standard pre processor to the build process which
> injects version numbers into the various build files
>
> This patch represents my opinion on the subject: if you can, don't have
> maven, but if you have maven, respect it's authority over your versioning :-)
>
> I have no idea why you call this a dependency. It's just a VERSION file
> that happens to have some extra XML around it.
>
> Rohit Yadav wrote:
> Leo thanks for the patch, how about we add a statement in pom.xml that
> creates a version.py (or pick any other suitable name) file that only have
> version information and setup.py imports it. We can commit a static
> version.py file just in mvn fails this way Marvin dist won't have to depend
> on pom.xml file and we can still dynamically change version string?
>
> I think this will adress Santhosh's comment. Can you re-submit the patch
> with such a logic or any better logic?
>
> Leo Simons wrote:
> Hey Rohit, nice idea, yes I guess we can come up with something like
> that...I'm not sure it is a better addressing of the 'maven dependency'
> concern. With the patch as I wrote it, to build a proper 'for distribution'
> release of marvin, with the right version, using automated tooling (i.e.
> jenkins build that splices in the build number), you need the ability to edit
> pom.xml, and distutils. If we have maven generate the version.py, all of a
> sudden you _do_ need mvn installed to make a release!
>
> ...but I guess we can avoid _that_ if we have jenkins write the version
> in the same way maven would. Maybe allow an env variable override? Ok, yeah,
> that's fine with me, I can work that out, I'll provide a new patch tomorrow.
> Anything can be solved with another layer of indirection, I should've thought
> of that :-)
>
> Rohit Yadav wrote:
> Hi Leo, was just going through reviewboard. Just wanted to discuss if
> this needs any improvements or my help?
>
> Leo Simons wrote:
> Hey rohit, AFAICS the (v2) patch is good to go so if there's no other
> feedback I think it just needs merging...if you could do that it'd be great :)
Hi Leo, your patch was not git formatted but was applied and committed in your
name to avoid going back and forth about it. Please submit git formatted patch
in future. Thanks for the fix:
commit 2279289465441447790bc64f38624e03fc0ec78b
Author: Leo Simons <lsim...@schubergphilis.com>
Date: Wed Aug 13 11:15:41 2014 +0200
marvin: Fix marvin.sync profile, fixes regression from e10f8e8
Signed-off-by: Rohit Yadav <rohit.ya...@shapeblue.com>
- Rohit
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24238/#review49460
-----------------------------------------------------------
On Aug. 5, 2014, 8:43 a.m., Leo Simons wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24238/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2014, 8:43 a.m.)
>
>
> Review request for cloudstack and Rohit Yadav.
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Commit e10f8e887571e28f31dbcf02a0beac163a901fe1 broke
> mvn -X -Pdeveloper,marvin.sync -Dendpoint=localhost -pl :cloud-marvin
> Because the maven file had hard-coded 0.1.0.
>
> This fix removes the hard-coding completely, by having the python setup read
> the version from the maven pom.xml, and the maven setup using its version.
>
>
> Diffs
> -----
>
> tools/marvin/mvn-setup.py PRE-CREATION
> tools/marvin/pom.xml 3bf70e041c6bab051d7a184ff9c3c81ea31d2054
> tools/marvin/setup.py 555d67dfc491cf5ab52d36b417a6b5842ea6ad96
>
> Diff: https://reviews.apache.org/r/24238/diff/
>
>
> Testing
> -------
>
> Commands that work now for me include:
>
> mvn -X -Pdeveloper,marvin.sync -Dendpoint=localhost -pl :cloud-marvin
> python setup.py install
> python setup.py develop
> mvn -P developer -pl :cloud-marvin
>
>
> Thanks,
>
> Leo Simons
>
>
