Rename x11 platform to 'linuxbsd' #27266

ddevault · 2019-03-20T14:12:51Z

Also renames OS_X11 to OS_LinuxBSD, and a few other similar changes.

ddevault · 2019-03-20T16:30:52Z

Sorry about the noise while I sorted out Travis issues. This should be ready for review.

shartte · 2019-03-20T16:32:59Z

Why is the new platform not called POSIX? Isn't that what it is programmed against? Unix is after all trademarked :-)
(I know I am being pedantic here).

(edit) Also: What's the plan for Linux specific code?
I.e. you renamed crash_handler_x11.cpp to crash_handler_unix.cpp, but that file is using a Linux specific approach to getting the stacktrace.

modules/mono/config.py

modules/thekla_unwrap/SCsub

ddevault · 2019-03-20T16:45:18Z

Why is the new platform not called POSIX? Isn't that what it is programmed against?

Well, POSIX doesn't specify a display server like x11 or Wayland. Other code here, like power stuff, crash handler, etc, aren't specified by POSIX.

OS_Unix (which already exists) is a bit closer to OS_POSIX, but a lot of platforms depend on it so it seems wiser to take the approach of least risk of introducing a "generic unix" flavor which is closer to the behavior of Unix than the more specialized derivatives like macOS and Android.

ddevault · 2019-03-20T17:00:04Z

I.e. you renamed crash_handler_x11.cpp to crash_handler_unix.cpp, but that file is using a Linux specific approach to getting the stacktrace.

It's actually worse - this is a glibc-specific way of getting the stack trace. See this ticket:

#20037

I imagine that in the long run we should use feature detection at build-time to enable or disable specific implementations of this sort of thing. Or we could use runtime guesswork like we do in PowerUnix - which could easily be adapted for e.g. BSD in the future. I think that needn't be solved in this particular patch, though.

shartte · 2019-03-20T17:01:16Z

Yeah the crash handler aspect has always been tough to figure out. I'd have to check out a recent version of Google's breakpad to look at what they're doing now. Maybe they have a better abstraction?

edit: While I haven't looked into it in a while, I thought breakpad also used SCons, so we might just take inspiration from them in case they already have feature detection code for this kinda stuff.

ddevault · 2019-03-20T17:03:16Z

imo the crash handler is nice-to-have at best - loading up a core dump is going to give you more useful information and tools to work with anyway. I used to have crash handlers in my programs but eventually removed them. Do you think solving this problem is a blocker for this patch?

shartte · 2019-03-20T17:04:24Z

Oh I am in no position to actually decide any of that heh. I was just wondering whether you had a concept for handling sub-platform specific code in generic_unix, such as this crash handler.

ddevault · 2019-03-20T17:06:08Z

As the need for that grows, we can start writing some platform/generic_unix/-specific interfaces to abstract those things and add implementations in e.g. platform/generic_unix/linux/ or even platform/generic_unix/glibc/.

aaronfranke · 2019-03-21T07:32:26Z

I much prefer the name linuxbsd which explicitly describes what it actually is. "generic_unix" makes me think of Unix from the 1970s. Linux and BSD are not Unix, they are Unix-like.

Neither Linux nor BSD are certified Unix, but Mac, Solaris, AIX, FTX, UnixWare, HP-UX, z/OS, K-UX are. It is very misleading to make the name containing "unix" when it doesn't actually run on any of those.

ddevault · 2019-03-21T14:55:26Z

Calling it linuxbsd would be misleading as well, and doesn't describe it for what it is either. I don't see any reason why any of the code here wouldn't run on Minix, OpenSolaris, macOS, Cygwin, or any of several more niche systems. No one takes Unix certification seriously, and none of the code here depends on the niche Unix features which are unique to certified Unicies. "Generic Unix" is used all over the internet to refer to generic Unix-like systems, for which Linux and BSD certainly qualify.

That being said, we don't need to bikeshed this to death. If you insist on a particular name I'll update the patch to use it.

aaronfranke · 2019-03-21T17:35:27Z

The final decision is not mine, it will probably be @akien-mga 's.

Megalomaniak · 2019-03-21T17:42:26Z

I'll just add here that I've seen users on forums be confused about what build to download for linux before because of the x11 naming. Not sure how much better calling it generic unix is going to be but I expect it to be at least an improvement.

toger5 · 2019-03-21T18:25:27Z

I checked the code real quick and it looks good. Didn't check every singe function tough, but the general idea of separating the DisplayDriver implementations by using inheritance is good Imo.
It would be amazing if @hpvb could also give an opinion on the whole topic.

vnen · 2019-03-21T18:54:10Z

Calling it linuxbsd would be misleading as well, and doesn't describe it for what it is either. I don't see any reason why any of the code here wouldn't run on Minix, OpenSolaris, macOS, Cygwin, or any of several more niche systems.

I don't think it is misleading because it is meant to run on Linux and *BSD. While it could run on other systems, it's not meant to, so you expect to experience incompatibilities.

In any case, I think that opening a PR (worse: many PRs) just to decide a naming convention is a waste of time. We should discuss on an issue until we reach a conclusion and then change the code for good. No use making the same changes for each name suggestion, just to later discard all the rejected ones.

ddevault · 2019-03-21T19:01:01Z

I don't think it is misleading because it is meant to run on Linux and *BSD. While it could run on other systems, it's not meant to, so you expect to experience incompatibilities.

What part of this is only meant to run on Linux or *BSD? Only some of the code is Linux-specific and is optional. The code will compile and run on any POSIX system with an X11 server.

In any case, I think that opening a PR (worse: many PRs) just to decide a naming convention is a waste of time

That's not what this PR is for. This PR also includes a considerable amount of cleanup from the previous WIP PR, which was not in a mergable state.

santouits · 2019-03-21T19:07:55Z

Compiled with both llvm and gnu and didn't find any regression in my game.

About the name, does posix_freedesktop describe the abstractions that were made?

ddevault · 2019-03-21T19:12:12Z

About the name, does posix_freedesktop describe the abstractions that were made?

No. POSIX would be more appropriate for drivers/unix/. POSIX doesn't describe a display server of any kind. Freedesktop doesn't either, Freedesktop.org is just the name of a website which provides tools which are used by XDG, a standards body which mostly decides standards for window managers, and hosts some of the code for Xorg server et al - like a GitHub for desktop developers. The group which maintains the predominant X11 implementation for Linux is X.org, which is separate still from Freedesktop, but still not right - Xorg != X11, a protocol which is governed separately and for which there are many implementations.

I'd sooner have linuxbsd than freedesktop, but both are wrong.

bruvzg · 2019-03-21T21:51:51Z

What part of this is only meant to run on Linux or *BSD? Only some of the code is Linux-specific and is optional. The code will compile and run on any POSIX system with an X11 server.

You can build and run X11 version of Godot on macOS (with exception of webm/webp modules, but these two fail on any "non-standard" configuration) by setting pkg-config search path to XQuartz location, and applying following minimal patch.

Patch

diff --git a/platform/x11/detect.py b/platform/x11/detect.py
index b5ad59e60..1439adddb 100644
--- a/platform/x11/detect.py
+++ b/platform/x11/detect.py
@@ -14,7 +14,7 @@ def get_name():
 
 def can_build():
 
-    if (os.name != "posix" or sys.platform == "darwin"):
+    if (os.name != "posix"):
         return False
 
     # Check the minimal dependencies
diff --git a/platform/x11/platform_config.h b/platform/x11/platform_config.h
index 9e6011ddf..21a2a6b6a 100644
--- a/platform/x11/platform_config.h
+++ b/platform/x11/platform_config.h
@@ -28,9 +28,12 @@
 /* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.                */
 /*************************************************************************/
 
