Skip to content

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
)
)

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

@AThousandShips AThousandShips Jan 12, 2024

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
Collaborator

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
Collaborator

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?

@AndrejLapin
Copy link

I like the idea of having a standardized way to show "unknown variables" message. What I would suggest is creating a hook for modifying godot-cpp scons options.

It could be implemented in a similar way to how the "custom_tools" is implemented. A PathVariable for example with a name "custom_options" is added here https://github.com/godotengine/godot-cpp/blob/master/tools/godotcpp.py#L222
After the call to opts.Update(env) , a path is taken from "custom_options", file at that path is executed with opts and env passed as locals. This is where new options would be added.

This way the user will have a way to add their own custom options and will not loose the ability to see "unknown variables" message when inputting something unexpected.

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