brndnmtthws / conky

Light-weight system monitor for X, Wayland (sort of), and other things, too
https://conky.cc
GNU General Public License v3.0
7.32k stars 620 forks source link

Fix OpenBSD build #2080

Closed g0mb4 closed 6 days ago

g0mb4 commented 2 weeks ago

Using OpenBSD 7.6.

Try to close #2073, unfortunately #2075 is not enough.

netlify[bot] commented 2 weeks ago

Deploy Preview for conkyweb canceled.

Name Link
Latest commit fc7dead199e9ef519dfb36a480f24b0d8bfb2499
Latest deploy log https://app.netlify.com/sites/conkyweb/deploys/674113562ec2b40008de05eb
Caellian commented 2 weeks ago

Not all these changes are needed for BSD to build.

I'm assuming you're basing these off of patches in BSD port because they look similar. Those patches do a lot of seemingly unnecessary modifications.

Me being mostly wrong For instance `void some_func(void)` is technically not needed anymore, esp. given that we use a C++ compiler, and is a C specific way of making a function explicitly 0-arg (cause errors when called with arguments). g++ and clang++ complain about this already I think. > As a matter of style, the C++ Core Guidelines recommend you don't use void to specify an empty formal parameter list. Changing return types of functions from void to int is also ~~out of scope of "Fixing BSD build" and is~~ another change that should ideally be removed from this PR. EDIT: ~~...because it doesn't help with getting OpenBSD to build and returning 0 everywhere doesn't really convey and information - the error value won't be handled specially anyway because conky ignores and skips most errors.~~ EDIT2: I was wrong twice on this. This change was required to link with conky methods because their signatures changed to (I assume) handle errors. Anyway, left it as is. Sorry for jumping to conclusions, I wasn't looking at the sources while writing the original comment. ~~Some of these are good changes, but I strongly suggest you remove them from this PR to reduce its surface area. Ideally, fixing BSD build shouldn't introduce any quirks to non-BSD code, so removing them will help when bugs have to be tracked down.~~ (EDIT: Didn't read the changed files, PR doesn't actually contain that many out of scope things) EDIT: Now that I got home I compared these changes to BSD port and they're not that similar, BSD port _does_ do `func(void)`, but it also does a whole bunch of other modifications that aren't included.

Also, while we're at this, adding an optional BSD workflow would make this more sustainable. I've started with this so I'll add it here if you don't mind. (tried it, very hairy and complicated, would have to run a VM which can take up to 20min to setup, and would have to host VM images somewhere, and actions don't work...)

Caellian commented 1 week ago

Referencing #2052 as cause of issues.

g0mb4 commented 1 week ago

I'm just running make and fixing the warnings/errors :)

I want it to compile first, then fix the issues with the other platforms.

g0mb4 commented 1 week ago

And yes, void f(void) is done by my autopiloted brain.

Caellian commented 1 week ago

Not me telling you to reduce changes and then refactoring the whole file :sweat_smile:

Uhm... got carried away, bsdapm.cc needed some love.

Caellian commented 1 week ago

I believe that pretty much covers my intent behind #2075. I'm building this from a minimal VM environment and would appreciate if you could test it @g0mb4. Feel free to implement any of the added stubs before requesting review.

For future reference (CI?), package names:

Minimal package dependencies:

Optional pkg deps:

g0mb4 commented 1 week ago

Wow! Thank you for the help! I'll test is as soon as possible.

g0mb4 commented 1 week ago

It works on my setup.

g0mb4 commented 1 week ago

I think adding more features really should be part of a new PR.

Caellian commented 1 week ago

Alright, then it's done.

I didn't test all possible features because built time was atrocious, but I think they're likely to work. Core, X11 stuff and LUA stuff is confirmed to build on different architectures.

Following BUILD_* features might warrant additional attention in separate PRs, but in general they should work if appropriate dependencies are installed: BUILD_* Works Port (/dependency)
AUDACIOUS should audacious
CURL should curl (in default install)
HTTP should libmicrohttpd
ICAL should libical
INTEL_BACKLIGHT unknown /sys/class/backlight/intel_backlight
IRC should libircclient
JOURNAL unlikely systemd
MYSQL should mysql.h provider (mariadb?)
NVIDIA nvidia support flaky NVCtrl header provider
PULSEAUDIO should pulseaudio
RSS should curl (default), libxml-2.13
WLAN not used; always enabled - packagers should enable it by default to avoid breakage ioctl and native headers
XMMS2 should xmms2

Adding CI would be nice even if OpenBSD isn't a primary support target because it would improve code quality by failing on non standard language/stdlib use.