> On Dec. 5, 2016, 6:33 p.m., Joseph Wu wrote: > > src/slave/constants.hpp, lines 141-144 > > <https://reviews.apache.org/r/54336/diff/5/?file=1577361#file1577361line141> > > > > There isn't any need to get rid of this constant, but we could update > > the comment. > > > > i.e. This is the _desired_ default, but if the path is inaccessible > > (posix non-root), the default will be a temporary folder. > > Alex Clemmer wrote: > I have a different view, actually. To me it seems like there's no > particular reason to have it, while there _are_ in my view a few good reasons > to not have it (and, I tend to believe that you should have to justify why > something exists rather than why it shouldn't exist). And, since it is used > only once, it costs nothing to change. > > * We should really try to keep path literals out of the codebase as much > as possible, because of the subtle bugs and problems with combining paths > with '\' and '/' on Windows. Even if it is safe in this case to use, I think > we should always think carefully about whether we have a good reason for > having a string literal path, and in this case, I don't see a clear benefit, > while I do see a clear risk of someone accidentally `path::join`'ing onto the > end at some point in the future. It seems like a better choice (as Jie > suggests in #54335) would be to implement this as `os::runstatedir`. See my > next point for more on this. > * To me, it makes sense to have a platform-indepenent variable data root > in a new function `os::var()` (_i.e._ `/var` on POSIX, and something like > `ProgramData` on Windows), and to use something like `path::join(os::var(), > ...)` to implement a function, `os::runstatedir()`. I think this is > reasonable in particular since (per conversation with Jie) we don't want to > change the default of this path away from `/var`. > > What are your thoughts? Am I missing something? > > Alex Clemmer wrote: > Oh, also, on the two points above, it's worth reading my next comment to > see a more complete picture for our current plan to accomplish the second > part in particular. > > Andrew Schwartzmeyer wrote: > I would like to point out that, as far as I can tell, this is the only > hardcoded absolute path in any of the `constants.hpp` headers. I think that > it should be removed as a matter of preventing its accidental use on a > platform for which it is incorrect. > > That said, I can see why you'd be not want to remove it given just this > commit, as it only moves the definition into the `Flags::runtime_dir`, which > is *also* not the right place for this. Perhaps it'd be appropriate to add > `os::runstate_dir` as Alex and Jie suggest to hold this platform-depenent > constant (keeping in mind that this commit also does not introduce the > correct Windows location of `ProgramData`).
Ok, sounds like a reasonable workaround for now. - Joseph ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54336/#review158103 ----------------------------------------------------------- On Dec. 5, 2016, 5:06 p.m., Andrew Schwartzmeyer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54336/ > ----------------------------------------------------------- > > (Updated Dec. 5, 2016, 5:06 p.m.) > > > Review request for mesos and Alex Clemmer. > > > Bugs: MESOS-6677 > https://issues.apache.org/jira/browse/MESOS-6677 > > > Repository: mesos > > > Description > ------- > > This commit fixes MESOS-6677, which breaks the ability > to run any agent on Windows, and thus is blocking all > Windows development progress on the `master` branch. > > The cause was that the default `runtime_dir` value was POSIX specific, > and used `os::user()` which is deleted on Windows. > The fix is to guard the POSIX code, and add a > Windows implementation. > > > Diffs > ----- > > src/slave/constants.hpp 6c381f06365b9deb84f43cdd101a2d2e5d826f57 > src/slave/flags.cpp 0de15eca7da9bf8fbdbb90c6e96edfe76f4a0f44 > > Diff: https://reviews.apache.org/r/54336/diff/ > > > Testing > ------- > > make && make check on Linux: 1411 tests passed, no failures. > > msbuild and attached to Linux master: no runtime failures. > > > Thanks, > > Andrew Schwartzmeyer > >
