cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Leo Simons" <leosim...@apache.org>
Subject Re: Review Request 24238: Fix mvn marvin.sync profile
Date Wed, 13 Aug 2014 06:37:24 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message