Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should the '||' on libtest/cmdline.cc:207 be '&&'? #321

Closed
esabol opened this issue Sep 4, 2021 · 2 comments
Closed

Should the '||' on libtest/cmdline.cc:207 be '&&'? #321

esabol opened this issue Sep 4, 2021 · 2 comments

Comments

@esabol
Copy link
Member

esabol commented Sep 4, 2021

Just wondering if anyone else thinks this is a typo (see line 207)?

gearmand/libtest/cmdline.cc

Lines 207 to 210 in dd9ac59

#if defined(POSIX_SPAWN_USEVFORK) || defined(__linux__)
// Use USEVFORK on linux
flags |= POSIX_SPAWN_USEVFORK;
#endif

I noticed this patch for compiling gearmand on alpine:3.9 and wondered if it's correct:

https://github.com/artefactual-labs/docker-gearmand/blob/6e3d05f69f7653682cfdcf5bbb9fbb57b4c335f8/1.1.18/patches/libtest-cmdline.cc.patch

@esabol esabol added the question label Sep 4, 2021
@esabol
Copy link
Member Author

esabol commented Sep 4, 2021

Interestingly, this patch for libmemcached changes the line to

#if defined(POSIX_SPAWN_USEVFORK) || defined(__GLIBC__)

instead. I think that's the better way to go. What do you think?

@esabol
Copy link
Member Author

esabol commented Sep 4, 2021

I think it's kind of a moot point since musl (what Alpine uses instead of glibc) has defined POSIX_SPAWN_USEVFORK since 2017. Note that I was able to successfully bootstrap, build, and test gearmand on alpine:latest in PR #296 without this patch, so I guess this only applies to older versions of Alpine. But it's probably worth fixing anyway in case someone wants to do that?

https://git.musl-libc.org/cgit/musl/tree/include/spawn.h
https://git.musl-libc.org/cgit/musl/log/include/spawn.h

@esabol esabol self-assigned this Oct 6, 2021
SpamapS pushed a commit that referenced this issue Nov 1, 2021
* Issue #321: Improve compatibility with older versions of Alpine
@esabol esabol closed this as completed Dec 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant