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

Import env if possible #1170

Merged
merged 1 commit into from
Jul 14, 2023
Merged

Conversation

adamscott
Copy link
Member

This PR make it possible to import env and use it instead of creating one from scratch every time.

Handy because we encourage users to use the godot-cpp SConstruct file as a base to their projects (see the test project). So, if a project want to override specific settings, (eg. make a path local to their SConstruct file, not local to the godot-cpp/SConstruct file), it can do so.

@adamscott adamscott requested a review from a team as a code owner July 9, 2023 13:31
@adamscott adamscott added the topic:buildsystem Related to the buildsystem or CI setup label Jul 9, 2023
@adamscott adamscott added this to the 4.x milestone Jul 9, 2023
@adamscott
Copy link
Member Author

For unknown reasons, if I run ./misc/scripts/file_format.sh, it changes a lot more than my changes.

@adamscott adamscott added the enhancement This is an enhancement on the current functionality label Jul 9, 2023
@Faless
Copy link
Contributor

Faless commented Jul 10, 2023

For unknown reasons, if I run ./misc/scripts/file_format.sh, it changes a lot more than my changes.

Make sure you have the same version of black used by CI (currently 22.3.0):

pip3 install black==22.3.0

@adamscott adamscott force-pushed the import-env-if-exists branch 3 times, most recently from d23c1e4 to 1e3e304 Compare July 10, 2023 18:03
@adamscott
Copy link
Member Author

@Faless It worked! Thanks!

Copy link
Contributor

@Faless Faless left a comment

Choose a reason for hiding this comment

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

Looking good, just some nit picks to make the behavior clearer (I think?):

diff --git a/SConstruct b/SConstruct
index 0edf7fa..11ec4cf 100644
--- a/SConstruct
+++ b/SConstruct
@@ -4,19 +4,11 @@ import os
 import platform
 import sys
 import subprocess
-from typing import TYPE_CHECKING
 from binding_generator import scons_generate_bindings, scons_emit_files
-from SCons.Script import Environment
 from SCons.Errors import UserError
-from SCons.Variables import BoolVariable, EnumVariable, PathVariable
 
 EnsureSConsVersion(4, 0)
 
-try:
-    Import("env")
-except:
-    pass
-
 
 def add_sources(sources, dir, extension):
     for f in os.listdir(dir):
@@ -59,10 +51,13 @@ elif ARGUMENTS.get("platform", ""):
 else:
     raise ValueError("Could not detect platform automatically, please specify with platform=<platform>")
 
-# Default tools with no platform defaults to gnu toolchain.
-# We apply platform specific toolchains via our custom tools.
-if not "env" in globals() or TYPE_CHECKING:
+try:
+    Import("env")
+except:
+    # Default tools with no platform defaults to gnu toolchain.
+    # We apply platform specific toolchains via our custom tools.
     env = Environment(tools=["default"], PLATFORM="")
+
 env.PrependENVPath("PATH", os.getenv("PATH"))
 
 # Default num_jobs to local cpu count if not user specified.

This PR make it possible to import `env` and use it instead of creating
one from scratch every time.

Handy because we encourage users to use the godot-cpp SConstruct file as
a base to their projects (see the test project). So, if a project want
to override specific settings, (eg. make a path local to their SConstruct
file, not local to the godot-cpp/SConstruct file), it can do so.
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.

This looks great to me! And Fabio's requested changes have been made

@dsnopek
Copy link
Collaborator

dsnopek commented Jul 14, 2023

Discussed at the GDExtension meeting, and we think this makes sense.

@dsnopek dsnopek merged commit 67bd2ea into godotengine:master Jul 14, 2023
@dsnopek
Copy link
Collaborator

dsnopek commented Jul 14, 2023

Thanks!

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.

4 participants