Thank you for the thorough feedback Julian! We will address these in the next candidate and incorporate the Apache Rat script into our release process.
PS. The info about how to use the KEYS is very useful- this is new territory for all of us. --Marc On Mon, Mar 6, 2017 at 7:45 PM, Julian Hyde <jh...@apache.org> wrote: > -1 (binding) > > Sorry my review is so late; I know that you’ve already closed the vote; > but I figured my feedback would be more useful now than waiting for RC6. > > The release is actually in good shape; well done. My formal “-1” vote is > due to issue 0 (content of the vote email) and 5 (copyright year). Please > fix these show-stoppers in the next RC. Other issues are well worth looking > into, because others may raise them in the IPMC vote. > > Julian > > > 0. I found the release candidate at https://dist.apache.org/repos/ > dist/dev/incubator/quickstep/0.1.0/RC5/ <https://dist.apache.org/ > repos/dist/dev/incubator/quickstep/0.1.0/RC5/>. Please include its URL > and its hashes (the URLs of the hashes AND the actual hash values) in the > vote email next time. The proposition to be voted upon needs to be > self-contained and immutable. > > 1. KEYS [ https://dist.apache.org/repos/dist/dev/incubator/quickstep/KEYS > <https://dist.apache.org/repos/dist/dev/incubator/quickstep/KEYS> ] looks > a bit strange — usually the user & signers are at the top of each block > (see for example http://www.apache.org/dist/calcite/KEYS < > http://www.apache.org/dist/calcite/KEYS>) but import succeeded: > > $ gpg --import KEYS > gpg: key A3C29E12: public key "Jignesh Patel <jign...@apache.org>" > imported > gpg: key 5A29899A: public key "Marc Spehlmann (apache) <sp...@apache.org>" > imported > gpg: Total number processed: 2 > gpg: imported: 2 (RSA: 2) > > 2. Checked hashes and keys. (Let’s move to something stronger than MD5 and > SHA1 soon. Also, let’s do a key signing party so that Marc & Jignesh are in > the web of trust.) > > 3. The tar-ball contains KEYS. Not much point in that (because if someone > compromised the tar-ball they would undoubtedly insert their a compromised > KEYS file!), but no harm either. > > 4. I didn’t try to build (I’m not on a machine with sufficient > horse-power), but it’s good to see clear build instructions in the tar-ball. > > 5. Checked that LICENSE, DISCLAIMER, NOTICE. The year in NOTICE must be > 2017. > > 6. Are all of the lines in NOTICE *required*? People tend to put lines > into a NOTICE out of *courtesy*. But it is mis-placed courtesy because the > consumers of your software are *required* to reproduce your NOTICE > verbatim. If Quickstep Technologies and Pivotal Software contributed under > the ASL, then I don’t believe their copyright notices are required. > > In NOTICE, by “third_party/benchmark”, do you mean “third_party/src/tmb/ > benchmarks”? > > third_party/src/tmb/README.md says "TMB is part of the Quickstep project > (copyright Pivotal Software, Inc.) and is distributed under the same > license terms.” This statement is confusing now that Quickstep is now > Apache Quickstep. If TMB has been contributed to Apache Quickstep can we > just move it out of third_party? > > third_party/linenoise and third_party/gpertools don’t seem to be included > in the tar-ball, so the entry can be removed from NOTICE. > > 7. It is useful (not required) to include release notes, including changes > since the previous release. Some projects include a HISTORY.md file. > > 8. There are pivotalsoftware.com URLs in README.md and DEV_README.md. Can > you change these to apache.org <http://apache.org/>. > > 9. Apache Rat found the following files without headers (ignoring files > under third_party/src): > > ./.gitattributes > ./.gitignore > ./.gitmodules > ./.travis.yml > ./BUILDING.md > ./CREDITS.md > ./DEV_README.md > ./Doxyfile > ./GENERATOR_FUNCTIONS.md > ./README.md > ./WORKING_WITH_AN_IDE.md > ./build/vagrant/README.md > ./parser/preprocessed/SqlLexer_gen.hpp > ./parser/preprocessed/genfiles.sh > ./query_execution/README.md > ./relational_operators/tests/text_scan_faulty_golden_output.txt > ./relational_operators/tests/text_scan_faulty_input.txt > ./relational_operators/tests/text_scan_golden_output.txt > ./relational_operators/tests/text_scan_input.txt > ./third_party/README.md > ./third_party/download_and_patch_prerequisites.sh > ./third_party/reset_third_party_dir.sh > ./third_party/patches/benchmark/benchmarkCMake.patch > ./third_party/patches/benchmark/benchmarkSrcCMakeLists.patch > ./third_party/patches/gflags/CMakeLists.patch > ./third_party/patches/gflags/gflags_reporting.cc.patch > ./third_party/patches/linenoise/CMakeLists.txt > ./third_party/patches/linenoise/linenoise.c.patch > ./third_party/patches/linenoise/linenoise.h.patch > ./third_party/patches/re2/re2CMake.patch > ./types/README.md > > Add headers to as many of these as you can. > > 10. Apache Rat found 2 files that it considered to be GPL3: > > GPL3 ./parser/preprocessed/SqlParser_gen.cpp > GPL3 ./parser/preprocessed/SqlParser_gen.hpp > > But according to https://issues.apache.org/jira/browse/LEGAL-291 < > https://issues.apache.org/jira/browse/LEGAL-291> these files can be > distributed under ASL. So we’re good. > > > Julian > > > > > On Mar 2, 2017, at 12:25 PM, Harshad Deshmukh <hars...@cs.wisc.edu> > wrote: > > > > Hi Marc, > > > > +1 from me. Here's a brief summary of what I tested - > > > > 1. Download the source code - OK > > > > 2. Download the third party libraries (dependencies) - OK > > > > 3. cmake build - OK > > > > 4. Source code build - OK > > > > 5. Running tests - OK. > > > > > > On 03/02/2017 11:48 AM, Jignesh Patel wrote: > >> +1 from me! > >> > >> On 3/1/17, 10:48 PM, "Jianqiao" <jianq...@apache.org> wrote: > >> > >> +1. I'm good with the release. > >> Best, > >> Jianqiao > >> 2017-03-01 17:21 GMT-06:00 HAKAN MEMISOGLU < > memiso...@cs.wisc.edu>: > >> > Hi Marc, > >> > > >> > +1 from me. It works with Docker. > >> > > >> > However I also get an error when I try to build it with Mac. > >> > The linker cannot find some symbols that protoc uses. > >> > > >> > > On Mar 1, 2017, at 4:30 PM, Marc Spehlmann < > spehl.apa...@gmail.com> > >> > wrote: > >> > > > >> > > Jianqiao and I worked today to merge some PRs that we thought > should go > >> > in > >> > > the first release. Accordingly we have created another > candidate. > >> > > > >> > > As per Apache, *we require 3 +1 votes* from project members to > make this > >> > an > >> > > official release. > >> > > > >> > > Before voting, please test the release (build, run ctest). For > a guide on > >> > > how to test, GOTO release/README.md > >> > > > >> > > This vote will remain open for 72 hours or until we find issues > that we > >> > can > >> > > quickly correct and create another candidate. > >> > > > >> > > more details on > >> > > > >> > > https://cwiki.apache.org/confluence/display/QUICKSTEP/ > How+To+Release > >> > > > >> > > --Marc > >> > > >> > > >> > >> > > > > -- > > Thanks, > > Harshad > > > >