-#ifdef __linux__
+#if defined (__linux__) || defined(__APPLE__)
 #include <alloca.h>
 #endif
+#if defined(__APPLE__)
+#define PTHREAD_RENAME_SELF
+#endif
 #if defined(__FreeBSD__) || defined(__OpenBSD__)
 #include <stdlib.h>
 #define PTHREAD_BSD_SET_NAME

Screenshot

akien-mga · 2019-04-05T13:38:48Z

IMO, "LinuxBSD" (platform/linuxbsd) is the best option. Sorry to be blunt, but we don't really care about other Unix-likes which are even more niche than Linux and BSD (and I say that as a daily Linux user and gamer, and strong advocate of Linux as a gaming platform). Godot is a game engine, so our scope is platforms where people actually develop and play games.

"LinuxBSD" is clear for end users and contributors that this is the platform code for Linux and *BSD. It can of course be made to support other Unixes, and those will have to do with being labelled wrongly, but that's not a big deal IMO. We already have a platform:linuxbsd GitHub label for platform/x11, so IMO it makes the most sense. (It used to be platform:linux, but we appended bsd a while ago.)

I'm fine with using "Freedesktop" if we really want to have an accurate-ish common denominator for all the platforms supported by this port, since the code in this port is mainly limited to Freedesktop (and XDG) related code. The Unix specific code lives in another place (drivers/unix) and is shared by other Unix-derived ports. As such I do agree that "generic_unix" is not a good name for this platform.

AFAICT "Freedesktop" and "LinuxBSD" are the only two names that core developers are somewhat OK with, so I'll just make a mini poll in the next comments. Add a 👍 to the name you prefer.

Note: Whatever name we settle on, keep it mind that it will only be relevant in the port's code and the name of the platform/ port folder:

For the export templates, we will export end user friendly names and likely provide different templates for the different platforms supported from this code (so a "Linux" preset, a "FreeBSD" preset, an "OpenBSD" preset, etc.). Even if the platform code is almost the same apart from a couple platform-specific defines, the export templates binaries can't be, so to support more platforms than just Linux we'll have to provide new binaries and presets.
For the SCons platform option, we can add as many aliases as we want if it feels weird to type scons p=linuxbsd or scons p=freedesktop. Aliases like linux, bsd or fdo are trivial to add.

akien-mga · 2019-04-05T13:39:32Z

If you favor the "LinuxBSD" (platform/linuxbsd) name, 👍 this comment.

akien-mga · 2019-04-05T13:40:08Z

If you favor the "Freedesktop" (platform/freedesktop) name, 👍 this comment.

wmww · 2019-04-08T14:51:26Z

I've consulted @ddevault and we've got a plan. He'll refactor this PR to use LinuxBSD and remove the bit of it that splits up OS and display code. It will then simply be a platform rename. I'll rebase #27747 on top of it, which should improve and clean my PR up.

ddevault · 2019-04-09T15:12:52Z

I've consulted @ddevault and we've got a plan. He'll refactor this PR to use LinuxBSD and remove the bit of it that splits up OS and display code. It will then simply be a platform rename. I'll rebase #27747 on top of it, which should improve and clean my PR up.

Done.

wmww · 2019-04-10T01:40:15Z

Nice work! This now simply improves the platform name with limited risk and no architectural changes. I hope it can move forward so we can start looking at #27747, which builds on top of it.

Edit: By the way, as the commit message indicates scons p=linux and scons p=bsd will now both be aliased to p=linuxbsd

platform/linuxbsd/export/export.cpp

hpvb · 2019-04-10T09:02:36Z

platform/linuxbsd/power_linuxbsd.cpp

@@ -1,5 +1,5 @@
 /*************************************************************************/
-/*  power_x11.cpp                                                        */
+/*  power_linuxbsd.cpp                                                   */


I'm not sure this code even works on BSDs. FreeBSD only has /proc for Linux compatibility iirc?

Weather it works or not, it is used on all X11 platforms (as the old name suggests). Therefor, I think this is an appropriate name. The more accurate name, PowerLinuxAndAlsoBSDAlthoughItsProbablyBrokenThere Just doesn't have the same ring to it.

It:

Works on non-X11 displays, like Wayland

Doesn't break on BSD

Is already somewhat generic and adding BSD support would be easy

So I agree with @wmww that the name here is right.

Calinou · 2019-04-10T11:11:50Z

It should be noted in the changelog that this is a compatibility-breaking change, as the feature tag will no longer be X11 and the OS name will no longer be Linux/X11 (as returned by OS.get_name()).

ddevault · 2019-04-10T14:22:37Z

It should be noted in the changelog that this is a compatibility-breaking change, as the feature tag will no longer be X11 and the OS name will no longer be Linux/X11 (as returned by OS.get_name()).

Changelog updated, though the OS name change was rolled back per @hpvb's suggestion.

Calinou · 2019-04-10T15:59:39Z

@ddevault I didn't mean you should add it to CHANGELOG.md (it would be for 3.2 anyway), it was mostly a note-to-self as I'm the one maintaining the changelog 🙂 Sorry for the confusion.

ddevault · 2019-04-16T15:37:31Z

Rebased against the latest master and addressed merge conflicts.

Also renames OS_X11 to OS_LinuxBSD, and a few other similar changes.

aaronfranke · 2019-07-08T03:02:34Z

@ddevault You will need to rebase this very often, as this code changes often. The milestone has also been changed to 4.0 so it won't be merged until after 3.2. Still, I suggest rebasing often to avoid bigger monolithic rebases later and to continually test this.

ddevault · 2019-07-08T03:04:45Z

I kept up with it for a while but without any feedback or estimate of when it'll be reviewed, it's not worth my time to keep it up-to-date. I'd be happy to rebase it once it starts receiving feedback.

Anutrix · 2019-07-15T10:41:35Z

platform/linuxbsd/export/export.cpp

+	platform->set_debug_32("linuxbsd_32_debug");
+	platform->set_release_64("linuxbsd_64_release");
+	platform->set_debug_64("linuxbsd_64_debug");
+	platform->set_os_name("Unix");


Just confirming. OS name is gonna be "Unix" and not "Linux/BSD"?

No. linuxbsd on account of there already being a unix OS (that includes MacOS)

So line 54 will be changed?

aaronfranke · 2019-10-19T20:42:35Z

SConstruct

+
+# Alias "linux" and "bsd" to "linuxbsd"
+if selected_platform == 'linux' or selected_platform == 'bsd':
+    selected_platform = 'linuxbsd'


Should also alias "x11" to "linuxbsd" so that exiting build scripts continue to work.

akien-mga · 2020-03-26T16:26:02Z

Superseded by #37317, but thanks for the contribution nevertheless!

ddevault requested review from akien-mga, karroffel, neikeq and reduz as code owners March 20, 2019 14:12

bruvzg added feature proposal platform:linuxbsd topic:core topic:porting labels Mar 20, 2019

bruvzg added this to the 3.2 milestone Mar 20, 2019

shartte reviewed Mar 20, 2019

View reviewed changes

modules/mono/config.py Outdated Show resolved Hide resolved

shartte reviewed Mar 20, 2019

View reviewed changes

modules/thekla_unwrap/SCsub Outdated Show resolved Hide resolved

ddevault changed the title ~~Refactor Linux into "generic_unix" and rehome X11 code~~ Refactor x11 into "generic_unix" and rehome X11 code Mar 20, 2019

ddevault changed the title ~~Refactor x11 into "generic_unix" and rehome X11 code~~ Rename x11 platform to 'linuxbsd' Apr 9, 2019

wmww mentioned this pull request Apr 10, 2019

