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

Make build system more friendly when it's no longer top-level #1358

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

Conversation

Kehom
Copy link
Contributor

@Kehom Kehom commented Jan 11, 2024

This PR aims to address #1334 at least partially. With the changes:

  • When the profile=some_file.py is given as argument, that file can be placed alongside the extension SConstruct directory rather than within the godot-cpp. Without the changed script, in order to keep the some_file.py with the extension then the argument must be changed to profile=../some_file.py (in other words, the path would have to be relative to godot-cpp SConstruct)
  • Some lines that don't do anything were removed.
  • Warning message of unknown SCons arguments are given only when godot-cpp SConstruct is running as top level script. If an extension author wishes to output that kind of warning it must be done within extension SConstruct, which becomes top-level.

A side note here. I kept the original profile=... logic of accepting file name without .py extension. However I have noticed that when this is the case, SCons generate a binary file with the exact same name of the profile file, but without any extension. As an example, if there is a release.py and the argument is given as profile=release, then a release file will be created. I was unable to track down what/where this thing happens.

@Kehom Kehom requested a review from a team as a code owner January 11, 2024 19:44
@@ -198,6 +198,8 @@ def options(opts, env):
)
)

opts.Add("profile", "Allow specification of customization file other than only custom.py", "")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
opts.Add("profile", "Allow specification of customization file other than only custom.py", "")
opts.Add(
PathVariable(
key="profile",
help="Allow specification of customization file other than `custom.py`.",
default=""
)
)

@AThousandShips AThousandShips added enhancement This is an enhancement on the current functionality topic:buildsystem Related to the buildsystem or CI setup labels Jan 12, 2024
PathVariable(
key="profile",
help="Allow specification of customization file other than `custom.py`.",
default=""
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
default=""
default=None,
validator=validate_file,

My bad didn't notice the other case, might also be best to do:

Suggested change
default=""
default=env.get("profile", None),
validator=validate_file,

SConstruct Outdated
customs.append(profile + ".py")
if not profile.endswith('.py'):
profile += '.py'

Copy link
Member

Choose a reason for hiding this comment

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

Trailing whitespace

Comment on lines -24 to -27
try:
customs += Import("customs")
except:
pass
Copy link
Contributor

Choose a reason for hiding this comment

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

This functionality seems to be completely removed - I don't see it re-added later.

See #1196 for some background on this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I wasn't sure how to implement that functionality. But I will wait until we get some feedback (as you have requested) before performing the change + rebase.

@dsnopek
Copy link
Contributor

dsnopek commented Apr 9, 2024

I like the idea behind this functionality! We don't want to show the "unknown variables" message when used in another project that defines it's own variables. However, I think we'll need someone who is more in tune with the build system to say if this is done in the right way.

@Faless @adamscott What do you think?

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.

None yet

3 participants