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

Remove .dev suffix from builds. #1705

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

Conversation

Ivorforce
Copy link
Contributor

@Ivorforce Ivorforce commented Feb 11, 2025

I know this is going to be controversial, but it's a simple PR, so I want to just put it out there. I'm not 100% we should merge this, myself.

I have pre-written some thoughts against this change below, and my thoughts for them.
They are a bit ramble-y, so feel free to partake in the discussion freely without acknowledging them 😅

Reasoning

Currently, godot-cpp automatically adds the .dev suffix to builds that are created with the dev_build=yes flag.
This can result in binaries named like so:

libgdexample.linux.template_release.dev.x86_32.so  ; For dev_build=yes
libgdexample.linux.template_release.x86_32.so  ; For dev_build=no (default)

The .gdextension file needs to choose one of these formats to load from:

linux.release.x86_32 = "res://bin/libgdexample.linux.template_release.x86_32.so"
; Or:
linux.release.x86_32 = "res://bin/libgdexample.linux.template_release.dev.x86_32.so"

It cannot choose 'either': One of the two must be chosen.

In a normal dev cycle, one may default to building with dev_build=yes to get faster builds for testing. Therefore, one may choose the .dev entry for the demo project.
However, when it comes to testing the project's release-ready binary, one must either test it in a different project, or edit the .gdextension file to load the non- .dev binary temporarily, or rename the binary to .dev by hand.

This is not only tedious, but actually confusing: I've seen a few people in Discord help channels run into problems because they built the version that wasn't loaded by the editor, and wondered why the GDExtension wouldn't respond to code changes. I have also run into this problem, and proceeded to [strip .dev from my own dev versions in SConstruct.

In defense of .dev

.dev is used for .dev binaries by Godot itself. godot-cpp tries to emulate godot upstream behavior as much as possible, to reduce complexity across the projects. However, Godot does not have the problems described above, as the binary is not added to a .gdextension file. I think it makes sense for Godot, but less for godot-cpp.

.dev denotes a compatibility change, like the other suffixes. .dev includes debug symbols, and can therefore do things regular builds cannot. However, this does not concern Godot itself. For godot, it does not matter whether the binary is built with debug symbols or not. Therefore, I do not think this argument holds.

.dev protects against confusing it for an optimized build. dev builds are unoptimized, so uploading one would be a mistake. .dev suffixes protect against accidentally uploading the wrong version. I don't have any contra for this argument, it should be weighed against benefits. We could add other protections too, such as a warning when dev libraries are loaded into Godot.

.dev can still be optimized, using explicit build args. If one builds with scons dev_build=yes lto=auto optimize=speed, the resulting binary will be optimized very similar to an actual build. This may be good enough for testing performance. This is a valid workflow, and if we reject this PR, it should be documented.

Compiling dev builds with .dev saves time when switching between the two. This is correct, and arguably, we should keep .dev suffixes for object files to preserve this. However, preserving the final binary saves at most the LTO step, I think.

Alternative Solution

Instead of stripping the .dev suffix, it would be possible to auto-generate a .gdextension file with a SCons tool. The entry would always load the latest build in Godot, no matter the configuration. This would allow the binaries to keep their current name, and the above confusion to still be resolved.
However, this would require making the tool, and .gdextension files would not be written by hand anymore. Instead, the tool would need to cover its whole range of features.

It is also possible for users that want to adopt this workflow to just strip .dev suffixes from the binary like I did. This could be documented to give users freedom of choice.

@Ivorforce Ivorforce requested a review from a team as a code owner February 11, 2025 21:31
@Calinou Calinou added enhancement This is an enhancement on the current functionality discussion topic:buildsystem Related to the buildsystem or CI setup labels Feb 11, 2025
@Calinou Calinou added this to the 4.x milestone Feb 11, 2025
@Naros
Copy link
Contributor

Naros commented Feb 12, 2025

It is also possible for users that want to adopt this workflow to just strip .dev suffixes from the binary like I did. This could be documented to give users freedom of choice.

Would it make sense to conditionalize this behavior for those who prefer to keep the designation?

@Ivorforce
Copy link
Contributor Author

Ivorforce commented Feb 12, 2025

Would it make sense to conditionalize this behavior for those who prefer to keep the designation?

You mean of my original proposal, or the part you quoted?
Either way the final choice is with the godot-cpp user, because they can strip or add .dev at will in their SConstruct. This discussion is more about which is the saner default.

@dsnopek
Copy link
Collaborator

dsnopek commented Feb 12, 2025

I'm not opposed to removing the .dev suffix.

This is something that folks have complained about for a long time, and it doesn't really provide a ton of value - I think it makes much more sense for an executable (like Godot) than a library.

@enetheru
Copy link
Contributor

I have the feeling that this is just covering for inadequacies in the error reporting coming out of godot itself. If the loading code was made to provide clearer errors, and useful information a lot of problems people face would be explained as they attempt to load their project.
We can also improve the examples with additional debug statements in the initialisation so that mistakes are picked up sooner.

I just see the same errors over and over again with loading and a lot of it can be explained by the lack of error reporting and error information coming out of godot.

@Ivorforce
Copy link
Contributor Author

I have the feeling that this is just covering for inadequacies in the error reporting coming out of godot itself. If the loading code was made to provide clearer errors, and useful information a lot of problems people face would be explained as they attempt to load their project. We can also improve the examples with additional debug statements in the initialisation so that mistakes are picked up sooner.

I just see the same errors over and over again with loading and a lot of it can be explained by the lack of error reporting and error information coming out of godot.

Better error handling would definitely help, but not all problems removing .dev solves are technically errors, I think.

@akien-mga
Copy link
Member

akien-mga commented Feb 14, 2025

Just noting that while it's currently not defined, a dev feature tag could be added upstream in OS::has_feature for DEV_ENABLED, so you could do:

linux.release.x86_32 = "res://bin/libgdexample.linux.template_release.x86_32.so"
linux.release.dev.x86_32 = "res://bin/libgdexample.linux.template_release.dev.x86_32.so"

and it would pick the dev one when running with a dev build of a Godot release template, and the non-dev one otherwise.

This wouldn't cover the possibility to use a dev build of a library with a non-dev build of Godot though (e.g. official editor binaries).

.dev denotes a compatibility change, like the other suffixes. .dev includes debug symbols, and can therefore do things regular builds cannot. However, this does not concern Godot itself. For godot, it does not matter whether the binary is built with debug symbols or not. Therefore, I do not think this argument holds.

It's not just enabling debug symbols though, it's mainly enabling DEV_ENABLED which can be used in your project or in the library itself to perform some operations only relevant at development time. E.g. DEV_ASSERT to crash in a condition you don't want to happen while developing, but you wouldn't want the library to crash on the end user because you made a mistake as a dev.

Aside from those notes, I don't have much stake in this so I don't mind if the .dev suffix is removed in godot-cpp. But I do foresee a risk of people shipping their extensions with dev builds by mistake (with probably 10x the size and a third of the performance), because there will be no indication in the library name of how it was built.

@dsnopek
Copy link
Collaborator

dsnopek commented Feb 14, 2025

Just noting that while it's currently not defined, a dev feature tag could be added upstream in OS::has_feature for DEV_ENABLED, so you could do:

There was a proposal about this: godotengine/godot-proposals#11409

I'm not sure that would be too useful, though, because it would mean the dev extension would only be loaded by a Godot build that was also built with dev_build=yes, and I don't think that's commonly what folks want. Mostly, folks want to use the dev extension with a normal editor build of Godot.

But I do foresee a risk of people shipping their extensions with dev builds by mistake (with probably 10x the size and a third of the performance), because there will be no indication in the library name of how it was built.

That is a risk!

I guess it'd be a question of which comes up more often: folks complaining about having to edit their .gdextension when they are doing development, or folks accidentally shipping dev builds of their extensions. But there's no way to know without dropping the dev suffix and seeing what happens :-)

@Ivorforce
Copy link
Contributor Author

It would also possible to throw out a warning if you're loading Godot with a dev build of a library :)
Probably wouldn't be too intrusive for testing, but it would show users immediately if a dev build had been uploaded by mistake, making it an easy mistake to fix.

@dsnopek
Copy link
Collaborator

dsnopek commented Feb 14, 2025

It would also possible to throw out a warning if you're loading Godot with a dev build of a library :)

This is a great idea!

It'd basically just be doing this early-ish in loading:

#ifdef DEV_ENABLED
    WARN_PRINT("...");
#endif

@Naros
Copy link
Contributor

Naros commented Feb 18, 2025

I guess it'd be a question of which comes up more often: folks complaining about having to edit their .gdextension when they are doing development, or folks accidentally shipping dev builds of their extensions. But there's no way to know without dropping the dev suffix and seeing what happens :-)

This where having a proper release pipeline with CI/CD avoids these issues and provides reproducible builds that eliminates any sort of human error during that process. This where having a solid GitHub project template with workflow actions to do these things pays off. I realize that not everyone may use such things, but that's the risk you assume by not using it.

The release pipelines I use for my day work at Red Hat and my side hussle with Orchestrator is all done automatically with scripts and GitHub actions, etc. I really don't see why anyone wouldn't do that.

@enetheru
Copy link
Contributor

This is the line for cmake that needs to be deleted. '/cmake/godotcpp.cmake#L299'

@dsnopek
Copy link
Collaborator

dsnopek commented Feb 18, 2025

Discussed at the GDExtension meeting, and everyone present was in favor of this change, especially with the warning that @Ivorforce suggested.

@Ivorforce
Copy link
Contributor Author

I've updated the PR to modify CMake builds too, and to print a warning on load.

The print works correctly and shows up in the editor. Unfortunately, it doesn't appear that the library name is currently exposed in any way to GDExtension code, so the warn message is missing the culprit 😅 If anybody has any ideas about that, let me know.

@Ivorforce
Copy link
Contributor Author

Ivorforce commented Feb 18, 2025

I brought back .dev to object suffixes. Now, only the binary itself loses the .dev suffix. This preserves fast build times when switching between dev_build=yes and dev_build=no frequently. I wonder if everyone else thinks this is a good idea? Let me know.

@enetheru Any idea how to duplicate this behavior for CMake?

@dsnopek
Copy link
Collaborator

dsnopek commented Feb 18, 2025

I brought back .dev to object suffixes. Now, only the binary itself loses the .dev suffix. This preserves fast build times when switching between dev_build=yes and dev_build=no frequently. I wonder if everyone else thinks this is a good idea?

Personally, I'd be fine either way on this one. I don't tend to quickly switch back and forth between dev and non-dev - I tend to spend longer periods of time in one or the other, so I don't think longer build times when switching would bother me. I don't know what folks do most commonly, though.

@enetheru
Copy link
Contributor

I brought back .dev to object suffixes. Now, only the binary itself loses the .dev suffix. This preserves fast build times when switching between dev_build=yes and dev_build=no frequently. I wonder if everyone else thinks this is a good idea? Let me know.

@enetheru Any idea how to duplicate this behavior for CMake?

There's no need. cmake uses build-folders for all generation and compile products, so it would just amount to using a separate folder.

@Naros
Copy link
Contributor

Naros commented Feb 19, 2025

There's no need. cmake uses build-folders for all generation and compile products, so it would just amount to using a separate folder.

And when you pair that with ccache, you get the fast build times like what you're used to seeing with scons.

Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks! This works in my testing, and the code looks good to me. I'll leave this open for a bit longer before merging, though, just in case anyone else has an opinion they haven't had a chance to share yet.

There is a lambda used here, but I'm personally fine with it: I think the code is somewhat clearer by having the whole thing together (rather than broken up into another proper function), and this is a dev-only warning. But if anyone else is opposed, it would be trivial to make lambda-less

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants