> 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?
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 :) - Leo ----------------------------------------------------------- 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 > >
