mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Coffler <j...@taltos.com>
Subject Re: Review Request 58673: Fix FlagsFileTest to check for absolute path properly on Windows.
Date Fri, 28 Apr 2017 21:46:57 GMT


> On April 25, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/flags/parse.hpp
> > Line 96 (original), 96 (patched)
> > <https://reviews.apache.org/r/58673/diff/2/?file=1699762#file1699762line96>
> >
> >     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`.
> 
> Joseph Wu wrote:
>     This particular chunk of code is something long deprecated, so even if the "correct"
thing is to use `path::absolute`, we shouldn't change it.
>     
>     Instead (in a separate patch, and only if you want to), we should simply `#ifndef
__WINDOWS__` the entire conditional and the body of the if-statement.
> 
> Jeff Coffler wrote:
>     I don't quite understand this, as the end result is identical (on non-Windows platforms).
The old line here, as you can see, is:
>     
>     ```
>      if (strings::startsWith(value, "/")) {
>     ```
>     
>     This change calls `path::absolute`. That routine, for Linux, is doing the identical
thing:
>     
>     ```
>     #ifndef __WINDOWS__
>       return strings::startsWith(path, os::PATH_SEPARATOR);
>     ```
>     
>     It feels like changing the code here to `#ifndef __WINDOWS__` is adding changes/complexity
when the end result is exactly what you're asking for (no behaviorial change).
>     
>     Is your concern here that, for Linux, we may change what we believe an "absolute
path" to be? While possible, that strikes me as exceedingly unlikely. For a VERY long time,
the way to tell (on Linux) that a path is absolute is to just check the first byte of the
string.
>     
>     Please clarify what your concern is here. I'd like to understand what you're thinkin'
here, thanks.

I just pushed a new review, this one was not addressed pending feedback from Joe.


> On April 25, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 79-86 (patched)
> > <https://reviews.apache.org/r/58673/diff/2/?file=1699763#file1699763line79>
> >
> >     I think this got a bit out of date. We settled on `\...` being an absolute path,
not just `\?...` (per below implementation).
> 
> Jeff Coffler wrote:
>     I didn't have a test for this specifically. The above is accurate, though. The 1-3
cases are for files on disk. Network shares are a special case and, while technically an absolute
path, isn't related to files on disk.
>     
>     That said, the function does implement this properly. The comment isn't out of date.
The note was just to point out that we handled this as well.
> 
> Andrew Schwartzmeyer wrote:
>     Sorry, I mean that your third bullet should read `3. "\..."` instead of `3. "\?..."`.

The latest review clarifies that these paths are for files on Windows.


> On April 25, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/path.hpp
> > Lines 92-108 (patched)
> > <https://reviews.apache.org/r/58673/diff/2/?file=1699763#file1699763line92>
> >
> >     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.
> 
> Jeff Coffler wrote:
>     Yeah. Before this, the code was essentially the same (a bunch of boolean conditionals),
but rather "ugly" (three or four lines to represent the conditions with `||` and `&&`).
At first I resisted Andy's suggestion, but ultimately decided it was a lot more readable,
so I adopted it.
>     
>     Andy strongly recommended using a RegEx. While use of a RegEx allows for a nice one-liner
on Windows, it was problematic:
>     
>     1. Windows and Linux behaved differently. Indeed, Linux would fail given a Regex
that was perfectly valid and worked fine on Windows. Ultimately, this didn't matter, though,
since the Windows solution was Windows-specific.
>     
>     2. I've worked with RegEx engines before, and I knew that it wouldn't perform. RegEx
engines are great for a lot of cases, but this one (a few hard-coded boolean comparisons)
isn't one of those cases. And indeed, a RegEx for this was > 70x slower, which is pretty
dramatic. This code is ultimately a few boolean conditions, although it's 15 lines of C++
code. It is nice and readable, though, if you understand lambda expressions.
>     
>     I liked this in the end, so this was what I submitted. But behind it was performance
analysis and all! :-)
> 
> Joseph Wu wrote:
>     Hm... I'm not seeing why the lambda is necessary here...
>     
>     Don't we get the same result by putting all four boolean conditions in sequence?
>     
>     ```
>     if (strings::startsWith(path, "\\")) {
>       return true;
>     }
>     
>     if (path.length() < 3) {
>       return false;
>     }
>     
>     std::string letter = path.substr(0, 1);
>     if (!((letter >= "A" && letter <= "Z") || 
>           (letter >= "a" && letter <= "z"))) {
>       return false;
>     }
>     
>     std::string colon = path.substr(1, 2);
>     return colon == ":\" || colon == ":/";
>     ```
> 
> Jeff Coffler wrote:
>     We could do that, and that's plenty readable. The original code did the same sort
of thing, but in a more compressed way. I don't have the original, but it was along the line
of:
>     
>     ```
>     return strings::startsWith(path, "\\")
>       || ((path.length() >= 3 && (path.substr(1, 2) == ":\" || path.substr(1,
2) == ":/")
>           && ((path.substr(0, 1) >= "A" && path.substr(0, 1) <=
"Z")
>               || (path.substr(0, 1) >= "a" && path.substr(0, 1) <= "b")))
>     ```
>     
>     Something like that, anyway. I thought that the lambda was much better than that.
Your suggestion is fine, too, and is clear without the lambda. I'll implement that and post
another patch.
> 
> Andrew Schwartzmeyer wrote:
>     Heh, this is why more eyes are better. Go with Joe's, it's much better.

