----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53555/#review155213 -----------------------------------------------------------
While it is clear how the current filesystem test can break under Windows, I do not understand why we need to wipe these utilities from all BSDs. The existing code already cleanly disabled the stout functions under Windows, and created useful wrappers for OS X which uses different parameters. What you suggest here seems to remove potentially useful functionality, and also force users of this library to `ifdef` code instead of us solving this in the library. I would prefer us writing mostly `ifdef`-free user-code as it is easier to understand and maintain, and exposes code to more setups. If you troubled by these functions living in the `stout/posix/` hierarchy, once could move all xattr related code to e.g., `stout/os/xattr.hpp`, and remove `stout/os/windows/xattr.hpp` and `stout/os/posix/xattr.hpp`. If there are issues under OS X we should fix them, and created related tests (the current test passes). 3rdparty/stout/tests/os/filesystem_tests.cpp (line 445) <https://reviews.apache.org/r/53555/#comment225048> This could just be `#ifndef __WINDOWS__`. It doesn't call into any Linux-specific functions to confirm the behavior of our wrapper, but instead uses observable effects that should work well under BSDs as well. - Benjamin Bannier On Nov. 7, 2016, 11:49 p.m., Joseph Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53555/ > ----------------------------------------------------------- > > (Updated Nov. 7, 2016, 11:49 p.m.) > > > Review request for mesos, Benjamin Bannier, Jie Yu, and Qian Zhang. > > > Bugs: MESOS-6360 > https://issues.apache.org/jira/browse/MESOS-6360 > > > Repository: mesos > > > Description > ------- > > The `xattr` utilities are Linux specific syscalls. Although these are > defined in some form on some flavours of Posix (such as OSX and BSD), > we do not intend to add the utility for all flavours of Posix. > > Hence, this moves the `xattr` functions into `stout/os/linux.hpp` > and removes any Posix-flavour-specific #ifdef-ing. > > This also fixes the Windows build by properly #ifdef-ing the related > filesystem test. > > > Diffs > ----- > > 3rdparty/stout/include/Makefile.am d5bc728ce378586e84173b98499b0d52047a83e1 > 3rdparty/stout/include/stout/os.hpp > bd085e4e29bbdb2d2baaaeff1d10c0bd95ca65ba > 3rdparty/stout/include/stout/os/linux.hpp > bf12e0b0577810c64cc8276ff0d987524327ffcd > 3rdparty/stout/include/stout/os/posix/xattr.hpp > 518940fdffab38ad97cf229078c4494fa944e1d8 > 3rdparty/stout/include/stout/os/windows/xattr.hpp > 3157f72c5a0d5fdf59dccb3a34b154e0bb719bfa > 3rdparty/stout/include/stout/os/xattr.hpp > b86b50addc9bfeb5ddefa757ea74d357df46fbeb > 3rdparty/stout/tests/os/filesystem_tests.cpp > 5c8f422443d0ea8e24b6a2bb506324966ce2e2ab > > Diff: https://reviews.apache.org/r/53555/diff/ > > > Testing > ------- > > See next review. > > > Thanks, > > Joseph Wu > >
