> 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.
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. - Leo ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/24238/#review49460 ----------------------------------------------------------- On Aug. 4, 2014, 4:08 p.m., Leo Simons wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/24238/ > ----------------------------------------------------------- > > (Updated Aug. 4, 2014, 4:08 p.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/MANIFEST.in 3d373818c2096db12b61902a51f2fe39f2e0608e > 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 > >
