New Charm: IBM XL C/C++

[shilpa]

Hi,

Here is a new charm IBM XL C/C++ for review.

The charm will deploy a 60 day evaluation version of IBM XL C/C++ 13.1.2.To test this charm, a license to IBM XL C/C++ evaluation version is required.

Repo: https://code.launchpad.net/~ibmcharmers/charms/trusty/ibm-xlc/trunk

Thanks,
Shilpa

[Cory Johns (johnsca)]

Shilpa,

Thank you for this charm submission!

It's great to see the use of status-set to provide better feedback to the user during the deployment process! It's quite useful for the charm to report that it is blocked, and what it's blocked on, as well as when everything's installed successfully. There are a couple of cases, however, where the status reporting could be improved:

  * When the license is not accepted and the software was not previously installed, the status is left at "Removing IBM XL C/C++ software"; if the status-set on line 38 in config-changed was moved down to just after line 54, it would provide better feedback to the user in that case

  * If the URL is set but the package download fails (line 123), it is only sent to the log; a status-set there would be nice

I also notice that the install hook will error out if the charm is deployed on a non-Power system. I don't see anything in either the README nor during the download process for getting the compiler package that indicates it will only work on Power. If that's the case, it should be mentioned in the README and I would also recommend using a "status-set blocked" instead of "exit 1" (with a corresponding check in config-changed) because removing a service that is in error state can be difficult, and the user doesn't get the useful feedback as to what's wrong that the status-set would provide. On the other hand, if it does work on non-Power, perhaps that check can simply be removed.

There is also a minor syntax issue in the README's markdown; in the Usage section (line 42), the xlc-url config example should be escaped with back-quotes: `xlc_url: "http://<server-name>/<REPOSITORY_DIR>/xlc_url"`

Your test case worked, with a couple of caveats:

  * Running the tests from the charm root folder, I had to move the local.yaml file from where it was created (pwd) to where it was needed (tests directory). Perhaps using `dirname $0`/local.yaml would work?

  * The charm accepts an empty default value for the xlc_package_name config, but the test does not. Not a big deal, but would be nice if the test allowed for the empty default.

Finally, when thinking about how this charm would be used, it seems like this would make a lot of sense as a subordinate charm, similar to the IBM JRE charm. This would allow for a CI or compiler farm type charm to be created that used a pluggable compiler infrastructure, which seems interesting! I would suggest using a "c-compiler" or "cpp-compiler" interface (or both!) as the subordinate relation.

Aside from these suggestions, the charm looks over all pretty good, and I look forward to seeing the few corner cases mentioned above cleaned up. Thanks again!

[shilpa (shilkaul)]

Hi Cory,

Thanks for reviewing my charm and providing valuable feedback.I will be working on this soon.

[shilpa (shilkaul)]

Hi Cory,

I have implemented the review comments in my charm as suggested by you.I have doubts on two points :

Review Comment : Running the tests from the charm root folder, I had to move the local.yaml file from where it was created (pwd) to where it was needed (tests directory). Perhaps using `dirname $0`/local.yaml would work?

Doubt : I changed the code in my amulet and added , "local_path = os.path.abspath("local.yaml")"In this way wherever my local.yaml file is created , and user runs deploy.py script it works.Please let me know if this is approach is fine. I have also noticed few things:
If we run deploy.py script from "/home/charm/charms" folder and "/home/charm/charms/trusty/ibm-xlc/tests" folder it fails , saying that charm not found.Can you help in understanding why amulet fails when we run from these two locations.

Review Comment : Finally, when thinking about how this charm would be used, it seems like this would make a lot of sense as a subordinate charm,......................

Doubt : I have dropped you a separate mail on this review comment.If we make this charm as subordinate , then end user cannot use it until its principal charm or parent charm is deployed. Its a development tool , so if we keep it as a standalone charm , anyone who wants an environment to write and run C/C++ programs, can use it after deploying it.
Please let me know your views on it.

Thanks,
Shilpa

[shilpa (shilkaul)]

I am putting the bug in Fix Committed for further review.

[Cory Johns (johnsca)]

Shilpa,

