Skip to content

Warn user about unrecognized or misspelled builtin_* options #98298

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

Closed
wants to merge 1 commit into from

Conversation

dustdfg
Copy link
Contributor

@dustdfg dustdfg commented Oct 18, 2024

Just imagine you run scons builtin_webo="no" instead of scons builtin_webp="no". Doesn't work for options defined inside profiles :/ Waiting for #91794 to add warning for profile defined variables too

@dustdfg dustdfg requested a review from a team as a code owner October 18, 2024 14:30
@dustdfg dustdfg changed the title Warn user about unrecognized or misspelled builtin_* options. Warn user about unrecognized or misspelled builtin_* options Oct 18, 2024
@Chaosus Chaosus added this to the 4.4 milestone Oct 18, 2024
Copy link
Contributor

@Repiteo Repiteo left a comment

Choose a reason for hiding this comment

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

Ohhh, this reminds me that I've had a PR for unrecognized options on the backburner for a while now1. Never considered the fact that this wouldn't play nice with anything defined in profiles though; I'm curious now how easily that can be addressed

Having said that, this works as expected. Might be a bit overspecialized, but I don't see this as a long-term solution so that's not a dealbreaker

Footnotes

  1. SCons: Warn when passing unknown variables #91213

@dustdfg
Copy link
Contributor Author

dustdfg commented Oct 18, 2024

Yeah opts.UnknownVariables() ignores variables defined in the profile file for some reasons. It seems that internally it just looks at ARGUMENTS or ARGLIST. So to normally load profile variables and get access in a very raw form can be used approach of Akien to get full list of defined options and then separately update opts:

custom_args = {}
if profiles_to_load:
    for profile in profiles_to_load:
        profile_args = {}
        with open(profile, "r", encoding="utf-8") as f:
            code = f.read()
            exec(code, {}, profile_args)
        custom_args = {**custom_args, **profile_args}
    # Update the variables based on the profiles.
    opts.Update(env, {**ARGUMENTS, **custom_args, **env})

The custom_args variable with using ARGUMENTS (or ARGLIST) and normal env variables could be used to create custom alternative to opts.UnknownVariables(). But PR is old and draft + Akien is on leave now... Politeness, not to "steal" someone else's PR and etc...

# TODO: Add the same check for options defined inside profile
# file. Look at alternative profile loading approach in:
# <https://github.com/godotengine/godot/pull/91794>
for key, _ in opts.UnknownVariables().items():
Copy link
Member

Choose a reason for hiding this comment

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

Forgive my ignorance, but am I correct to assume that opts.UnknownVariables().items() is a list of command line arguments that didn't match to an existing command line option?

Copy link
Contributor Author

@dustdfg dustdfg Oct 28, 2024

Choose a reason for hiding this comment

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

Yes, it is correct if by "existing command line option" you mean something like: opts.Add(BoolVariable("vulkan", "...", True)).

Just formally it should work for profile defined variables too but it doesn't work because SCons is weird

And .items() is redundant (I forgot to delete it after tests 😅 ) but just formally it still works with it. .items() was necessary for loop and wasn't necessary for another place...

Copy link
Contributor Author

@dustdfg dustdfg Oct 28, 2024

Choose a reason for hiding this comment

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

Yes, it is correct if by "existing command line option" you mean something like: opts.Add(BoolVariable("vulkan", "...", True)).

I meant that each platform has it's own platform specific options which added only if your target build platform is that platform. For example linux defines alsa, pulseaudio ... If you build for windows, providing them will be detected by opts.UnknownVariables() as unknown... It is a reason why I decided to create PR that specifically targets exactly builtin_* options as all of them are defined inside main file so won't have problems. This problem is a reason why #91213 is very good thing but needs to wait for Akien profile loading approach to be merged in one or another way.

If you define variables not in the main file, you will encounter some edge cases. For example env["<var>" will sometimes fail. I think it is the main reason why many of the platform specific options are defined inside main file. (vsproj, xaudio2, metal, d3d12...)

The problem of treating these options is a big net (on which I currently try to work 😅) of interconnected features which seems independent... But all of them are connected with one thing. SCons have very weird variable handling so without Akiens approach it is possible to fix it but it will be messy (something like 4-8 new lines of unnecessary code for each new line added with Akiens approach and it is only for places where logic can be mapped...)

@dustdfg
Copy link
Contributor Author

dustdfg commented Nov 12, 2024

Superseded by #99055

@dustdfg dustdfg closed this Nov 12, 2024
@dustdfg dustdfg deleted the builtin_unknowns branch November 12, 2024 20:10
@Repiteo Repiteo removed this from the 4.4 milestone Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants