-
-
Notifications
You must be signed in to change notification settings - Fork 613
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
base: master
Are you sure you want to change the base?
Conversation
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? |
I'm not opposed to removing the 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. |
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. 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 |
Just noting that while it's currently not defined, a
and it would pick the This wouldn't cover the possibility to use a
It's not just enabling debug symbols though, it's mainly enabling Aside from those notes, I don't have much stake in this so I don't mind if the |
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
That is a risk! I guess it'd be a question of which comes up more often: folks complaining about having to edit their |
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 |
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. |
This is the line for cmake that needs to be deleted. '/cmake/godotcpp.cmake#L299' |
Discussed at the GDExtension meeting, and everyone present was in favor of this change, especially with the warning that @Ivorforce suggested. |
edeb8ee
to
681e3fd
Compare
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. |
681e3fd
to
69542a1
Compare
I brought back @enetheru Any idea how to duplicate this behavior for CMake? |
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. |
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. |
…s loaded from a dev build.
69542a1
to
4aaa2f5
Compare
There was a problem hiding this 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
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 thedev_build=yes
flag.This can result in binaries named like so:
The
.gdextension
file needs to choose one of these formats to load from: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 thedemo
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 inSConstruct
.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 whendev
libraries are loaded intoGodot
..dev
can still be optimized, using explicit build args. If one builds withscons 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 aSCons
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.