Thanks Kim. Some replies inline. Robbie
On 2 October 2017 at 22:07, Kim van der Riet <[email protected]> wrote: > Hi Robbie, > > Thanks so much for your feedback. Nearly all of this is straight forward, > and I will make the changes. However, I do have some questions, and an > answer: > > On 09/25/2017 10:04 AM, Robbie Gemmell wrote: >> >> I kicked the tyres as follows: >> - Verified the signature and checksums. >> - Ran RAT to verify licence headers, result (see below) is good in >> terms of files being licenced however it does show some things (more >> below). >> - Gave things a build (against proton-c master/0.18.0). >> >> General question: are the proton tests dependent on proton >> master/0.18.0? Being lazy, I didn't try earlier versions as I had >> master installed, I just noted there didnt seem to be noted >> enforcement of the version used. If they do need it we should >> definitely wait for it to release. However even if they aren't it >> perhaps feels like it might still be a good idea to wait and align >> with it at this stage, given I'd guess thats mainly what has actually >> been getting tested with it of late? > > > The dependency on Proton is that of the various client APIs under test. > These need proton to compile, link and run. I agree that some sort of test > for a version of Proton should be included. > >> >> I noticed some issues that need fixed before release, and some things >> that I think could be improved before release: >> - You will need to update >> https://dist.apache.org/repos/dist/release/qpid/KEYS to add your key >> so folks can verify the signature. I used >> https://people.apache.org/keys/group/qpid.asc to get your key for now. >> - Can you change it to have a SHA512 checksum in a .sha512 file, >> rather than a SHA1, to match the ASF release policy recommendations >> and our other recent releases. > > Will do. > >> - I think using >> https://dist.apache.org/repos/dist/dev/qpid/interop-test/ would be >> nicer and more consistent with all the existing component dirs, rather >> than the added >> https://dist.apache.org/repos/dist/dev/qpid/qpid-interop-test > > Agreed. > >> - The RAT output reveals a lot of surprising files. For examples there >> is around 15 files about testing dtx, which it isn't even defined how >> to with the protocol yet. Some of the clients with related files also >> dont even support [local] transactions yet I believe. I think it would >> be a lot better if those files were removed for now. > > That was an oversight. Thanks for pointing that out. > >> - The build pom specifies the v17 apaache parent, whereas all the >> other components have used v18 for some time, so this should be >> updated to align various plugin versions. I've made that change on >> master. > > Great, thanks > >> - The pom versions are still SNAPSHOT, they need updated before the >> release is cut. Something like this would work: mvn >> org.codehaus.mojo:versions-maven-plugin:2.4:set -DnewVersion=0.1.0 > > Done. > I realise you havent actually pushed anything on this and I don't envisage us deploying artifacts at the point of release with the interop bits regardless, rather just sticking with the source, so its much less of an issue here than usual, but.. For clarity, ordinarily you only use a non-snapshot version in the commit forming the release, with the subsequent commit changing the version back to the next appropriate snapshot, helping ensure its clear what release the code is still building toward, and a given cut is done or not. >> - The JMS shim defines a SNAPSHOT dependency on qpid-jms. Releases >> should use released dependencies. > > Fixed. I assume it is still ok to use 0.18 or greater? > Typically I'd just go for the latest at the point of release, i.e. 0.25.0 currently (though 0.26.0 coming real soon). Folks can then just use that, or could use the proposed version property to toggle what is used on the command line when building. In the case of JMS clients theres not really going to be any API change to worry about so mostly any should work other than allowing for particular bugs. >> - The JMS shim defines dependency on JMS 2.0 and JMS 1.1 which seems >> odd (and the latter is even noted as being obsolete in the pom) > > OK, here I am stuck. Where do you see this? Are you meaning the shims java > source files? > In shims/qpid-jms/pom.xml at #L74. Though I actually overlooked when skimming it that its already commented out below the note. >> - The dependency version management details should probably be >> consolidated to the parent, with a nice property that can be overriden >> on the command line to pick versions. See any of the other java >> components for example. > > Will do. > >> - I think the Quickstart instructions need an overhaul before release: >> >> The instructions indicate several times that you must 'install' Qpid >> JMS before using. This makes little sense unless specifically testing >> changes you have made to the JMS client on master, thats about the >> only typical time a user would need to build and install the client. >> In other cases its just going to be retreived from maven central (or >> the apache snapshots repo) automatically when actually utilised. Given >> the requirement to build and install the interop tests (which it seems >> install a fat jar containing the client dependency) before running >> that should all just be seemless normally. The instructions make it >> out to be way more complicated than it actually is. > > Ok, so all that is needed is to let maven find it? I'll test that out, and > delete references to downloading/building qpid-jms. Yep. By default Maven will look for it in its local repo, then if its not there (or its a stale snapshot) and it isn't in offline mode then it will look in the configured repositories for it until found. If its found then it grabs it and sticks it in the local repo for future use, and if it cant be found anywhere then it will fail and indicate why. >> >> >> Even ignoring that, the actual steps given for installing the JMS >> client also have issues. First they direct people to build the source >> repo master, when steps for using a release tag would be better as we >> dont direct users to non-releases as a matter of course. Next, the >> instructions detail that the client is available to install from the >> Fedora 26 package repository, when actually following that instruction >> would result in installing a positively ancient version of the client >> into some fedora specific locations on the filesystem that wont even >> get used by a regular downloaded Maven install (not sure if it will if >> you use Fedoras packaged maven, maybe it does some very ugly >> overriding to allow it). Thats overlooking that it is already much >> older than the test shims declared *snapshot* dependency on the client >> which means it cant be used anyway. > > I assume this issue will "go away" if Maven finds it - see above. Also noted > for F23. Yep >> >> >> The "## 3. Build and install qpid-interop-test" instructions includes >> cloning the repo, which seems redundant as the person already got this >> package (or have the repo checked out already) and so dont need it. We >> also shouldnt direct regular users toward non-releases as a matter of >> course. The repo itself is mentioned in the readme if folks need it. > > Quite right, I need to reword this around unpacking the tarball instead. >> >> >> I think the Artemis instructions in "## 2. Obtaining a broker" are out >> of date. The configuration update you are directed to apply already >> seems to be present by default. > > Fixed. >> >> > > Thanks again for taking a detailed look, I appreciate your experience and > knowledge of the Java bits! > > Kim --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
