Thank you for the review Sergio :) On Mon, Nov 27, 2017 at 7:37 PM, Sergio Fernández <wik...@apache.org> wrote:
> Hi, > > * Incubator DISCLAIMER included. > * LICENSE file contains information that should go in the NOTICE. > Interested in which you think should be in there. The license file is pointing to licenses lower in the tree that contain their various items. One could argue the Copyright for the BSD etc should either be in LICENSE or NOTICE, but it is in those lower directories. > * Build worked fine on my desktop (Ubuntu 17.10, GCC 7.2.0). > > * I'd put the install instruction somewhere more prominent > than docs/install/index.md, and probably more CLI-friendly text document. > Actually I was confused by the very different instruction from > http://mxnet.apache.org/install/index.html So always take into account > usability of your source release. > This one always bothers me too. I get why there are lots of .md files, that's how life on GitHub works, but it makes life difficult when reviewing the source tarball. > * The KEYS file are not correctly linked in the VOTE email. They weren't > that hard to find at https://dist.apache.org/repos/dist/dev/incubator/ > mxnet/KEYS, but should be properly linked.. > > Event thoughmy vote for this RC1 is -1 (binding), because: > > * Binary nnvm/tvm/apps/android_rpc/gradle/wrapper/gradle-wrapper.jar is > distributed without NOTICE. > Noting that nnvm is a third party app (though one that some of the MXNet committers work on). Perhaps a bug report/patch could be submitted there. However, the gradle wrapper is under Apache 2.0, and does not have a NOTICE file (uppercase NOTICE). NNVM has an Apache 2.0 license in its root. It's not as user friendly as it could be, but I believe it is being distributed with license notice (lower case notice), which is all the license requires. (While Gradle does have additional licensing in their LICENSE file, I don't see any of that in the gradle-wrapper.jar). > * Files' header contain, after the normative ASF license header, confusing > copyright information that should be cleaned to avoid confusions. > If you mean: * Copyright (c) 2017 by Contributors Then that was reintroduced by specific request of a previous Incubator vote/Incubator PMC feedback. Not all the code is covered by a CLA/grant (I counted 400 contributors), so the original Copyright statement is maintained (the enormously ugly 'Copyright Contributors'). Thanks, Hen
