You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Santhosh Edukulla <sa...@citrix.com> on 2013/11/07 11:11:03 UTC
Review Request 15304: Added installation of marvin along with packaging to
mvn build
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15304/
-----------------------------------------------------------
Review request for cloudstack and Prasanna Santhanam.
Bugs: CLOUDSTACK-5073
https://issues.apache.org/jira/browse/CLOUDSTACK-5073
Repository: cloudstack-git
Description
-------
Currently, we dont install the Marvin packages with default mvn install. As part of this command currently, we just source
distribute the Marvin. For marvin,We assume user to have run its own installation of packages post distribution.
This way, some times if any new addition of package changes happens, user has to explicitly run installation of marvin.
If not installed,the changes are not available to tests and other code. With this change, we package and
install them as well
Diffs
-----
tools/marvin/pom.xml 0869248
Diff: https://reviews.apache.org/r/15304/diff/
Testing
-------
Tested that installation works post packaging
Thanks,
Santhosh Edukulla
Re: Review Request 15304: Added installation of marvin along with packaging
to mvn build
Posted by Prasanna Santhanam <ts...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15304/#review28367
-----------------------------------------------------------
I'm with Hugo on this. It's going to become harder to fit marvin's build, package and install phases into maven's lifecycle goals. The marvin.sync for that reason is maintained as a separate profile. Unfortunately, it's not in the path of the developer build and so not many use it. I think we can defer this change and later cleanly separate out marvin into its own pypi repo like cloudmonkey.
Then users can simply do
$ pip install cloudstack-marvin
$ pip upgrade cloudstack-marvin
This is a more clear instruction than mixing it with maven commands which I have never liked but had to facilitate in the past.
- Prasanna Santhanam
On Nov. 7, 2013, 10:11 a.m., Santhosh Edukulla wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15304/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2013, 10:11 a.m.)
>
>
> Review request for cloudstack and Prasanna Santhanam.
>
>
> Bugs: CLOUDSTACK-5073
> https://issues.apache.org/jira/browse/CLOUDSTACK-5073
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Currently, we dont install the Marvin packages with default mvn install. As part of this command currently, we just source
> distribute the Marvin. For marvin,We assume user to have run its own installation of packages post distribution.
> This way, some times if any new addition of package changes happens, user has to explicitly run installation of marvin.
> If not installed,the changes are not available to tests and other code. With this change, we package and
> install them as well
>
>
> Diffs
> -----
>
> tools/marvin/pom.xml 0869248
>
> Diff: https://reviews.apache.org/r/15304/diff/
>
>
> Testing
> -------
>
> Tested that installation works post packaging
>
>
> Thanks,
>
> Santhosh Edukulla
>
>
Re: Review Request 15304: Added installation of marvin along with packaging
to mvn build
Posted by Santhosh Edukulla <sa...@citrix.com>.
> On Nov. 7, 2013, 10:40 a.m., Hugo Trippaers wrote:
> > tools/marvin/pom.xml, line 88
> > <https://reviews.apache.org/r/15304/diff/1/?file=380419#file380419line88>
> >
> > How do we ensure it also works for people that use easy_install instead of pip?
1. The current way of usage is we just package as part of mvn install and leave it to user to install the Marvin package separate either through pip or otherwise. With this, as mentioned in review description, if we do any changes and package, then users has to separately install Marvin. This is ok and no issues either. Either user installs marvin every time he downloads and builds cs code or he is made aware that there is some change and its time to install. But, We have seen that any new change leads to this additional installation procedure and leads to failures if not installed and queries related to its failures as often people tend to miss the installation of Marvin for its usage.
2. So, we are adding this facilitation as part of packaging and then installation using "pip" ( widely used ) . If we see current sync procedure, we are already assuming that user has pip and so he is running it. We just extended that facility to install procedure as additional execution. Yes, we made some assumption( assuming it is right and pip is widely used ) and its current usage under marvin.sync, the same actually would have applied equally there. So, far users using sync, not much have complained of missing pip on their facility and any additional facility to use easy_install against pip. We may\maynot add support for easy_install but we can if the change to installation is ok to be added and its worth adding installing with easy_setup as well. We may encounter issues with multiple versions of easy_install on his setup again. pip should suffice i believe.
Regarding changes, it will do to local machine, this is same either user installs it explicitly or we do it through mvn. But, marvin anyways has been along with CS repo currently with different branches and versions, if user checksout x version, even marvin for that version is available and packaged. If he is trying to run his tests with out install, he may still face the issue and then may manually install it because tests for version x may not work with installed Marvin. Its just we are avoiding that install burden automatically. If he checksout another version of y and marvin version available will be packaged for that version but not installed, its just that he continues to use the installed version and tests with latest checked out code assumes that installed marvin works.
Iam ok with not installing marvin with mvn install and install it separately( good or bad ) but as mentioned in step1, users do complain of missing package failures when they run marvin tests and we recently has seen 4-5 mails to group as an illustration to this. We just added this support,post those mails raising issue with marvin.
- Santhosh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15304/#review28364
-----------------------------------------------------------
On Nov. 7, 2013, 10:11 a.m., Santhosh Edukulla wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15304/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2013, 10:11 a.m.)
>
>
> Review request for cloudstack and Prasanna Santhanam.
>
>
> Bugs: CLOUDSTACK-5073
> https://issues.apache.org/jira/browse/CLOUDSTACK-5073
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Currently, we dont install the Marvin packages with default mvn install. As part of this command currently, we just source
> distribute the Marvin. For marvin,We assume user to have run its own installation of packages post distribution.
> This way, some times if any new addition of package changes happens, user has to explicitly run installation of marvin.
> If not installed,the changes are not available to tests and other code. With this change, we package and
> install them as well
>
>
> Diffs
> -----
>
> tools/marvin/pom.xml 0869248
>
> Diff: https://reviews.apache.org/r/15304/diff/
>
>
> Testing
> -------
>
> Tested that installation works post packaging
>
>
> Thanks,
>
> Santhosh Edukulla
>
>
Re: Review Request 15304: Added installation of marvin along with packaging
to mvn build
Posted by Hugo Trippaers <ht...@schubergphilis.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15304/#review28364
-----------------------------------------------------------
I'm not sure this is a good idea. The install target doesn't actually install cloudstack. It "installs" the generated artifacts into the local maven repository, but it does't make any changes to the machine itself.
Adding this install procedure would cause our compile phase to make actual changes to the developers machine. Especially with multiple versions this might cause unintended behavior. Especially as the versions numbering of Marvin is not inline with the cloudstack code base.
tools/marvin/pom.xml
<https://reviews.apache.org/r/15304/#comment55185>
How do we ensure it also works for people that use easy_install instead of pip?
- Hugo Trippaers
On Nov. 7, 2013, 10:11 a.m., Santhosh Edukulla wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15304/
> -----------------------------------------------------------
>
> (Updated Nov. 7, 2013, 10:11 a.m.)
>
>
> Review request for cloudstack and Prasanna Santhanam.
>
>
> Bugs: CLOUDSTACK-5073
> https://issues.apache.org/jira/browse/CLOUDSTACK-5073
>
>
> Repository: cloudstack-git
>
>
> Description
> -------
>
> Currently, we dont install the Marvin packages with default mvn install. As part of this command currently, we just source
> distribute the Marvin. For marvin,We assume user to have run its own installation of packages post distribution.
> This way, some times if any new addition of package changes happens, user has to explicitly run installation of marvin.
> If not installed,the changes are not available to tests and other code. With this change, we package and
> install them as well
>
>
> Diffs
> -----
>
> tools/marvin/pom.xml 0869248
>
> Diff: https://reviews.apache.org/r/15304/diff/
>
>
> Testing
> -------
>
> Tested that installation works post packaging
>
>
> Thanks,
>
> Santhosh Edukulla
>
>