Thanks. I updated the comments as you suggested, merged and pushed.

The code looks good. Obviously, I'm no Mac expert, so I'd say if it works on Mac 
OS X, it works :)

I think we can merge this now.

Jendrik, could you review the changes?
https://bitbucket.org/flogo/downward-issues/pull-requests/11/issue615/diff

Good to know. I think my clang accepted the option as well. We don't want to set
this option on the command line though. We only want to ignore a large set of
warnings from the OSI code, so this is done with #pragma statements.

I think I fixed it now, by wrapping the additional #pragma statement in an
#ifdef __clang__. This works for me and Michael both with gcc and clang.


Just for later reference: gcc >= 4.4 ignores options it does not recognize
unless there are other warnings as well
(https://gcc.gnu.org/gcc-4.4/changes.html search for "unknown options").
Apparently, this is not the case for #pragma statements.

> I also cannot find it in the clang documentation, so I guess its either only
> available in specific versions of clang or only in Apple's version of clang
> (or both).

I don't think the man pages are necessarily complete. The clang version on my
oldest Linux machine (3.4) understands -Wconstant-conversion.

In the long run, it's perhaps too optimistic that we'll be able to use exactly
the same options on clang and gcc. Would it be possible to add
-Wconstant-conversion to our option string for clang and see if that resolves
the issue?

We fixed some more issues with missing "override" statements. There is one more
issue: clang generates the warning "-Wconstant-conversion" for some OSI code. We
already ignore some warnings around this code, but the problem with
"-Wconstant-conversion" is that is is not recognized by g++. I also cannot find
it in the clang documentation, so I guess its either only available in specific
versions of clang or only in Apple's version of clang (or both). Ignoring a
warning that the compiler does not recognize generates another warning, so can't
just add it to the ignore list.

Michael's version is reported as "Apple LLVM version 7.0.0 (clang-700.0.72)" and
I also cannot find good documentation on that.

Yes, looks good to me.

Ahh, ok, if we wrap th LInux code in an #ifdef that is not an issue.

I think the functions write_reentrant and read_char_reentrant already do what we
wanted to do with safe_write and safe_read. Can you have a look at the pull request?

https://bitbucket.org/flogo/downward-issues/pull-requests/11

> C) introduce wrappers safe_open/safe_close 
>
> If I understood you correctly, you don't want C).

No, I think C) is an OK solution. But unless I've overlooked a caller, the only
occurrences of open/close in the code are ones that will always fail on the Mac
anyway. So there's an alternative solution D) to #ifdef the code that accesses
/proc on the Mac. One thing we could do to simplify the code is not to register
the signal handler on OS X at all. Or we could go with the code in
get_peak_memory_in_kb and test if it works in a signal handler.

Ahh, I was assuming that we do the same thing on OS X based on the exit handler.
You're right this is a bug and and will fail when we reach this code on OS X. I
guess we need a reentrant version of the code on OS X. We don't have that on
Windows as well, so for now, I can just add a TODO.

How do you want to handle open/close? If we leave it as it currently is in the
default branch, OS X will complain about the missing macro. If we leave out the
macro, then the behaviour on Linux changes. We could

A) define the macro as a no-op on OS X
B) make the while loop explicit in the code
C) introduce wrappers safe_open/safe_close 

If I understood you correctly, you don't want C). I find A) to be misleading,
because it would seem like "TEMP_FAILURE_RETRY(write())" would work, even though
you should use "safe_write()" instead. If we do B) I wonder why we don't do the
same for read and write. Is there another option that I don't see? Sorry for the
long discussion on a trivial issue, I just don't understand what you're suggesting.



As a side note: if the number of "#if OPERATING_SYSTEM == OSX" statement keeps
growing, we should think about splitting system_unix into system_unix,
system_linux and systems_darwin (or something like this).

Macs don't have a /proc filesystem; see issue275 and the "#if OPERATING_SYSTEM
== OSX" in get_peak_memory_in_kb. I guess our exit handler will fail on Macs.

> I'm not sure I fully understand your suggestion. If we introduce functions
> safe_read and safe_close (did you mean safe_write?), then we would still need
> either the macro or the while loop on Linux.

I meant safe_write, not safe_close, yes. I don't fully understand the rest of
this, though -- if we use a safe function (which would internally use a loop),
then I think it would make sense to use it on Linux, too.

Yes, the "file" we read is /proc/self/status. I didn't find a reason why this
would not be done on OS X. On both systems we register an exit handler, which
then calls print_peak_memory_reentrant.

I'm not sure I fully understand your suggestion. If we introduce functions
safe_read and safe_close (did you mean safe_write?), then we would still need
either the macro or the while loop on Linux. We would also need the loop/macro
for open and close (on Linux). So why wouldn't we need a wrapper for them? Did
you want to specifically avoid having the loop on OS X for system calls that
cannot be interrupted?

IIRC, the "file" in question is not a physical file but something in the /proc
filesystem to determine memory usage, is that right? In that case, it can never
be connected to a tty. But I seem to recall (perhaps wrongly) that this code
path is never used on the Mac anyway.

