> On Nov. 7, 2016, 3:37 p.m., Benjamin Bannier wrote: > > 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).
To address your three points: When we add the various function prototypes in the `windows/*.hpp` files suffixed with `= delete;`, these are essentially TODOs. Currently, there are only a few of these remaining (`chroot`, `su`, `getuid`, `getgid`, `chmod`, `chown`, `glob`, and `mknod`). These are all functions defined in Posix, but with no functional equivalent in Windows. While it doesn't hurt to add more deleted functions, it also doesn't do the codebase any good (because these will never be implemented). We can't avoid #ifdef's. It's just a question of using `#ifdef __linux__` versus `#ifdef __WINDOWS__` (or similar). We have **tons** of Linux-specific code and #ifdef-ing already. `stout/os/xattr.hpp` wouldn't be appropriate, as there is obviously no cross-platform implementation. We can always add back the OSX implementation of xattrs if we ever use it. But currently, this code is only exercised in the Linux code. > On Nov. 7, 2016, 3:37 p.m., Benjamin Bannier wrote: > > 3rdparty/stout/tests/os/filesystem_tests.cpp, line 446 > > <https://reviews.apache.org/r/53555/diff/1/?file=1556079#file1556079line446> > > > > 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. If you want to be complete, the #ifdef would be: ``` #ifdef __linux__ || __APPLE__ || __FreeBSD__ || __NetBSD__ || __OpenBSD__ || __DragonFly__ ``` And we'd need to expand that for every potential OS that implements xattr :( Simpler to just limit it to where we use it. - Joseph ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53555/#review155213 ----------------------------------------------------------- On Nov. 7, 2016, 2: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, 2: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 > >
