> On July 12, 2016, 2:40 p.m., Alex Clemmer wrote: > > support/windows-build.bat, lines 32-33 > > <https://reviews.apache.org/r/49926/diff/1/?file=1441664#file1441664line32> > > > > I'm not super familiar with Jenkins, but I wonder why we create `tmp` > > in the Jenkins script, but create `build` here? It seems like we'd want to > > create the `tmp` file in this script, and leave the Jenkins script to do > > only the configuration stuff, like getting patch.exe? What do you think?
I left everything specific to the ASF machine in the Jenkins script. (i.e. things that have absolute paths) The `tmp` folder may or may not exist on someone's machine; and maybe it holds some non-temporary stuff (after all, `/tmp` is not a temporary folder on Windows :D ). I didn't want this build script to unilaterally create or delete things outside of the Mesos directory. --- This reminded me to add a check for the root directory. It doesn't make sense to run this script in any other location. > On July 12, 2016, 2:40 p.m., Alex Clemmer wrote: > > support/windows-build.bat, line 34 > > <https://reviews.apache.org/r/49926/diff/1/?file=1441664#file1441664line34> > > > > Do you need to check the error code for `MKDIR` too? I'm not super up > > on my batch semantics. I left it out, because the folder might already exist. Although, I think MKDIR in windows does not error if the folder already exists. (Either way, no need to check if `CD` will check for us.) > On July 12, 2016, 2:40 p.m., Alex Clemmer wrote: > > support/windows-build.bat, line 40 > > <https://reviews.apache.org/r/49926/diff/1/?file=1441664#file1441664line40> > > > > Interesting, why does Jenkins pass in `PATCHEXE_PATH` but not > > `ENABLE_LIBEVENT`? On Windows, libevent is required (you can't `cmake ..` without it). But the GNU Patch path is specific to the Jenkins box. > On July 12, 2016, 2:40 p.m., Alex Clemmer wrote: > > support/windows-build.bat, line 44 > > <https://reviews.apache.org/r/49926/diff/1/?file=1441664#file1441664line44> > > > > Heh. Do we want to actually explain that this is a bug? This comment > > seems more like an inside joke right now. :) I'm not clear whether this is a bug or not. We definitely want to explain why we're passing the option though. And AFAICT, specifying the `PreferredToolArchitecture` only changes the build time (by a lot). I don't think it changes the behavior of the executables. > On July 12, 2016, 2:40 p.m., Alex Clemmer wrote: > > support/windows-build.bat, line 1 > > <https://reviews.apache.org/r/49926/diff/1/?file=1441664#file1441664line1> > > > > Although we use this commend syntax in other batch scripts, we should > > avoid the LABEL syntax `::` and instead use `REM`. > > > > The latter is a standards-compliant, well-documented way of adding > > comments, while the former is not a commenting scheme at all. And because > > it is not a commenting scheme, there are many rules about where you can and > > can't place them (for example, you can't put any whitespace before the `::` > > symbols, and they can't appear in a `for` block), and if you disregard > > these things, you will get weird and hard-to-diagnose errors (for example, > > it might cause you to accidentally ignore parts of lexical blocks or output > > errors like `The system cannot find the drive specified` for seemingly no > > reason). > > > > For more information, see the bulleted list at the end of this post: > > http://www.robvanderwoude.com/comments.php I'll make the change, but it's a lot less pretty/readable though. I almost feel like I need to add a comment, explaining that `REM` lines are comments :D And it shows up when you have `@echo on`. - Joseph ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/49926/#review141965 ----------------------------------------------------------- On July 12, 2016, 4:16 p.m., Joseph Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/49926/ > ----------------------------------------------------------- > > (Updated July 12, 2016, 4:16 p.m.) > > > Review request for mesos, Daniel Pravat, Artem Harutyunyan, Alex Clemmer, and > Vinod Kone. > > > Bugs: MESOS-5822 > https://issues.apache.org/jira/browse/MESOS-5822 > > > Repository: mesos > > > Description > ------- > > This adds a script that will be run on the ASF CI, similar to how the > `docker_build.sh` script is used for the existing Linux-based CI. > > This Jenkins job for Mesos on Windows can be found here: > https://builds.apache.org/job/Mesos-Windows/ > > > Diffs > ----- > > support/windows-build.bat PRE-CREATION > > Diff: https://reviews.apache.org/r/49926/diff/ > > > Testing > ------- > > Pointed the Jenkins job at my local branch and triggered the build. > > This is what Jenkins is running: > ``` > :: Run a script provided by Visual Studio which sets up a > :: couple environment variables needed by the build. > CALL "F:/Microsoft/Visual Studio CE 2015/VC/vcvarsall.bat" > > @echo on > > :: Recreate the `/tmp` directory needed by tests. > RMDIR /q /s %CD:~0,3%tmp > MKDIR %CD:~0,3%tmp > if %errorlevel% neq 0 exit /b %errorlevel% > > :: We need to specify the path to GNU Patch, > :: since it is installed in a non-default location. > SET OTHER_CMAKE_OPTIONS=-DPATCHEXE_PATH="F:/Program Files (x86)/GnuWin32/bin" > > :: Call the Windows build script. > "support/windows-build.bat" > ``` > > > Thanks, > > Joseph Wu > >
