Hey Justin, Thanks a lot for taking the time to check out the rc and vote. A couple notes inline:
On Mon, Feb 22, 2016 at 4:32 PM, Justin Mclean <jus...@classsoftware.com> wrote: > Hi, > > +1 (binding). Nice work on the LICENSE. > > I checked: > - signature and hashes correct > - release name contain incubating > - DISCLAIMER exits > - LICENSE file has some minor issues (see below) > - NOTICE good > - unable to compile on OS X (but notes say it only has experiment support) > > Minor issues LICENSE file: > - BSD for Async HBase has "Google Inc.” in the 3rd clause probably a copy and > paste error? Hmm, I'm not seeing this -- got a line number? Line 614 in LICENSE.txt says "StumbleUpon". > - Path to WebRTC licensed files should include src/kudu/util/random.h > - Missing license in LICENSE for FindGMock [1] > - Possible incorrect Apache header on BSD license in file [2]? Should also be > in LICENSE. > - Header with copyright Cloudera which should be ASF? [3] Thanks, will fix the four above for our next release. > - Short form i.e. pointers to license file are preferred. Can you clarify what this means? I thought, with the BSD license in particular, it's important to reproduce the whole license because there is a "substitution" of a company name into one of the clauses? > There is possibly a more serious issue with the licensing of this file [4]. > See also [5][6]. > > From inside the file: > "Permission to make digital or hard copies of all or part of this work for > personal or classroom use is granted without fee provided that copies are not > made or distributed for profit or commercial advantage and that copies bear > this notice and the full citation on the first page. To copy otherwise, to > republish, to post on servers or to redistribute to lists, requires prior > specific permission and/or a fee.” > > May be serious enough for another RC? IMO Up to the RM/PMC to decide that or > fix in the next incubating release. > Right, we actually noticed this as well and got in touch with the ACM to clarify the licensing. We already fixed it for the next release in this commit: https://github.com/apache/incubator-kudu/commit/6991cd432d0be35743995b3ca7cb5eedc072e3cb Given it's not software so much as a template for a publication, there isn't a particular open source license associated with it. But, we received permission to redistribute, which we cited in the commit message above and added the header requested by the ACM. We figured that it wasn't necessary to re-spin the RC given the response was basically that we were in the clear. > A few other minor things: > - NOTICE file file line should probably be "Apache Kudu (incubating)" rather > than "Apache Kudu” Will fix. > - There’s another github mirror here https://github.com/cloudera/kudu - does > anyone else think that a little odd? Since we originally lived at that location, we have a lot of stars, watchers, and forks from that repository. So, we wanted to continue maintaining it as a mirror, though we've edited the header on the top of the page to indicate that it's not the primary source control. We've been figuring this is no different than other projects that have multiple github mirrors. > > JFYI The OSX compile error was (after about 1/2 hour of compiling things): > + make -j8 install > CDPATH="${ZSH_VERSION+.}:" && cd . && aclocal-1.14 -I m4 > /bin/sh: aclocal-1.14: command not found > make: *** [aclocal.m4] Error 127 Interesting. I run on Ubuntu but hopefully another Kudu developer will pipe up and might know what's going on here - probably missing some 'brew install <foo>' in the docs. -Todd -- Todd Lipcon Software Engineer, Cloudera --------------------------------------------------------------------- To unsubscribe, e-mail: general-unsubscr...@incubator.apache.org For additional commands, e-mail: general-h...@incubator.apache.org