Move display driver functionality from OS to new DisplayDriver singleton #27747

Closed

hpvb requested changes Apr 10, 2019

View reviewed changes

ddevault and others added 4 commits April 23, 2019 10:55

Rename x11 platform to 'linuxbsd'

37bac8c

Also renames OS_X11 to OS_LinuxBSD, and a few other similar changes.

Fix power manager type in OS_Server

ae7d1d4

Alias "linux" and "bsd" to "linuxbsd" in build system

c7d799e

Set LinuxBSD platform name back to Linux/X11 for compatibility

2241a1c

akien-mga modified the milestones: 3.2, 4.0 May 28, 2019

akien-mga requested review from hpvb and removed request for karroffel and neikeq May 28, 2019 10:50

Anutrix reviewed Jul 15, 2019

View reviewed changes

aaronfranke requested changes Oct 19, 2019

View reviewed changes

aaronfranke mentioned this pull request Nov 5, 2019

Add Wayland support #576

Closed

akien-mga closed this Mar 26, 2020

akien-mga added the archived label Mar 26, 2020

Rename x11 platform to 'linuxbsd' #27266

Rename x11 platform to 'linuxbsd' #27266

Conversation

ddevault commented Mar 20, 2019 • edited Loading

ddevault commented Mar 20, 2019

shartte commented Mar 20, 2019 • edited Loading

ddevault commented Mar 20, 2019

ddevault commented Mar 20, 2019

shartte commented Mar 20, 2019 • edited Loading

ddevault commented Mar 20, 2019

shartte commented Mar 20, 2019

ddevault commented Mar 20, 2019 • edited Loading

aaronfranke commented Mar 21, 2019 • edited Loading

ddevault commented Mar 21, 2019 • edited Loading

aaronfranke commented Mar 21, 2019

Megalomaniak commented Mar 21, 2019

toger5 commented Mar 21, 2019

vnen commented Mar 21, 2019

ddevault commented Mar 21, 2019

santouits commented Mar 21, 2019

ddevault commented Mar 21, 2019

bruvzg commented Mar 21, 2019

akien-mga commented Apr 5, 2019

akien-mga commented Apr 5, 2019

akien-mga commented Apr 5, 2019

wmww commented Apr 8, 2019

ddevault commented Apr 9, 2019

wmww commented Apr 10, 2019 • edited Loading

hpvb Apr 10, 2019

Choose a reason for hiding this comment

wmww Apr 10, 2019 • edited Loading

Choose a reason for hiding this comment

ddevault Apr 10, 2019

Choose a reason for hiding this comment

Calinou commented Apr 10, 2019 • edited Loading

ddevault commented Apr 10, 2019

Calinou commented Apr 10, 2019 • edited Loading

ddevault commented Apr 16, 2019

aaronfranke commented Jul 8, 2019

ddevault commented Jul 8, 2019

Anutrix Jul 15, 2019

Choose a reason for hiding this comment

wmww Jul 15, 2019

Choose a reason for hiding this comment

Anutrix Jul 15, 2019

Choose a reason for hiding this comment

ddevault Jul 15, 2019

Choose a reason for hiding this comment

aaronfranke Oct 19, 2019

Choose a reason for hiding this comment

akien-mga commented Mar 26, 2020

ddevault commented Mar 20, 2019 •

edited

Loading

shartte commented Mar 20, 2019 •

edited

Loading

shartte commented Mar 20, 2019 •

edited

Loading

ddevault commented Mar 20, 2019 •

edited

Loading

aaronfranke commented Mar 21, 2019 •

edited

Loading

ddevault commented Mar 21, 2019 •

edited

Loading

wmww commented Apr 10, 2019 •

edited

Loading

wmww Apr 10, 2019 •

edited

Loading

Calinou commented Apr 10, 2019 •

edited

Loading

Calinou commented Apr 10, 2019 •

edited

Loading