-
-
Notifications
You must be signed in to change notification settings - Fork 463
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
base: master
Are you sure you want to change the base?
Conversation
tools/godotcpp.py
Outdated
@@ -198,6 +198,8 @@ def options(opts, env): | |||
) | |||
) | |||
|
|||
opts.Add("profile", "Allow specification of customization file other than only custom.py", "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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="" | |
) | |
) |
tools/godotcpp.py
Outdated
PathVariable( | ||
key="profile", | ||
help="Allow specification of customization file other than `custom.py`.", | ||
default="" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default="" | |
default=None, | |
validator=validate_file, |
My bad didn't notice the other case, might also be best to do:
default="" | |
default=env.get("profile", None), | |
validator=validate_file, |
SConstruct
Outdated
customs.append(profile + ".py") | ||
if not profile.endswith('.py'): | ||
profile += '.py' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trailing whitespace
try: | ||
customs += Import("customs") | ||
except: | ||
pass |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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? |
This PR aims to address #1334 at least partially. With the changes:
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 thesome_file.py
with the extension then the argument must be changed toprofile=../some_file.py
(in other words, the path would have to be relative to godot-cpp SConstruct)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 arelease.py
and the argument is given asprofile=release
, then arelease
file will be created. I was unable to track down what/where this thing happens.