You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Leo Simons <le...@apache.org> on 2014/08/04 18:08:26 UTC
Review Request 24238: Fix mvn marvin.sync profile
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24238/
-----------------------------------------------------------
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
Re: Review Request 24238: Fix mvn marvin.sync profile
Posted by Leo Simons <le...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24238/#review49575
-----------------------------------------------------------
tools/marvin/pom.xml
<https://reviews.apache.org/r/24238/#comment86794>
I am unsure whether this --allow-external should be there. I needed it yesterday, but not today, and I can't figure out what changed.
- Leo Simons
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
>
>
Re: Review Request 24238: Fix mvn marvin.sync profile
Posted by Leo Simons <le...@apache.org>.
-----------------------------------------------------------
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.
Changes
-------
Implemented the alternative solution proposed by Rohit.
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 (updated)
-----
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
Re: Review Request 24238: Fix mvn marvin.sync profile
Posted by Rohit Yadav <bh...@apache.org>.
> 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 <ls...@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 <ro...@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
>
>
Re: Review Request 24238: Fix mvn marvin.sync profile
Posted by Leo Simons <le...@apache.org>.
> 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
>
>
Re: Review Request 24238: Fix mvn marvin.sync profile
Posted by Leo Simons <le...@apache.org>.
> 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?
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 :-)
- 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
>
>
Re: Review Request 24238: Fix mvn marvin.sync profile
Posted by Leo Simons <le...@apache.org>.
> 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
>
>
Re: Review Request 24238: Fix mvn marvin.sync profile
Posted by Rohit Yadav <bh...@apache.org>.
> 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.
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?
- Rohit
-----------------------------------------------------------
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
>
>
Re: Review Request 24238: Fix mvn marvin.sync profile
Posted by Rohit Yadav <bh...@apache.org>.
> 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 :-)
Hi Leo, was just going through reviewboard. Just wanted to discuss if this needs any improvements or my help?
- 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
>
>
Re: Review Request 24238: Fix mvn marvin.sync profile
Posted by Santhosh Edukulla <sa...@citrix.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/24238/#review49460
-----------------------------------------------------------
tools/marvin/setup.py
<https://reviews.apache.org/r/24238/#comment86531>
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.
- Santhosh Edukulla
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
>
>