dmedvinsky / gsimplecal

Simple and lightweight GTK calendar (BSD license)
http://dmedvinsky.github.io/gsimplecal
Other
198 stars 19 forks source link

fix missing header sys/sysctl.h #39

Closed yauhen-l closed 3 years ago

yauhen-l commented 3 years ago

sys/sysctl.h does not exist anymore. Details: https://sourceware.org/pipermail/glibc-cvs/2020q2/069366.html

dmedvinsky commented 3 years ago

This doesn't seem very compatible way to solve this. sysctl is primarily used on BSD systems while the upstream change seems to be Linux specific. Looks like this will break building on FreeBSD and such. Maybe the correct way would be to #include the header only if AC_CHECK_HEADERS sets the corresponding config constant?

yauhen-l commented 3 years ago

Hi @dmedvinsky , I expected something like this. Since my knowledge is limited in c++ development an idea of this PR was to highlight the issue and fix it with your help as a maintainer of this project. Do you have time and interest in implementing your idea?

dmedvinsky commented 3 years ago

Unfortunately, I'm currently using macOS computer for my working needs and setting up Linux/BSD VMs with GUI is too much hustle to afford the time right now for me. I'm hoping to switch back to Linux (macOS kinda sucks) as I don't really need xcode anymore, but that still might take some time. So if anyone else can pick this up, that'd be awesome.

dive-deeper commented 3 years ago

The only place where sysctl is used is in Unique.cpp:

#elif HAVE_SYSCTL && defined(CTL_KERN) && defined(KERN_PROC) && defined(KERN_PROC_PATHNAME)
    // Try sysctl call (FreeBSD).
    int mib[4];
    mib[0] = CTL_KERN;
    mib[1] = KERN_PROC;
    mib[2] = KERN_PROC_PATHNAME;
    mib[3] = -1;
    size_t cb = PATH_MAX;
    sysctl(mib, 4, pathname, &cb, NULL, 0);
#else
...

The code is using the HAVE_SYSCTL macro anyway so I added a preprocessor instruction using it as well:

#if HAVE_SYSCTL
#include <sys/sysctl.h>
#endif
...

It compiled alright in Linux (OpenSuse Tumbleweed, kernel 5.10.5)

yauhen-l commented 3 years ago

Thank you @dionisiovj ! Works fine for me as well (Fedora 33, kernel 5.9.15-200.fc33.x86_64). I applied suggestion of @dionisiovj , could take a look as well @dmedvinsky ?

dmedvinsky commented 3 years ago

Yeah, thanks a lot, guys. I will try to get the virtual machine running and test it out during the week. But it totally makes sense to only include it if it exists, lol. So I will merge it anyway even if I fail to get some time to do the testing.

dmedvinsky commented 3 years ago

Sorry about being such a slowpoke. Thanks a lot for contribution. I've added you both to AUTHORS file.