hyprwm / Hyprland

Hyprland is an independent, highly customizable, dynamic tiling Wayland compositor that doesn't sacrifice on its looks.
https://hyprland.org
BSD 3-Clause "New" or "Revised" License
19.63k stars 829 forks source link

Support for musl based systems #3539

Closed notmentaloutlaw closed 11 months ago

notmentaloutlaw commented 11 months ago

Description

For musl systems execinfo.h and its functions are not able to be used and it fails to compile. You may suggest installing libexecinfo to mitigate this problem however for musl systems libexecinfo has been dropped from alpine as well as gentoo for a long time now because it really wasn't a solution. I have created a patch that will allow every other system apart from musl to retain the execinfo stuff but on musl systems it removes that feature so it can compile. I am not a C developer so there are maybe a few mistakes in this patch file but I would love to hear your thoughts about this and hopefully we can work on incorporating this.

diff --git a/src/debug/CrashReporter.cpp b/src/debug/CrashReporter.cpp
--- a/src/debug/CrashReporter.cpp
+++ b/src/debug/CrashReporter.cpp
@@ -1,7 +1,9 @@
 #include "CrashReporter.hpp"
 #include <random>
 #include <sys/utsname.h>
+#if defined(__DragonFly__) || defined(__FreeBSD__) || defined(__GLIBC__) || defined(__NetBSD__) || defined(__OpenBSD__)
 #include <execinfo.h>
+#endif
 #include <fstream>
 #include <signal.h>

@@ -78,8 +80,9 @@
     finalCrashReport += std::format("\n\nos-release:\n\t{}\n\n\n", replaceInString(execAndGet("cat /etc/os-release"), "\n", "\n\t"));

     finalCrashReport += "Backtrace:\n";

+#if defined(__DragonFly__) || defined(__FreeBSD__) || defined(__GLIBC__) || defined(__NetBSD__) || defined(__OpenBSD__)
     const auto CALLSTACK = getBacktrace();
+#endif

 #if defined(KERN_PROC_PATHNAME)
     int mib[] = {
@@ -106,6 +109,7 @@
     const auto FPATH = std::filesystem::canonical("/proc/self/exe");
 #endif

+#if defined(__DragonFly__) || defined(__FreeBSD__) || defined(__GLIBC__) || defined(__NetBSD__) || defined(__OpenBSD__)
     for (size_t i = 0; i < CALLSTACK.size(); ++i) {
         finalCrashReport += std::format("\t#{} | {}\n", i, CALLSTACK[i].desc);

@@ -117,6 +121,7 @@
         const auto ADDR2LINE = replaceInString(execAndGet(CMD.c_str()), "\n", "\n\t\t");
         finalCrashReport += "\t\t" + ADDR2LINE.substr(0, ADDR2LINE.length() - 2);
     }
+#endif

     finalCrashReport += "\n\nLog tail:\n";

diff --git a/src/helpers/MiscFunctions.cpp b/src/helpers/MiscFunctions.cpp
--- a/src/helpers/MiscFunctions.cpp
+++ b/src/helpers/MiscFunctions.cpp
@@ -6,7 +6,9 @@
 #include <sys/utsname.h>
 #include <iomanip>
 #include <sstream>
+#if defined(__DragonFly__) || defined(__FreeBSD__) || defined(__GLIBC__) || defined(__NetBSD__) || defined(__OpenBSD__)
 #include <execinfo.h>
+#endif

 #if defined(__DragonFly__) || defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__)
 #include <sys/sysctl.h>
@@ -686,6 +688,7 @@
     return subject;
 }

+#if defined(__DragonFly__) || defined(__FreeBSD__) || defined(__GLIBC__) || defined(__NetBSD__) || defined(__OpenBSD__)
 std::vector<SCallstackFrameInfo> getBacktrace() {
     std::vector<SCallstackFrameInfo> callstack;

@@ -702,8 +705,9 @@

     return callstack;
 }
+#endif

 void throwError(const std::string& err) {
     Debug::log(CRIT, "Critical error thrown: {}", err);
     throw std::runtime_error(err);
notmentaloutlaw commented 11 months ago

I know I should have probably created a PR instead of a feature request but I am quite unsure still of how git works but if there are no issues with what is above I am more than happy to create a PR.

vaxerski commented 11 months ago

didn't @jbeich already do that for bsd systems?

Anyways we don't support musl because I don't even know what musl really is and I care even less, as it seems to be like... a linux inside linux.

notmentaloutlaw commented 11 months ago

There isn't really much you have to know for musl other than it being a standard libc implementation. Everything that compiles for other libc's can also compile with musl and everything works just as fine. The only thing musl does not support in this case is and the backtrace functions.

My proposed patch only adds a few lines before and after the execinfo and backtrace functions in the code base. All this does is if it detects a BSD system or a glibc system it behaves just as you intended and there is no difference. However, if its a musl system it will just remove the lines that relate to those specific functions so that it can compile on musl. This means that its rather automatic and doesn't need anything new or specific to work on musl. What I meant with "support for musl" is just so that it can compile you dont have to actively do anything extra than what you are already doing.

Also in regards to what @jbeich did with bsd systems I believe you are referring to these such lines:

if defined(__DragonFly__) || defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__)

This is so that if on a BSD platform it will compile something specific to support those implementations. However, my line is

if defined(__DragonFly__) || defined(__FreeBSD__) || defined(__GLIBC__) || defined(__NetBSD__) || defined(__OpenBSD__)

This includes glibc linux systems. This means that for example lets take the first patch.

+#if defined(__DragonFly__) || defined(__FreeBSD__) || defined(__GLIBC__) || defined(__NetBSD__) || defined(__OpenBSD__)
 #include <execinfo.h>
+#endif

If its a BSD or glibc linux system please compile execinfo. If it isn't (musl) then please don't include it. I think its a great thing to have libc agnostic code that can work on various implementations. And I feel that since this patch is so small and a minuscule (This is everything as of 0.30.0) compared to the whole code base it would be very easy to have it in the code and more systems will be able to benefit from having hyprland!

Hope this is received well

NovaAndrom3da commented 11 months ago

I've used patches like this in the past and have been running Hyprland on a musl system (without gnulibc's execinfo) for several months now

vaxerski commented 11 months ago

if such patches exist, wouldn't it be the job of maintainers of musl systems to keep them up?

I have really no interest in official support for something I cannot maintain and triage

parona-source commented 11 months ago

@vaxerski Could you be instead be tempted with using boost::stacktrace (will be implemented in c++23 as std::stacktrace)?

Keeping in mind that execinfo currently has some problems of its own https://bugs.gentoo.org/904142#23 (boost::stacktrace can handle pie).

To my mind this would seem sensible as Hyprland uses C++23 features already, so adding stacktrace isn't out of place, while using execinfo from glibc is.

https://www.boost.org/doc/libs/1_83_0/doc/html/stacktrace/getting_started.html https://en.cppreference.com/w/cpp/compiler_support#C.2B.2B23_library_features

inferenceus commented 11 months ago

didn't @jbeich already do that for bsd systems?

Anyways we don't support musl because I don't even know what musl really is and I care even less, as it seems to be like... a linux inside linux.

If you're developing software, you really should care about these issues. Also, musl can easily be found via a few-second search. The response you made here does not give me trust or confidence in your abilities. There really is no need to be so hostile towards these very valid improvements.

As for these patches being implemented, it's very important to be libc-agnostic when possible, even if you do allow glibc extensions and such when people are using glibc (which most people are); however, musl is making quite an impact in the Linux space for various reasons, most notably how well-coded and correct it is vs glibc, and is thus a rising choice among lightweight and secure systems (it's effectively the OpenBSD of Linux libcs, and can be used elsewhere such as Android). musl users such as myself would very much appreciate you implementing these trivial patches so we can use this software on our systems; there are no doubt people who enjoy your software and find it useful, and this kind of behaviour is simply pushing them away.

You don't even have to maintain patches if you implement it properly as how @parona-source stated. It will make you a better developer along with providing more widespread usage of your software which you surely want people to use; it's a win-win situation.

eli-schwartz commented 11 months ago

and is thus a rising choice among lightweight and secure systems (it's effectively the OpenBSD of Linux libcs

The openbsd of anything is a terrifying thought. Yikes.

inferenceus commented 11 months ago

The openbsd of anything is a terrifying thought. Yikes.

This isn't really relevant to why these patches should be implemented and why musl (and other libcs) should be supported, but I'll play along for this one comment. I'm not willing to get further off-topic when there is an actual matter at hand.

So, you're saying well-coded and well-audited, as well as very small codebase, vs what most other people write, is a bad thing? Take a look at how much glibc is exploited and how many vulnerabilities it has had vs musl; yes, userbase matters a lot, but musl isn't increasing despite its userbase increasing, and it's still a good reference point. Whether you like it or not, musl is becoming a thing at accelerating pace; at the beginning of 2022, a lot of software didn't work on musl, now most of it does (at least, what I've come across), and people are more than willing in most cases to support it.

musl is heavily-focused on standards and making things work properly, without unnecessary extensions which deviate from that. While, like OpenBSD, it doesn't yet have some security mitigations such as CFI support, it's largely irrelevant when the code is written so well it can be audited many times in a short timeframe, unlike most other codebases such as glibc or Linux itself which are massive and practically impossible to audit by any amount of people (even Linus Torvalds doesn't audit everything he pulls, he trusts 10-15 people he receives the code from).

Either way, it's the developers' choice of whether or not to support musl and other libcs, but their hostility to their users is not going unnoticed and this issue has gained quite a bit of attention at this point among Linux users. Again, as @parona-source stated, this isn't difficult to implement, unless they want to brush it off out of spite, which is far from professional.

eli-schwartz commented 11 months ago

So, you're saying well-coded and well-audited, as well as very small codebase, vs what most other people write, is a bad thing?

No, I'm saying you've drunk the kool-aid, and swallowed a joke. Hook, line and sinker.

I'm sorry for your loss. OpenBSD is not good tech. If you want to use a BSD, then for your own sake, please choose literally any other BSD.

jbeich commented 11 months ago
+#if defined(__DragonFly__) || defined(__FreeBSD__) || defined(__GLIBC__) || defined(__NetBSD__) || defined(__OpenBSD__)

I'd prefer <execinfo.h> detection done at configure time (via CMake and Meson) instead of hardcoding a list of platforms for what is effectively platform-neutral code.

can be used elsewhere such as Android

See also https://github.com/aosp-mirror/platform_bionic/commit/2a9843fe63a3

vaxerski commented 11 months ago

Firstly, I wanted to mention that issues are not a place to submit patches. Those should be made as merge requests.

If you're developing software, you really should care about these issues.

oh really? About every system configuration? Should I also care about making sure it runs properly on templeOS, linux 3.1, darwin and nt?

Hyprland runs just fine on absolutely all the popular distros and their derivatives. Fedora, Arch, Debian.

If one has such a huge problem with glibc, one can use the patches themselves. I will not be testing every commit on 12 different configurations. I already have enough problems with testing even half of the features with each release.

hyprland-plugins/borders-plus-plus was broken for like 2 months before I noticed it.

You are speaking as if I am a paid developer. Guess what, I am not. This is a hobby.

To my mind this would seem sensible as Hyprland uses C++23 features already, so adding stacktrace isn't out of place, while using execinfo from glibc is.

clang doesn't support stacktrace yet. Once it does, I will shift to the std, just like I did with <format>.

I'd prefer <execinfo.h> detection done at configure time (via CMake and Meson) instead of hardcoding a list of platforms for what is effectively platform-neutral code.

Most sane solution, imo. We already do it with libsysd

NotAShelf commented 11 months ago

Should I also care about making sure it runs properly on templeOS, linux 3.1, darwin and nt?

what's the point of developing Hyprland if it can't run on ENIAC, MS DOS, first iteration of darwin and every modern iteration of the linux kernel at the same time?

eli-schwartz commented 11 months ago

oh really? About every system configuration? Should I also care about making sure it runs properly on templeOS, linux 3.1, darwin and nt?

Well yes, TempleOS is more popular so I would suggest supporting TempleOS before musl. :)

parona-source commented 11 months ago

clang doesn't support stacktrace yet. Once it does, I will shift to the std, just like I did with .

Which is why I pointed at boost::stacktrace. The standard library version is based on it and can be used in the interim while compiler support is work in progress (not even GCC is ready for general use yet).

https://en.cppreference.com/w/cpp/utility/basic_stacktrace

boost::stacktrace::basic_stacktrace (available in Boost.Stacktrace) can be used instead when std::basic_stacktrace is not available.

vaxerski commented 11 months ago

I refuse to use boost.

NovaAndrom3da commented 11 months ago

I believe a more acceptable and logical patch would be the one found here, but I digress. I'll whip up a PR later with these changes and we'll see what happens.