Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Linux/BSD platform: Change export name and fix bsd feature tag #76996

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev commented May 12, 2023

1. The bsd feature tag now includes all BSD systems, not just "other BSDs" (than FreeBSD, OpenBSD and NetBSD). See #76990. Extracted to #78272.

Outdated

String OS_LinuxBSD::get_name() const {
#ifdef __linux__
return "Linux";
#elif defined(__FreeBSD__)
return "FreeBSD";
#elif defined(__NetBSD__)
return "NetBSD";
#elif defined(__OpenBSD__)
return "OpenBSD";
#else
return "BSD";
#endif
}

bool OS_LinuxBSD::_check_internal_feature_support(const String &p_feature) {
#ifdef FONTCONFIG_ENABLED
if (p_feature == "system_fonts") {
return font_config_initialized;
}
#endif
if (p_feature == "pc") {
return true;
}
// Match against the specific OS (linux, freebsd, etc).
if (p_feature == get_name().to_lower()) {
return true;
}
return false;
}

2. The display name of the platform has been changed. X11 left as Wayland is not yet supported.

The term "Linux/BSD" is already in use:

String EditorExportPlatformLinuxBSD::get_option_label(int p_index) const {
return (p_index) ? TTR("Stop and uninstall") : TTR("Run on remote Linux/BSD system");
}
String EditorExportPlatformLinuxBSD::get_option_tooltip(int p_index) const {
return (p_index) ? TTR("Stop and uninstall running project from the remote system") : TTR("Run exported project on remote Linux/BSD system");
}

Docs:

doc/classes/EditorExportPlatformPC.xml: 1
  4: 61: 		Base class for the desktop platform exporter (Windows and Linux/BSD).
doc/classes/EditorPaths.xml: 1
  9: 23: 		[b]Note:[/b] On the Linux/BSD platform, Godot complies with the [url=https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html]XDG Base Directory Specification[/url]. Y
doc/classes/OS.xml: 7
137:106: [/i] cache data directory according to the operating system's standards. On the Linux/BSD platform, this path can be overridden by setting the [code]XDG_CACHE_HOME[/code] environment variab
197:114: r configuration directory according to the operating system's standards. On the Linux/BSD platform, this path can be overridden by setting the [code]XDG_CONFIG_HOME[/code] environment varia
212:105: l[/i] user data directory according to the operating system's standards. On the Linux/BSD platform, this path can be overridden by setting the [code]XDG_DATA_HOME[/code] environment variabl
486: 95:  element holds the driver version. For e.g. the [code]nvidia[/code] driver on a Linux/BSD platform, the version is in the format [code]510.85.02[/code]. For Windows, the driver's format is 
487: 65: 				[b]Note:[/b] This method is only supported on the platforms Linux/BSD and Windows when not running in headless mode. It returns an empty array on other platforms.

Perhaps we should replace the parentheses with brackets?

3. Added 2 feature tags to the dropdown list. linuxbsd = linuxbsd, linuxbsd = ∅.

freebsd, openbsd and netbsd are supported but not added to the list due to their rarity.

@dalexeev dalexeev requested a review from a team as a code owner May 12, 2023 10:08
@dalexeev dalexeev force-pushed the linux-bsd-platform-name-and-feature-tags branch 3 times, most recently from d00852f to e91d89b Compare May 12, 2023 12:05
Comment on lines +307 to +311
if (E == "linuxbsd") {
presets.insert("linux");
presets.insert("bsd");
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should keep only linuxbsd in the list (without linux, bsd and other "sub-platforms")? Project Settings UI now allows feature tags that are not in the list.

Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing the bsd feature tag is great, thanks!

platform/linuxbsd/export/export.cpp Outdated Show resolved Hide resolved
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me, but I have not tested this and I don't know all the implications of these changes.

platform->set_name("Linux/X11");
platform->set_os_name("Linux");
platform->set_name("Linux/BSD");
platform->set_os_name("LinuxBSD");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the reasoning behind having both name and os_name?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set_name() sets the name displayed in the list of platforms. set_os_name() sets rather a technical name (I did not find it displayed somewhere in the interface). Also, a feature tag is formed from os_name (converted to lower case).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The os_name was changed in #61352. So it looks like we wanted to go the opposite direction and split Linux and BSD? cc @Faless

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, Linux and BSD are not compatible systems.
Binaries for Linux do not work on BSD, and vice versa, they have different OS APIs, they are not the same platform even if they share a good amount of source code.
And we must be able to detect which platform we are in if we want gdextensions to work (and not end up trying to load a linux shared library on BSD).
As I mentioned in the other PR, we might be able to simply add another exporter dedicated to BSD.

@akien-mga
Copy link
Member

  1. The bsd feature tag now includes all BSD systems, not just "other BSDs" (than FreeBSD, OpenBSD and NetBSD). See OS.has_feature("linux") returns false on linux #76990.

That part is good so I would suggest extracting it to a separate PR so we can merge it for 4.1.

The rest I'm not convinced yet. We only provide Linux export templates so calling the export preset Linux/BSD is IMO wrong. And I haven't reviewed all the implications of the other changes but I'm not sure they go in the right direction.

The various BSD platforms we support are only working if you compile export templates for each of them manually, and use those as custom export templates. Currently you have to reuse the Linux/X11 export preset for that, which is sub par, but I'm not sure the approach taken here makes it better.

@dalexeev
Copy link
Member Author

That part is good so I would suggest extracting it to a separate PR so we can merge it for 4.1.

Done: #78272.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants