Kattis / problemtools

Tools to manage problem packages using the Kattis problem package format.
MIT License
101 stars 70 forks source link

Avoid pipe2 in support/interactive #148

Closed thorehusfeldt closed 4 years ago

thorehusfeldt commented 4 years ago

pipe2 is linux-specific, so that make fails on BSD-based unixes such as OS X.

My (limited) understanding is that the flag in the 2nd argument pipe2(fd, O_CLOEXEC) in makepipe ensures that the pipes are closed upon exit, which can be achieved with fcntl in a platform-independent way.

https://github.com/Kattis/problemtools/blob/eb7b60722582b26d9a0ad75963f5d31616fba8f3/support/interactive/interactive.cc#L194

Linux man page: http://man7.org/linux/man-pages/man2/pipe.2.html

An easy fix is to revert that part of the code to the state before 0d38a8bc9a6f455e976aa9824f6329f6f7d1f790

simonlindholm commented 4 years ago

The changes in that commit should really be made conditional on Linux, yeah. Some previous discussion in #122.

thorehusfeldt commented 4 years ago

Like this:

#ifdef _GNU_SOURCE
const int PIPE_SIZE = 1<<20; // 1 MiB (the default max value for an unprivileged user)
#endif 
...
#ifdef _GNU_SOURCE
    if(pipe2(fd, O_CLOEXEC)) {
        perror("pipe failed");
        exit(EXIT_FAILURE);
    }
    if (fcntl(fd[0], F_SETPIPE_SZ, PIPE_SIZE) == -1) {
        perror("failed to set pipe size");
    }
 #else
    if(pipe(fd)) {
        perror("pipe failed");
        exit(EXIT_FAILURE);
    }
    for(int i = 0; i < 2; i++) {        
        set_cloexec(fd[i], 1);      
    }
 #endif

Not sure which variable to ifdef on. With the above changes, I can cleanly compile this part at least on OS X. (The other issue, apparently also known, is with libchecktestdata.)

thorehusfeldt commented 4 years ago

Re “the other issue”: the one-line patch at https://github.com/DOMjudge/checktestdata/commit/470b9e5d00d95748b7daba27ad38ff82076b8003 does indeed fix libchecktestdata, and with those two changes the whole problemtools suite seems to compile on OS X. (Well, there are some warnings about unused variables in libchecktestdata.)

thorehusfeldt commented 4 years ago

@simonlindholm : What’s the preferred way to test for “are we on Linux?”

simonlindholm commented 4 years ago

Not certain, I guess #ifdef __linux__?

thorehusfeldt commented 4 years ago

Closed in 9d85d66068950d990c25ef9f9e2c07f5eeb0f94c