OpenVPN
Fix non-systemd build
When building in a non-systemd environment, #include <systemd/sd-journal.h> causes the build to break.
Thank you for your contribution. How to do code contributions has unfortunately not been well communicated in this project so far. I just added a template giving some pointers.
Since this is a code change (even though trivial in this case), we do kindly request you to follow our contribution guidelines. I will give you some time to adjust to this, otherwise this change will be handled differently which may result in you not getting the credit deserved.
Thanks for the template, I tried to apply all required changes to the PR, joined the mailing list and sent a patch message.
Thanks a lot! I've seen the patch on the ML and will run some more tests on it over the weekend. I do wonder if #include "config.h" needs to be added before the #ifdef HAVE_SYSTEMD; since that macro is defined in config.h, based on what ./configure discovers and are told to enable or disable.
Ah fair point - my local build pipeline uses that patch already.
But since I only set it up for the non-systemd case, I never got to try the other case and the current change might just always have #ifdef HAVE_SYSTEMD false.
Just to be clear, your patch isn't wrong by itself. And config.h is generated on-the-fly by ./configure; manual changes here might be overwritten on the next compilation round.
But it looks like even the initial starting point (before your patch; commit 2f05b046) was wrong too! It didn't pull in config.h in the right places, so when compiling journald.o it would implicitly always expect systemd headers to be available, but the HAVE_SYSTEMD macro was accessible due to config.h being hidden further below a longer include chain [1].
So your fix is really appropriate and fixes a build-time bug, but it seems we also need #include "config.h" before the #ifdef you moved.
[1] log/logwriters/journald.cpp -> log/logwriter.hpp -> log/logevent.hpp -> dbus/glibutils.hpp -> dbus/exceptions.hpp -> config.h
Thanks for your update here. I've tested this, and it works fine on my RHEL-8 box - both with and without ENABLE_SYSTEMD set in config.h.
Can you squash these two commits into a single commit and send it as a v2 patch to the openvpn-devel mailing list? Then I can complete this one this week.
Thanks for all your helpful comments - I think I performed all necessary steps now.
Having such a minimal change was a great introduction into your dev workflow, maybe I'll join for some bigger ones in the future :).
Having such a minimal change was a great introduction into your dev workflow, maybe I'll join for some bigger ones in the future :).
Please do! :slightly_smiling_face: