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

SCons warning message with custom variables #1334

Open
Kehom opened this issue Dec 13, 2023 · 5 comments
Open

SCons warning message with custom variables #1334

Kehom opened this issue Dec 13, 2023 · 5 comments
Labels
discussion topic:buildsystem Related to the buildsystem or CI setup

Comments

@Kehom
Copy link
Contributor

Kehom commented Dec 13, 2023

Godot version

4.2

godot-cpp version

4.2

System information

Windows 10 and Linux Mint

Issue description

SCons provides means for us to add custom variables and easily parse them within the build script. I started to explore this to give users options to disable certain parts of addon. While the build did work, I got the WARNING: Unknown SCons variables were passed and will be ignored message. Because I'm learning SCons this made me extremely confused, specially because I couldn't find any help regarding this error.

Upon digging up I did find this in the godot-cpp SConstruct script:

unknown = opts.UnknownVariables()
if unknown:
    print("WARNING: Unknown SCons variables were passed and will be ignored:")
    for item in unknown.items():
        print("    " + item[0] + "=" + item[1])

Ahhh, that's where the message is coming from! Since I still can deal with the extra variables and generate the expected binary I'm aware that I can safely ignore that warning message. However I think anyone attempting to perform a custom build will be greeted by the message and most likely be very confused by that.

I did a (rather small) local change to this script that allows me to create custom variables. So in my extension's SConstruct I can either

extraopts = [ ... custom options here ...]
SConscript("godot-cpp/SConstruct", exports='extraopts')

or

Export("extraopts")
SConscript("godot-cpp/SConstruct")

Then desirable custom variables won't generate the warning message. Is this a good solution or there something better? Also, if this is good enough I can PR the change.

Steps to reproduce

Somewhat in the description of the "bug"

Minimal reproduction project

N/A

@Kehom
Copy link
Contributor Author

Kehom commented Dec 15, 2023

I have been testing a bunch with customizing the Extension build and have noticed several usability quirks as well as some "bugs"?.

  1. In the godot-cpp SConstruct it does handle the profile argument allowing us to specify the path to a custom set of parameters within a file. However said argument is not added into the opts variable, which leads to the warning message I mentioned in the original post in case profile=... is indeed used. This is a trivial "one-liner" fix.

  2. If we use relative path, it will be based on the "current directory", which will most likely be the Extension's one. Now consider that we want to deal with custom variables within the extension itself, leading us to add such customization file into the extension's directory (normally one bellow godot-cpp - at least based on the main example given). In order for the godot-cpp SConstruct to correctly deal with its own expected variables we have to provide profile=../some_file.py when calling SCons from the extension's directory. This is rather counter intuitive!

Yet, I do like the fact that current setup allows the extension's SConstruct to skip "reading" the variables/profile since the godot-cpp one already deals with it (and add such data into the environment).

Locally I have changed the godot-cpp SConstruct to deal with the profile argument differently. In there I prepended the path with the top relative path # symbol. It seems to be working but I don't know if it would mess up anything else.

  1. The following lines doesn't seem to do anything since Import() always "return" None:
try:
    customs += Import("customs")
except:
    pass

Regardless if we Export'ed a customs from the Extension's SConstruct, those lines will always fall into the except branch.
Perhaps while fixing this portion of the code it could also provide means to deal with (2)?

  1. The current SConstruct file given as reference for the extension itself directly loads the godot-cpp build system through SConscript. While this works relatively well for several cases, if we want to provide customization builds for the Extension, adding/removing a single flag will trigger a full rebuild of the godot-cpp. To prevent that I'm currently using two different environments within my SConstruct.

@AThousandShips AThousandShips added discussion topic:buildsystem Related to the buildsystem or CI setup labels Dec 18, 2023
@dsnopek
Copy link
Contributor

dsnopek commented Dec 22, 2023

Thanks!

Hm. Since we do expect the SConstruct from godot-cpp to be used in other projects, which may have added options later or to a different environment, does it even make sense to have opts.UnknownVariables()? Or, is there a way that we could have that only get used when the SConstruct is the top-level SConstruct, and not do it when it's included in another project?

@Kehom
Copy link
Contributor Author

Kehom commented Dec 22, 2023

Hm. Since we do expect the SConstruct from godot-cpp to be used in other projects, which may have added options later or to a different environment, does it even make sense to have opts.UnknownVariables()? Or, is there a way that we could have that only get used when the SConstruct is the top-level SConstruct, and not do it when it's included in another project?

I did indeed consider if the opts.UnknownVariables() does actually make any sense. When godot-cpp SConstruct becomes a subsidiary script (using same nomenclature of SCons documentation), then it indeed doesn't make much sense. But then, we can do isolated builds, which would be interesting to have a warning message in case of misspelling. Indeed it leads to your second suggestion. I do not have enough experience with SCons. I have searched a little about it and I believe it can be done with something like this:

if Dir("#").abspath == Dir(".").abspath:
   # this is top-level script

I did see if there was any other option, but all of them seemed unreliable because of current directory being changed and all. I don't believe Dir(".") will have unreliable results, but then, I have very little experience with SCons (and Python), so I'm unsure of the reliability here. I have quickly tested that code and it seemed to be working.

@Kehom
Copy link
Contributor Author

Kehom commented Jan 5, 2024

I have been experimenting with the opts.UnknownVariables(), specially after adding the code to only output it if it's running as top level script. At first it indeed does work. Because I believe it's useful I decided to add code to output messages about unexpected variables in the extension's SConstruct. That's where I started to notice some things:

  • It completely ignores any variable set in a custom.py or file provided by profile=some_file. This is probably (and most likely) not a real issue, just something that must be kept in mind when dealing with SCons.
  • The now top-level SConstruct containing the message about unknown variables must duplicate the declaration of the expected variables that are specific to godot-cpp. Bellow I add a more detailed explanation.

As an example, if I use profile=some_file or use_mingw=1 then I will get the warning message If those are not also added in the top level script. When those are duplicated within the newly top level SConstruct, calling scons --help will also show those variables twice! I have attempted to import godotcpp using Tool much like it's done in the main script file, just so I could duplicate what is there. However not only I failed at that, I think it would result in duplicated entries when calling scons --help.

At this point I just gave up on using the Unknown variables thingy within my extension's SConstruct.

PS: should I submit a PR as a draft (or something of the like) or more discussion is required? Or someone else with more knowledge adds the changes/fixes to the SConstruct (and tools/godocpp.py)?

@dsnopek
Copy link
Contributor

dsnopek commented Jan 10, 2024

PS: should I submit a PR as a draft (or something of the like) or more discussion is required?

If you have a proposal for how this could be fixed, then please make a draft PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

No branches or pull requests

3 participants