Latest review implements Joe's suggestion (without lambda). Tests pass successfully.


> On April 25, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/tests/CMakeLists.txt
> > Lines 44-59 (original), 44-59 (patched)
> > <https://reviews.apache.org/r/58673/diff/2/?file=1699764#file1699764line44>
> >
> >     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.
> 
> Jeff Coffler wrote:
>     Six of one, a dozen of another. I viewed the unit test changes as "part of" the implementation,
so I put them in a single commit. If Joe prefers, I can separate them out. I suppose I could
have had one for the implementation, and then one for the unit test changes (and cmake changes,
they were related).
>     
>     In my mind, all code should be tested, so the unit tests were "part of" the implementation
change. Thus, one commit. But if you take the viewpoint of "each commit should stand on it's
own", I could have easily had a separate commit for the cmake and unit test changes.
>     
>     Joe's call, if he wants me to make a revision, I'm happy to do it.
> 
> Joseph Wu wrote:
>     Keep the build change together with this one.  
>     
>     If you consider the change, you're basically adding a new file.  And it wouldn't
make too much sense to create a new file _without_ making a build file change.

No change made as per Joe's comments.


> On April 25, 2017, 11:16 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/tests/path_tests.cpp
> > Lines 156-173 (patched)
> > <https://reviews.apache.org/r/58673/diff/2/?file=1699766#file1699766line156>
> >
> >     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`.
> 
> Jeff Coffler wrote:
>     Trivial to add new test cases now (just one line per test case). I can add whatever
you'd like if Joe wants me to.
> 
> Joseph Wu wrote:
>     The more (comprehensive) the merrier :)
>     
>     For example, this could use some more drive letter name variety.
> 
> Jeff Coffler wrote:
>     I'm going to do a change anyway to eliminate the lambda. I'll add a few tests here,
specifically for network paths, our explicitly unsupported case of `\path` (since while Windows
calls that "absolute", it's not - it depends on the current disk of your CWD) and some additional
drive letter names.
>     
>     This will force a re-test on both Linux and Windows, so it'll take me a bit. I hope
to have a new review posted in the early afternoon.

New tests added as per my above comments. New tests (as well as original tests) pass on Windows.
Linux tests unaffected (but I did run them, they still pass).


- Jeff


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58673/#review173000
-----------------------------------------------------------


On April 28, 2017, 9:43 p.m., Jeff Coffler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58673/
> -----------------------------------------------------------
> 
> (Updated April 28, 2017, 9:43 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/3/
> 
> 
> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message