Hi Justin, Thanks for your feedback - I've replied inline below.
On Mon, May 5, 2014 at 6:28 PM, Justin Mclean <jus...@classsoftware.com>wrote: > Hi, > > As an IPMC/previous shepherd thought I give my feedback. > > Signatures and hashes all good > DISCLAIMER correct > Filename contains "incubating" > NOTICE and LICENCE good (checked all 3rd party bit of code and they all > seem to be there) > Not all files have correct headers - may or may not be OK > There's a few binaries in the release > Can compile from source > Test pass > > From a quick look there are python, CSS, JS, JSON and HMTL template (tpl) > files that are misisng headers. Some are 3rd party so may not be an issue > but the ones in src/main/resources/org/apache/aurora/scheduler may be? > Given the large number of files (207) it's a little hard to easily review > this. Has rat been run on the release? Filed https://issues.apache.org/jira/browse/AURORA-394 > > Binary files in release - some of these may be OK > /3rdparty/javascript/bower_components/angular/angular.min.js.gzip > /gradle/wrapper/gradle-wrapper.jar > /src/test/resources/org/apache/aurora/scheduler/app/AuroraTestKeyStore > /src/resources/org/apache/thermos/root/checkpoints/failure/coordinator.* > > Every Bower component is essentially a vendored JavaScript library checked in. It's pretty much equivalent to a pristine upstream jar. We do have a ticket (https://issues.apache.org/jira/browse/AURORA-353) to move from having them checked in to pulled in as part of the build; however our build is already relatively complex (code in Java, Python, and JavaScript, with Thrift, JNI and Python native module dependencies adding a C++ component as well) and I do want to try my best to ensure the project is buildable. > Re the gradle wrapper I was able to use gradle to compile without it so it > may not actually be required in the source release? The generated gradle wrapper script (and associated jar) eliminates the need to have multiple versions of gradle installed and allows the project to explicitly specify its dependency on a particular gradle version. While I'm certain the buildscript may work with different versions of Gradle, to me that's just another variable that threatens build reproducibility. Further, gradle documentation states "The Gradle Wrapper (henceforth referred to as the 'wrapper') is the preferred way of starting a Gradle build. The wrapper is a batch script on Windows, and a shell script for other operating systems. When you start a Gradle build via the wrapper, Gradle will be automatically downloaded and used to run the build." > > As it was not mentioned directly in the README etc I was unaware that you > should use gradlew command instead. I saw later that it is mentioned in > docs/developing-aurora-scheduler.md. > Good point, the top-level README is currently very sparse. I'm working on improving it now. > A few (very) minor things: > - Release file doesn't have apache in it's name, while not required it may > give extra legal protection [1] > Good idea, created https://issues.apache.org/jira/browse/AURORA-392 > - KEYS are usually outside release package > KEYS are available both inside and outside the release package. Do you suggest removing them from the source distribution? > - Not sure if this is possible, as i'm not familiar with gradle, but you > may want to consider a source and binary release with the binary having the > gradle jar? > This is a difficult question since there are several different components in play here. Our build system is capable of producing useful binary packages, as well as source packages in much more useful form (Python sdists and Java source JARs). However in the interest of more quickly producing a release candidate the community can use we haven't produced those artifacts, though we can in the future. > - BUILD is usually named BUILDING or the build information put in README. > These are actually BUILD configuration files for pants ( http://pantsbuild.github.io/build_files.html). > - I notice the .asc file has .md5 and .sha hashes of it in the RC > directory. There are probably not required. > Filed https://issues.apache.org/jira/browse/AURORA-393 > > I'm sure you know your own project better that me, so if I've mentioned > something that already been discussed or I've misunderstood something I > apologise in advance. The fact I was able to take the source package and > compile it will minimal effort certainly indicates to me that it's release > ready. > > Thanks for taking the time to have a look! We really appreciate the feedback. Best regards, Kevin -- Kevin Sweeney @kts
