Re: Review Request 40851: Windows:[1/2] Updated zookeeper-3.4.5.patch to fix VS2015 build.
> On Jan. 27, 2016, 3 a.m., Alex Clemmer wrote: > > Aside from the relatively minor comments below, I have one major > > suggestion: I'd like to point Windows builds at ZK commit > > 06d3f3fa1bff258e62c0670309ad1849b1434bb1[1], (or _some_ commit later in the > > version history) so that we can transition as much away from patching the > > ASM and .c files as we can. I'll volunteer to prepare a candidate fix for > > this patch tonight so we have something to complete the review tomorrow. > > > > My justification follows: > > > > (1) At some point, when the diff between the C/ASM source in the supported > > version and the version that compiles on Windows becomes big enough, it > > stops being worth it to maintain a .patch file instead of upgrading the ZK > > version. In this particular case, I am not comfortable replacing that ASM > > code with that function call without a code review from the ZK team, and > > since there is a competing implementation in the master branch (see issue > > in the tracker[2] and the `fetch_and_add` function in the relevant source > > file[3]) I propose we just upgrade the Windows build to point at a version > > of ZK with this patch. > > (2) We already point to commit hashes for other third party projects, like > > glog, so this is not a solution without precedent. > > > > My next steps are the following: > > > > * We'll probably need to keep the patch file, since we still need .vcxproj > > and .sln files that build on VS2015. > > * The current build solution basically unpacks a tarball from the > > `3rdparty` directory that has the Jute files pre-generated; we'll need to > > make another tarball that the Windows can download that also has the jute > > files generated. > > > > Let me know of other thoughts or issues; otherwise I'll see you on the > > other side with a (god willing) working version of this patch. > > > > [1] > > https://github.com/apache/zookeeper/commit/06d3f3fa1bff258e62c0670309ad1849b1434bb1 > > [2] https://issues.apache.org/jira/browse/ZOOKEEPER-1635 > > [3] https://github.com/apache/zookeeper/blob/trunk/src/c/src/mt_adaptor.c I happy to say that we have a working cadidate fix for these changes. They're in two temp commits: https://github.com/hausdorff/mesos/commit/3ed2626c11ddf45108d2a9711131cce80bd85dfb https://github.com/hausdorff/mesos/commit/945b239de3af4f4a9fc1b36c88d70f9913f1cda1 I'll work with Lawindi tomorrow to formulate them into cohesive patches as part of these two ZK patches he's made. - Alex --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40851/#review116500 --- On Jan. 26, 2016, 9:03 p.m., M Lawindi wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40851/ > --- > > (Updated Jan. 26, 2016, 9:03 p.m.) > > > Review request for Alex Naparu, Dario Bazan, Daniel Pravat, Alex Clemmer, and > M Lawindi. > > > Repository: mesos > > > Description > --- > > Windows:[1/2] Updated zookeeper-3.4.5.patch to fix VS2015 build. > > > Diffs > - > > 3rdparty/zookeeper-3.4.5.patch 3ca180d0c81f5de521ada7fb6c1c248a871ab2da > > Diff: https://reviews.apache.org/r/40851/diff/ > > > Testing > --- > > - Applied patch to original zookeeper 3.4.5 > - Successfully opened 2015 solution in VS2015 > - Cli and Zookeeper build successfully > > > Thanks, > > M Lawindi > >
Re: Review Request 40851: Windows:[1/2] Updated zookeeper-3.4.5.patch to fix VS2015 build.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40851/#review116447 --- Bad patch! Reviews applied: [39850, 39851, 39852, 39888, 39889, 40102, 40851] Failed command: ./support/apply-review.sh -n -r 40851 Error: .. 2016-01-26 20:47:03 URL:https://reviews.apache.org/r/40851/diff/raw/ [27690/27690] -> "40851.patch" [1] 40851.patch:211: trailing whitespace. 40851.patch:286: trailing whitespace. -lock xadd dword ptr [eax], ecx; 40851.patch:287: trailing whitespace. - lock xadd dword ptr [eax], ebx; 40851.patch:288: trailing whitespace. -mov result, ecx; // result = ebx; 40851.patch:290: trailing whitespace. - return result; warning: 5 lines add whitespace errors. 3rdparty/zookeeper-3.4.5.patch:202: trailing whitespace. + 3rdparty/zookeeper-3.4.5.patch:277: trailing whitespace. +-lock xadd dword ptr [eax], ecx; 3rdparty/zookeeper-3.4.5.patch:278: trailing whitespace. +- lock xadd dword ptr [eax], ebx; 3rdparty/zookeeper-3.4.5.patch:279: trailing whitespace. +-mov result, ecx; // result = ebx; 3rdparty/zookeeper-3.4.5.patch:281: trailing whitespace. +- return result; Full log: https://builds.apache.org/job/mesos-reviewbot/11058/console - Mesos ReviewBot On Jan. 26, 2016, 8:18 p.m., M Lawindi wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/40851/ > --- > > (Updated Jan. 26, 2016, 8:18 p.m.) > > > Review request for mesos, Alex Naparu, Dario Bazan, Daniel Pravat, Alex > Clemmer, and M Lawindi. > > > Repository: mesos > > > Description > --- > > Windows:[1/2] Updated zookeeper-3.4.5.patch to fix VS2015 build. > > > Diffs > - > > 3rdparty/zookeeper-3.4.5.patch 3ca180d0c81f5de521ada7fb6c1c248a871ab2da > > Diff: https://reviews.apache.org/r/40851/diff/ > > > Testing > --- > > - Applied patch to original zookeeper 3.4.5 > - Successfully opened 2015 solution in VS2015 > - Cli and Zookeeper build successfully > > > Thanks, > > M Lawindi > >
Re: Review Request 40851: Windows:[1/2] Updated zookeeper-3.4.5.patch to fix VS2015 build.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40851/ --- (Updated Jan. 26, 2016, 8:18 p.m.) Review request for mesos, Alex Naparu, Dario Bazan, Daniel Pravat, Alex Clemmer, and M Lawindi. Repository: mesos Description --- Windows:[1/2] Updated zookeeper-3.4.5.patch to fix VS2015 build. Diffs (updated) - 3rdparty/zookeeper-3.4.5.patch 3ca180d0c81f5de521ada7fb6c1c248a871ab2da Diff: https://reviews.apache.org/r/40851/diff/ Testing --- - Applied patch to original zookeeper 3.4.5 - Successfully opened 2015 solution in VS2015 - Cli and Zookeeper build successfully Thanks, M Lawindi
Re: Review Request 40851: Windows:[1/2] Updated zookeeper-3.4.5.patch to fix VS2015 build.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/40851/ --- (Updated Jan. 25, 2016, 11:21 p.m.) Review request for mesos, Alex Naparu, Dario Bazan, Daniel Pravat, Alex Clemmer, and M Lawindi. Summary (updated) - Windows:[1/2] Updated zookeeper-3.4.5.patch to fix VS2015 build. Repository: mesos Description (updated) --- Windows:[1/2] Updated zookeeper-3.4.5.patch to fix VS2015 build. Diffs (updated) - 3rdparty/zookeeper-3.4.5.patch 3ca180d0c81f5de521ada7fb6c1c248a871ab2da Diff: https://reviews.apache.org/r/40851/diff/ Testing --- - Applied patch to original zookeeper 3.4.5 - Successfully opened 2015 solution in VS2015 - Cli and Zookeeper build successfully Thanks, M Lawindi
