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

Add SCons variant_dir support #1439

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

Conversation

Cedev
Copy link

@Cedev Cedev commented Apr 14, 2024

Adds support for using SCons's variant_dir feature to specify the build path for where to build a variant of the the library.

Correct paths in SConstruct and godotcpp tool for the following, even when the current directory is not the root of the repository:

  • the binding_generator python module
  • godotcpp tools
  • source code and includes
  • generated source code

@Cedev Cedev requested a review from a team as a code owner April 14, 2024 21:30
@AThousandShips AThousandShips added enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup labels Apr 16, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Apr 16, 2024
@dsnopek
Copy link
Collaborator

dsnopek commented Apr 24, 2024

Thanks!

Can you show how to use the functionality added in this PR?

@Cedev
Copy link
Author

Cedev commented Apr 29, 2024

Can you show how to use the functionality added in this PR?

If you pass variant_dir to SConscript it will build files in that directory instead of in the source tree. You can use this to separate out builds for different variants like platform and architecture/target/debug/precision

For example, the following SConstruct file for something conscripting godot-cpp will build it twice, once for env["precision"]="single" and once for env["precision"]="double".

env = Environment()
env["precision"] = "single"
env = SConscript(
  "godot-cpp/SConstruct",
  exports=['env'],
  variant_dir="#build/libgodot-cpp-single",
  duplicate=False)

env = Environment()
env["precision"] = "double"
env = SConscript(
  "godot-cpp/SConstruct",
  exports=['env'],
  variant_dir="#build/libgodot-cpp-double",
  duplicate=False)

Because of the variant_dir options the builds will be done in two separate directories, "#build/libgodot-cpp-single" and "#build/libgodot-cpp-double"

The generated binding header files depend on both the env["precision"] and env["arch"], changing either would result in generating different include files and rebuilding everything that depends on them with each build, but with the results of the single and double precision variant builds in different directories, subsequent builds can reuse the portion of each variant that didn't change (which for a project depending on godot-cpp is usually everything).

Here's the output from the second run of that build on my machine.

Cedric@ELIDED-COMPUTER-NAME MINGW64 /e/elided-project-path (master)
$ scons
scons: Reading SConscript files ...
Auto-detected 4 CPU cores available for build parallelism. Using 4 cores by default. You can override it with the -j argument.
Building for architecture x86_64 on platform windows
Auto-detected 4 CPU cores available for build parallelism. Using 4 cores by default. You can override it with the -j argument.
Building for architecture x86_64 on platform windows

scons: warning: Two different environments were specified for target E:\elided-project-path\compile_commands.json,
        but they appear to have the same action: write_compilation_db(target, source, env)
File "E:\elided-project-path\godot-cpp\tools\godotcpp.py", line 444, in generate
scons: done reading SConscript files.
scons: Building targets ...
scons: `build\libgodot-cpp-single\bin\libgodot-cpp.windows.template_debug.x86_64.lib' is up to date.
scons: `build\libgodot-cpp-double\bin\libgodot-cpp.windows.template_debug.double.x86_64.lib' is up to date.
scons: done building targets.

@Cedev Cedev force-pushed the scons-variant_dir-support branch from e60fdc1 to d6edb72 Compare April 29, 2024 15:50
@dsnopek
Copy link
Collaborator

dsnopek commented Apr 29, 2024

Ah, ok, thanks! That sounds really cool :-)

@Faless Do you have any concerns with these changes?

@dsnopek dsnopek requested a review from Faless April 29, 2024 16:22
@Faless
Copy link
Contributor

Faless commented Apr 29, 2024

Ah, ok, thanks! That sounds really cool :-)

@Faless Do you have any concerns with these changes?

Indeed this seems pretty nice, I've been investigating this on the godot side a couple of times, it's nice to have it at least in godot-cpp.

The only note I have is about the warning with the provided example:

scons: warning: Two different environments were specified for target E:\elided-project-path\compile_commands.json,
but they appear to have the same action: write_compilation_db(target, source, env)

I wonder if the default compile_command.json should be generated in the variant dir when not specified. Not a blocker in any case, since the problem is pretty specific to the synthetic example.

Not sure why CI tests are failing...

@Cedev
Copy link
Author

Cedev commented Apr 29, 2024

I wonder if the default compile_command.json should be generated in the variant dir when not specified. Not a blocker in any case, since the problem is pretty specific to the synthetic example.

I couldn't decide whether to touch that, well eventually I decided not to. Usually you only invoke SConscript once in an scons script, env["compiledb_file"] already provides a way to control that, and by default env["compiledb"] = False and the file isn't generated.

To keep from potentially breaking things I only changed where godotcpp and SConstruct are looking in their own source directories for their own files, and not when the location of a file is provided by a user. (I think that's the scope of changes, whether or not that's true would be relevant review).

@Cedev
Copy link
Author

Cedev commented Apr 29, 2024

...the CI tests are failing with the same error on this other recent PR too: https://github.com/godotengine/godot-cpp/actions/runs/8852986336/job/24312834460?pr=1450

For the tiniest of bifurcation, the checks were successful against this PR when the base was b7661a60a4788cc7d5ce690cf8a5815b0aab511, but it looks like there's way more recent PRs than that that they were passing against.

@dsnopek
Copy link
Collaborator

dsnopek commented May 16, 2024

...the CI tests are failing with the same error on this other recent PR too: https://github.com/godotengine/godot-cpp/actions/runs/8852986336/job/24312834460?pr=1450

Try rebasing on master. The changes from PR #1461 may have fixed the CI issues (but we'll only know after enough CI builds run successfully)

Adds support for using SCons's variant_dir feature to specify the build
path for where to build a variant of the the library.

Correct paths in SConstruct and godotcpp tool for the following, even
when the current directory is not the root of the repository:
 - the binding_generator python module
 - godotcpp tools
 - source code and includes
 - generated source code
@Cedev Cedev force-pushed the scons-variant_dir-support branch from d6edb72 to 5b47e4d Compare May 17, 2024 05:02
@dsnopek
Copy link
Collaborator

dsnopek commented Jun 26, 2024

Thanks! Unfortunately, it looks like this needs another rebase to address conflicts in SConstruct

@Ewall198
Copy link

Ewall198 commented Dec 9, 2024

@dsnopek or @Cedev Looks like this hasn't been touched in a while, but it'd be a nice changed to have. Any suggestions on a workaround in the meantime?

@dsnopek
Copy link
Collaborator

dsnopek commented Dec 10, 2024

@Ewall198 Looking back on the previous comments, it seems like the PR was basically acceptable, it just needs a rebase to (hopefully) fix CI issues and resolve conflicts that have come up in the meantime. If @Cedev is no longer interested in pushing this forward, someone else could take it over in a new PR?

@Ivorforce
Copy link
Contributor

Sure, I'll make a rebase PR. It will hel address a personal concern of mine anyway (#1646).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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