-----------------------------------------------------------
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
>
>
|