Add SCons variant_dir support

[Ivorforce]

Supersedes #1439 - it's basically just a rebase. It compiles, but I haven't checked if it does what it says it does (allow godot-cpp to be included from elsewhere).

[Ivorforce]

Commenting "my own" PR because so far I've only rebased. Would like to see if anybody has comments on my comments, otherwise I'll probably just adjust things as I see fet.

[Ivorforce]

Alright, code wise, I think this is ready now.
I still haven't tested it though. Someone should probably do that before a merge (i may, but it may take a few more days).

[Ivorforce]

Alright, I tested this, it seems to work.

To use it, one has to call SConscript like env = SConscript("../SConstruct", variant_dir="build_folder"). I tested this from the test project.

[dsnopek]

To use it, one has to call SConscript like env = SConscript("../SConstruct", variant_dir="build_folder"). I tested this from the test project.

I tested this in the same way and it worked for me!

However, shouldn't this be an option given on the command-line, rather than something that developers hard-code into their project? Why not add this to other default options?

Also, this only seemed to affect the build artifacts from godot-cpp itself, and not the test project. What do developers need to do so that their artifacts also build in a separate directory?

[Ivorforce]

However, shouldn't this be an option given on the command-line, rather than something that developers hard-code into their project? Why not add this to other default options?
Also, this only seemed to affect the build artifacts from godot-cpp itself, and not the test project. What do developers need to do so that their artifacts also build in a separate directory?

This would be possible! I tested this as well, and arrived at this solution:

• Add a variant_dir command line option
• Call the VariantDir function with the appropriate value, if passed
• Compile <variant_dir>/* instead of src/* in godotcpp.py (or analogously, the user SConstruct)

As you observed, applying variant_dir godot-cpp side would only move godot-cpp object files, so the option would need to be added on the user side (godot-cpp-template?).

[Faless]

We might want to add something like:

diff --git a/test/SConstruct b/test/SConstruct
index b949bca..20f62b1 100644
--- a/test/SConstruct
+++ b/test/SConstruct
@@ -1,6 +1,6 @@
 #!/usr/bin/env python
 
-env = SConscript("../SConstruct")
+env = SConscript("../SConstruct", variant_dir=ARGUMENTS.get("variant_dir", None))
 
 # For the reference:
 # - CCFLAGS are compilation flags shared between C and C++

to the test SConstruct, or the cpp-template so people can "guess" how to use this feature.

But it isn't really important for this PR right now, so let's not block it for this.

[Faless]

Looking great! 🎆 🏆

[Repiteo]

As mentioned in RocketChat: variant_dir is only unfeasible on the main repo because of how our code generators are setup. It should be totally fine on godot-cpp!

While this should be given more thorough testing at a later point, this should serve as an entirely functional baseline. Great job getting the ball rolling!

[dsnopek]

Thanks!

[dsnopek]

After this change, custom_api_file isn't working for me. This PR added converter=normalize_path for this option, but it still seems to be looking for the file relative to the "godot-cpp" directory, rather than relative to the directory where scons was run which is at a higher-level for my extension

Also, there's a bunch of options that take paths, that don't have the converter=normalize_path, for example: build_profile, compiledb_file, custom_tools. Shouldn't these also have the converter (assuming it actually worked)?

[dsnopek]

I think this is a SCons version thing. I normally use SCons 4.0.1, because that's what comes with my distro (Ubuntu 22.04). I tried updating to SCons 4.9.1 via pip, and then this seemed to work fine.

We still have EnsureSConsVersion(4, 0) in our SConstruct. We should either rework this to work for SCons 4.0+, or update our version requirement

[Ivorforce]

After this change, custom_api_file isn't working for me. This PR added converter=normalize_path for this option, but it still seems to be looking for the file relative to the "godot-cpp" directory, rather than relative to the directory where scons was run which is at a higher-level for my extension

Hrm, I suppose that means normalize_path has to be fixed? I'm a bit surprised by that since the function hasn't been touched. Perhaps just a SCons bug?

We still have EnsureSConsVersion(4, 0) in our SConstruct. We should either rework this to work for SCons 4.0+, or update our version requirement

If SCons comes with 22.04 Ubuntu, it would be unfortunate to increase the minimum requirement beyond that. I'd rather work around the issue for the moment if it's a SCons bug.

Also, there's a bunch of options that take paths, that don't have the converter=normalize_path, for example: build_profile, compiledb_file, custom_tools. Shouldn't these also have the converter (assuming it actually worked)?

Personally, I think so. Assuming we get it to work as intended anyway.

[dsnopek]

@Ivorforce I just posted #1819 which fixes this for me

Hrm, I suppose that means normalize_path has to be fixed? I'm a bit surprised by that since the function hasn't been touched. Perhaps just a SCons bug?

In some further testing, it looks like normalize_path() works as expected, but it's the converter=... bit that doesn't do its thing correctly. Seems like a SCons bug that was later fixed

Also, there's a bunch of options that take paths, that don't have the converter=normalize_path, for example: build_profile, compiledb_file, custom_tools. Shouldn't these also have the converter (assuming it actually worked)?

Personally, I think so. Assuming we get it to work as intended anyway.

Well, it seems we can't rely on converter=... if we're going to support SCons 4.0.1, so we'll have to leave those as they are (which call normalize_path() manually)