> On Dec. 20, 2016, 12:29 a.m., Daniel Pravat wrote: > > 3rdparty/stout/include/stout/windows/os.hpp, line 748 > > <https://reviews.apache.org/r/54877/diff/1/?file=1589098#file1589098line748> > > > > I don't think the conversion to UTF-8 is appropiate here. > > Andrew Schwartzmeyer wrote: > What would you convert it to? It's currently in UTF-16, and Windows paths > are allowed to have (almost) any Unicode character. > > Alex Clemmer wrote: > I think he's saying that bad things will happen if you use strings that > have Unicode in them, so it's probably better to just error out. > > Andrew Schwartzmeyer wrote: > I'm not following... That is a case our codebase must deal with, NTFS > paths are stored in Unicode: > > > https://msdn.microsoft.com/en-us/library/windows/desktop/dd317748(v=vs.85).aspx > > and on top of that, Windows file paths can: > > > Use any character in the current code page for a name, including > Unicode characters and characters in the extended character set (128–255) > > > https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx > > So what I'm saying is that bad things _must not happen_ if we have paths > with non-ASCII characters in them; we need to handle that at any point we > deal with file paths on Windows. > > I'm not saying this function is likely to return a path like `C:\ÂÑÐ¥`, > but that would be a valid file path, and so we must be treating all our paths > on Windows as Unicode. > > If this is not the case, are purposefully constraining ourselves to > ASCII, and if so, why? > > Alex Clemmer wrote: > Sorry, I meant bad things will happen to Mesos if you give it strings > with unicode in them. Mesos itself does not deal with Unicode gracefully. > > Andrew Schwartzmeyer wrote: > Oooh. We should probably fix that.
Alex, Daniel, do we have a resolution on this? I spoke with Alex and we feel that we should be doing two main things: avoid exacberating the problem in MESOS-6817 by being explicit in our use of `W` APIs, rather than relying on the mercurial value of `TCHAR`; and not attempt to guard against paths that Mesos does not handle, as it is a bug that Mesos does not handle paths with valid extended characters (since both Linux and Windows may have these paths), so failing here would be inappropriate as it hides the actual bug. (Not to speak for you Alex, I think this summarizes our discussion yesterday.) - Andrew ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54877/#review159672 ----------------------------------------------------------- On Dec. 19, 2016, 11:20 p.m., Andrew Schwartzmeyer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54877/ > ----------------------------------------------------------- > > (Updated Dec. 19, 2016, 11:20 p.m.) > > > Review request for mesos, Daniel Pravat, Alex Clemmer, and Joseph Wu. > > > Repository: mesos > > > Description > ------- > > The API `SHGetKnownFolderPath` requires `Shell32.dll`, > which is not available on Nano server. > The equivalent API `GetAllUsersProfileDirectory` > only requires `Userenv.dll`, which is available on Nano. > > This API is also friendlier, as we own the allocation. > > The Unicode version `GetAllUsersProfileDirectoryW` is > explicitly used so that we are guaranteed a Unicode path, > which we then convert from UTF-16 to UTF-8, > instead of using the ASCII version which depends on a > varying Windows code-page, and is not recommended. > > A `vector<wchar_t>` is used over a `wstring` to avoid dealing > with the placement of the null-terminating character. > > > Diffs > ----- > > 3rdparty/stout/include/stout/windows.hpp > e641c46d033372e1b6c9f9c066b1ad4957d55088 > 3rdparty/stout/include/stout/windows/os.hpp > 5cd92545a49648e39e8eb7cf131895e9cfc97902 > > Diff: https://reviews.apache.org/r/54877/diff/ > > > Testing > ------- > > cmake && msbuild, attach agent to master and check default `runtime_dir` > value. > > > Thanks, > > Andrew Schwartzmeyer > >
