----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58673/#review173000 -----------------------------------------------------------
3rdparty/stout/include/stout/flags/parse.hpp Line 96 (original), 96 (patched) <https://reviews.apache.org/r/58673/#comment246063> There are more instances of erroneous absolute path checking through the code base, some of which we should probably fix. That said, this change should probably be in a separate commit/patch to address the particular bug, followed by more patches for the other instances (which don't need to come immediately). But I don't think I'd include this directly with the porting of `path::absolute`. 3rdparty/stout/include/stout/path.hpp Lines 79-86 (patched) <https://reviews.apache.org/r/58673/#comment246061> I think this got a bit out of date. We settled on `\...` being an absolute path, not just `\?...` (per below implementation). 3rdparty/stout/include/stout/path.hpp Lines 92-108 (patched) <https://reviews.apache.org/r/58673/#comment246062> I'm hoping Joe approves of this style; it's what I came up with while working with Jeff. It seems to be the most readable. For what it's worth, we also tried `std::regex`, and profiled it. Even in release mode with explicit optimization, it was 70x slower. 3rdparty/stout/tests/CMakeLists.txt Lines 44-59 (original), 44-59 (patched) <https://reviews.apache.org/r/58673/#comment246064> Personally, I am very liberal with commits, and would have a separate patch to touch the build system. But if Joe's happy, I'm happy. 3rdparty/stout/tests/path_tests.cpp Lines 156-173 (patched) <https://reviews.apache.org/r/58673/#comment246065> I am happy to have these, although we're missing cases like a valid network path `\server\share`, and our explicitly unsupported case of a relative `\path`. - Andrew Schwartzmeyer On April 25, 2017, 6:24 p.m., Jeff Coffler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58673/ > ----------------------------------------------------------- > > (Updated April 25, 2017, 6:24 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and > Li Li. > > > Bugs: MESOS-5937 > https://issues.apache.org/jira/browse/MESOS-5937 > > > Repository: mesos > > > Description > ------- > > Added 3rdparty/stout/tests/path_tests.cpp tests to Windows platform, > but disabled tests that did not pass. Enabled absolute path tests, > adding tests that were appropriate for the Windows platform. > > Note that, for Windows, absolute paths may not be valid. For example, > a path like "\\?\abc:file.txt" is not valid. In this case, the > path::absolute method is undefined; the expectation is that it is > called with valid paths (absolute or relative, but valid). > > > Diffs > ----- > > 3rdparty/stout/include/stout/flags/parse.hpp > 65edd86372596c2107e9f29cf27301e025e6620e > 3rdparty/stout/include/stout/path.hpp > 2d2088aadfa1ea82c59424242671c4fb655dede1 > 3rdparty/stout/tests/CMakeLists.txt > 4bbe713f259e7858d423dcb33956d41e62a915eb > 3rdparty/stout/tests/flags_tests.cpp > e2681f8f68f6478d4c8a20c1e75ddb050d16b56d > 3rdparty/stout/tests/path_tests.cpp > 0490d93908566c46a10d91b05790e5a7f2f289bc > > > Diff: https://reviews.apache.org/r/58673/diff/2/ > > > Testing > ------- > > Passes `make check` on the Linux platform. > > On Windows, the FlagsFileTest.JSONFile now passes, along with new > PathTest.Absolute tests that I added due to new path::absolute handling on > the Windows platform. > > PS C:\mesos> .\build\3rdparty\stout\tests\Debug\stout-tests.exe > --gtest_filter="*FlagsFileTest*" > Note: Google Test filter = *FlagsFileTest*- > [==========] Running 2 tests from 1 test case. > [----------] Global test environment set-up. > [----------] 2 tests from FlagsFileTest > [ RUN ] FlagsFileTest.JSONFile > WARNING: Logging before InitGoogleLogging() is written to STDERR > W0425 10:05:20.959357 1060440 parse.hpp:97] Specifying an absolute filename > to read a command line option out of without using 'file:// is deprecated and > will be removed in a future release. Simply adding 'file://' to the beginning > of the path should eliminate this warning. > [ OK ] FlagsFileTest.JSONFile (9 ms) > [ RUN ] FlagsFileTest.FilePrefix > [ OK ] FlagsFileTest.FilePrefix (6 ms) > [----------] 2 tests from FlagsFileTest (18 ms total) > > [----------] Global test environment tear-down > [==========] 2 tests from 1 test case ran. (23 ms total) > [ PASSED ] 2 tests. > PS C:\mesos> .\build\3rdparty\stout\tests\Debug\stout-tests.exe > --gtest_filter="*PathTest*" > Note: Google Test filter = *PathTest*- > [==========] Running 2 tests from 1 test case. > [----------] Global test environment set-up. > [----------] 2 tests from PathTest > [ RUN ] PathTest.Absolute > [ OK ] PathTest.Absolute (1 ms) > [ RUN ] PathTest.Comparison > [ OK ] PathTest.Comparison (0 ms) > [----------] 2 tests from PathTest (2 ms total) > > [----------] Global test environment tear-down > [==========] 2 tests from 1 test case ran. (6 ms total) > [ PASSED ] 2 tests. > > YOU HAVE 4 DISABLED TESTS > > PS C:\mesos> > > > Thanks, > > Jeff Coffler > >