The changes to the status reporting look great. Thanks for making those changes! It's really nice to see useful, informative feedback to the user in `juju status` indicating what they need to do to proceed.

Unfortunately, in my previous review I did miss one case that would be nice to add a status-set for: if the SHA check fails. Currently, the service would keep whatever the last status message was in that case and the user would have to check the logs to see what was wrong.

Regarding the test, in order for Amulet to know what charm it is testing, it has to be run from somewhere within the charm directory. There was a change submitted to Amulet to allow the tests to be run from within the tests/ directory, but I don't know if it was accepted. So, that means that the tests must be run from the root charm folder (/home/charm/charms/trusty/ibm-xlc/ in your case).

The issue that I had with the local.yaml file was that the 00-setup and 10-deploy.py tests were using different locations, with 00-setup using the current directory, whatever it happened to be, while 10-deploy.py was always looking in the tests/ directory. So, your change does address that and it works fine.

Finally, as I mentioned in my reply to your email, having a standalone dev environment is certainly a valid use-case for the compiler, but I do think it limits your options for being able to integrate with other services. As we now have several different compiler / runtime charms like this (Zulu8, XL Fortran, XL C/C++, ibmjavasdk), I wonder if it might be worth looking at using a layer approach for these so that they could satisfy your dev environment use-case and serve as a basis for other services as well. At any rate, that would take some design consideration, and if you think this charm is best served as a standalone dev environment, then we can move forward with that.

[shilpa (shilkaul)]

Hi Cory,

Have changed the status to In-Progress and will be working on the Subordinate feature.

Thanks
Shilpa

[shilpa (shilkaul)]

Hi,

Implemented the below changes for IBM-XLC charm

Implementing IBM-XLC as a Subordinate Charm
Placing the trial version packages in /files/archives directory
No License Flag required as it is Trial version

Thanks,
shilpa

[Review Queue (review-queue)]

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-lxc/1613/

[Review Queue (review-queue)]

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-aws/1596/

[Kevin W Monroe (kwmonroe)]

Hi Shilpa,

First, I wanted to say thanks for this charm! I think it will make a nice addition for any developer that wants to try the IBM XLC compiler on ppc64le. As a subordinate service, this also gives users the ability to deploy other development tools into a "development environment". The charm deployed without any problems, and the tests ran successfully!

I noticed that IBM has a repository for XL C/C++:

http://public.dhe.ibm.com/software/server/POWER/Linux/xl-compiler/eval/ppc64le/

To reduce the size of this charm, would you consider installing from that instead of packing the software into ./files/archive? I've created a merge proposal that does just that:

https://code.launchpad.net/~kwmonroe/charms/trusty/ibm-xlc/bug1489829/+merge/280933

This MP also includes README changes to reflect the changed installation location, as well as fixes to improve the source readability. Please consider this proposal, and if you are ok with the changes, merge into your ibm-xlc/trunk branch with:

bzr merge lp:~kwmonroe/charms/trusty/ibm-xlc/bug1489829

Then commit and push as normal.

[Kevin W Monroe (kwmonroe)]

Moving this bug to "In Progress" while Shilpa considers the requested merge proposal.

[shilpa (shilkaul)]

Hi,

Merge Changes have been incorporated and I have added config parameter "version" which allows user specific version to be installed.
The charm supports only evaluation version, the development team agreed to go ahead with charming of evaluation version as of now.

Putting this charm in Fix Committed

Thanks and Regards,
Shilpa Kaul

[Kevin W Monroe (kwmonroe)]

Hi Shilpa! Thanks for including the version config option. I think this makes ibm-xlc much more versatile to use when setting up a ppc64le development environment.

All tests passed and the recent additions to this charm look good. This charm is now promulgated and is available at https://jujucharms.com/ibm-xlc. It can be deployed with “juju deploy cs:trusty/ibm-xlc”.

The promoted charm project can be found in Launchpad at: https://launchpad.net/charms/+source/ibm-xlc

You can start monitoring for bugs at: https://bugs.launchpad.net/charms/trusty/+source/ibm-xlc

Thanks again for working to make this charm a high quality offering. Congratulations on making it into the charm store!