On Wed, Sep 5, 2012 at 10:23 AM, Wido den Hollander <[email protected]> wrote: > On 09/05/2012 04:17 PM, Chip Childers wrote: >> >> On Wed, Sep 5, 2012 at 10:08 AM, Pradeep Soundararajan >> <[email protected]> wrote: >>> >>> Hello Hugo/Chip, >>> >>> >>> >>> These are the review comments I have received so far. >>> >>> >>> >>> - For build-cloud.properties, that line should be optional. This means >>> add a '#' in front of it and others an make it required when copying it into >>> override. >>> >>> - The change for developer.xml to read in the build-cloud.xml should be >>> done as a precondition to all build targets. >>> >>> - The change does not take into account that the file may be on the class >>> path and so the property value should only be optional. Here it made it >>> explicit. >>> >>> - Also, we need to add the mysql connector as a rpm dependency. That >>> change is not in. >>> >>> >>> >>> Hugo >> I did a quick fix to get the build working again, just pushed the >>> commit to master. The mysql connector rpm is already installed on the >>> Jenkins server, I just modified the scripts to it is picked up by the build. >>> >>> >> I can add it to the spec file for rpm based distro's. I think it >>> should be added to the client rpm. >>> >>> >>> >>> I see that you have added the class path explicitly in order to resolve >>> this temporarily. >>> >>> >>> >>> # Add mysql jar from mysql-connector-java package to CP >>> >>> # for Jenkins >>> >>> CP=${CP}${PATHSEP}/usr/share/java/mysql-connector-java.jar >>> >>> >>> >>> Also, I have seen that you have added that rpm in ‘cloud.spec’ as well. >>> >>> >>> >>> Since you are calling mysql-connector explicitly from ‘deploy-db-dev.sh’ >>> class-path, the changes from ‘build-cloud.properties’ and ‘developer.xml’ >>> are not required from my patch. Please correct me if I am wrong. >>> >>> >>> >>> If everything is fine, shall we close this topic. Please share if anyone >>> have any comments? >> >> >> I *think* that's right. Let's see how the Maven build process shakes >> out, since this is actually tied to that as well. We know that >> mysql-connector needs to be a system dependency, and not be >> distributed with the ASF release. >> >> One item that does come to mind though... shouldn't it be included in >> the DEB packages too? Can you add that? >> > > Ubuntu and Debian both ship the MySQL connector in a package. I already > added a dependency for that: > https://git-wip-us.apache.org/repos/asf?p=incubator-cloudstack.git;a=commit;h=9064236879dc9f2f11538d891271545e1ff10e9c > > Wido
Great! Then I think you're right Pradeep. We can probably close out this topic for now. > > >>> Thanks, >>> >>> Pradeep.S >>> >>> >>> >>> >>> >>> -----Original Message----- >>> >>> From: Alex Huang >>> [mailto:[email protected]]<mailto:[mailto:[email protected]]> >>> On Behalf Of Alex Huang >>> >>> Sent: Friday, August 31, 2012 8:37 PM >>> >>> To: David Nalley; Alex Huang >>> >>> Cc: cloudstack; Pradeep Soundararajan >>> >>> Subject: Re: Review Request: CS-15694:Remove MYSQL connector. Added the >>> explicit class path for mysql connector which will call the jar from the >>> desired location >>> >>> >>> >>> >>> >>> ----------------------------------------------------------- >>> >>> This is an automatically generated e-mail. To reply, visit: >>> >>> https://reviews.apache.org/r/6752/#review10943 >>> >>> ----------------------------------------------------------- >>> >>> >>> >>> >>> >>> Also, we need to add the mysql connector as a rpm dependency. That >>> change is not in. >>> >>> >>> >>> - Alex Huang >>> >>> >>> >>> >>> >>> On Aug. 24, 2012, 10:19 a.m., Pradeep Soundararajan wrote: >>> >>>> >>> >>>> ----------------------------------------------------------- >>> >>> >>>> This is an automatically generated e-mail. To reply, visit: >>> >>> >>>> https://reviews.apache.org/r/6752/ >>> >>> >>>> ----------------------------------------------------------- >>> >>> >>>> >>> >>>> (Updated Aug. 24, 2012, 10:19 a.m.) >>> >>> >>>> >>> >>>> >>> >>>> Review request for cloudstack, David Nalley and Alex Huang. >>> >>> >>>> >>> >>>> >>> >>>> Description >>> >>> >>>> ------- >>> >>> >>>> >>> >>>> CS-15694:Remove MYSQL connector. Added the explicit class path for mysql >>>> connector which will call the jar from the desired location >>> >>> >>>> >>> >>>> >>> >>>> Diffs >>> >>> >>>> ----- >>> >>> >>>> >>> >>>> build/build-cloud.properties 552de7f74db7b715da70cc48ff4dc8945fa066f8 >>> >>> >>>> build/developer.xml f2e5aa6463ec849a3e97343e82423ff0ac622222 >>> >>> >>>> setup/db/deploy-db-dev.sh f149e9efd029bd311b1d247e21764b9103fd01d9 >>> >>> >>>> >>> >>>> Diff: https://reviews.apache.org/r/6752/diff/ >>> >>> >>>> >>> >>>> >>> >>>> Testing >>> >>> >>>> ------- >>> >>> >>>> >>> >>>> Executed: "ant build-all deploy-server deploydb" successfully. >>> >>> >>>> >>> >>>> >>> >>>> Thanks, >>> >>> >>>> >>> >>>> Pradeep Soundararajan >>> >>> >>>> >>> >>>> >>> >>> > >