However, stdout/stderr are often connected to a tty, so we would indeed need an
EINTR wrapper there. From the last link I gave, which look authoritative enough
for me, we would need a wrapper for read and write, but not for open or close.
So I suggest we write safe_read, safe_close functions (or whatever we want to
call them) that handle EINTR.

Do you think we should do something different then? We currently use four system
calls that are affected: one call to open, read, and close for a file; and one
call to write to stdout/stderr.

I think using while loops as I did in the current pull request is an OK
solution. Its explicit and should work on slow and fast devices on both systems.

...but apparently only when the read/write is from/to a tty, where the
interruption makes sense to me:

https://lists.gnu.org/archive/html/bug-gnulib/2011-07/msg00063.html

Hmmm, on the other hand:

https://lists.gnu.org/archive/html/bug-gnulib/2011-07/msg00016.html

Hmmm, actually I'm not convinced by Florian's link. Check out this (search for
"BSD"):

http://www.gnu.org/software/libc/manual/html_node/Interrupted-Primitives.html

https://books.google.de/books?id=kCTMFpEcIOwC&pg=PA329&lpg=PA329&dq=%22slow+device%22+unix&source=bl&ots=zvIvWLSzmK&sig=6qbg10h1HrI7PXYfxm83MBJbzGc&hl=de&sa=X&ved=0ahUKEwisndDb_-fJAhWD1hQKHUC_AbEQ6AEINjAD#v=onepage&q=%22slow%20device%22%20unix&f=false

For those who were part of the discussion, here is the famous "worse is better"
article: https://www.jwz.org/doc/worse-is-better.html. (The context was more or
less similar to what we discussed, but it wasn't a BSD vs. SysV thing.)

The macro relies on a non-standard GNU extension. Malte had the suspicion that
the macro does not exist on OS X because the it is already part of the system
calls, i.e., that they cannot fail with EINTR on OS X. Unfortunately, this is
not the case
(https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man2/read.2.html).

I now avoid using the macro by adding a while loop around our system calls,
essentially making the macro substitution myself.

I think most of the error can be fixed by including <mach/mach.h> on OS X.
The macro TEMP_FAILURE_RETRY does not exist on OS X, so we have to define it
ourselves there. I found that others are doing the same and took the version
from http://fossies.org/linux/rose/src/frontend/SageIII/sage3basic.h.

I created a pull request for this on bitbucket, but we should wait with merging
until Michael can confirm that this actually fixes the problem :-) I cannot test
it here unfortunately.

https://bitbucket.org/flogo/downward-issues/pull-requests/11/issue615/diff

Apple does not support statically linked binaries
(https://developer.apple.com/library/mac/qa/qa1118/_index.html), so we should
use dynamic linking there. This requires some changes for clang and gcc.

Currently, we add the linker flags "-static" (both gcc and clang),
"-static-libgcc" (gcc), and "-static-libstdc++" (clang). On linux we still need
those flags even for clang (e.g., on our buildbot). On OS X, we should not add them.




Michael also reported errors when compiling without those flags (with gcc) most
of those look like an include to some header mach/mach_*.h is missing:

src/search/utils/system_unix.cc:43:72: error: 'TEMP_FAILURE_RETRY' was not
declared in this scope
         int written = TEMP_FAILURE_RETRY(write(filedescr, message, len));

src/search/utils/system_unix.cc:171:5: error: 'task_basic_info' was not declared
in this scope
     task_basic_info t_info;

src/search/utils/system_unix.cc:172:5: error: 'mach_msg_type_number_t' was not
declared in this scope
     mach_msg_type_number_t t_info_count = TASK_BASIC_INFO_COUNT;

src/search/utils/system_unix.cc:174:34: error: 'mach_task_self' was not declared
in this scope
     if (task_info(mach_task_self(), TASK_BASIC_INFO,

src/search/utils/system_unix.cc:174:37: error: 'TASK_BASIC_INFO' was not
declared in this scope
     if (task_info(mach_task_self(), TASK_BASIC_INFO,

src/search/utils/system_unix.cc:175:36: error: 'task_info_t' was not declared in
this scope
src/search/utils/system_unix.cc:175:50: error: 't_info' was not declared in this
scope
                   reinterpret_cast<task_info_t>(&t_info),
                                                  ^
src/search/utils/system_unix.cc:176:20: error: 't_info_count' was not declared
in this scope
                   &t_info_count) == KERN_SUCCESS)
                    ^
src/search/utils/system_unix.cc:176:37: error: 'KERN_SUCCESS' was not declared
in this scope
                   &t_info_count) == KERN_SUCCESS)
                                     ^
src/search/utils/system_unix.cc:176:49: error: 'task_info' was not declared in
this scope
                   &t_info_count) == KERN_SUCCESS)
                                                 ^
src/search/utils/system_unix.cc:177:9: error: expected ')' before 'memory_in_kb'
         memory_in_kb = t_info.virtual_size / 1024;

src/search/utils/system_unix.cc:177:50: error: suggest braces around empty body
in an 'if' statement [-Werror=empty-body]
         memory_in_kb = t_info.virtual_size / 1024;