boostorg / process

Boost Process
https://www.boost.org/libs/process
115 stars 114 forks source link

Fixed close handles with Alpine and MUSL #376

Open dchansen opened 4 months ago

dchansen commented 4 months ago

The MUSL C library does not provide the close_range in unistd.h and Alpine linux therefore cannot compile boost::process::v2, at close_handles.ipp fails to compile.

This PR adds a check to only use close_range if we are using GLIBC.

klemens-morgenstern commented 4 months ago

Looks good, will merge if all tests pass.

klemens-morgenstern commented 4 months ago

Ok, I don't think this is correct, as it doesn't handle libc++. I'll need to add a MUSL test to CI.

t43rr7 commented 3 months ago

Hello, @dchansen, I've encountered the same issue, but with glibc. The close_range checks here would be a bit more complicated. I can include your changes in my patch, so it could resolve 2 issues with single PR.

dchansen commented 3 months ago

@t43rr7 That looks good to me. Could you make a PR for my branch? then I will merge it in.

t43rr7 commented 3 months ago

@dchansen I've opened PR to your branch. Looks like your branch's HEAD a bit behind origin develop, so there're a bit more changes that expected. Anyway, the most interesting diffs here. I've also added raw system call support which should work even on MUSL library.

Please, verify this changes on your setup, because I have no possibility to do it fast (with MUSL). Thanks!

UPD: Same changes can be verified in this PR

t43rr7 commented 3 months ago

@klemens-morgenstern please, look at the new changes

klemens-morgenstern commented 3 months ago

Can't we just put a close_range wrapper around the syscall into a detail namespace and use using ::close_range otherwise? That way we don't need to reimpl close_all.

t43rr7 commented 3 months ago

Do you mean something like "code below"?


int close_range_wrapper(...) {
#ifdef HAS_CLOSE_RANGE
    return ::close_range(...);
#elif defined(HAS_CLOSE_RANGE_SYSCALL) 
    return ::syscall(SYS_close_range, ...);
#endif
}

I like this idea :) But this PR is not from my fork, I can do it in similar opened PR. How do you feel about this?

t43rr7 commented 3 months ago

@klemens-morgenstern done in this commit

Please, see PR

t43rr7 commented 3 months ago

@klemens-morgenstern are there any news?