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: Convert platform get_flags to dictionary #92124

Merged
merged 1 commit into from
May 23, 2024

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented May 19, 2024

Minor formatting update to make get_flags return a dictionary instead of a tuple array, as it's already being parsed as if it were a dictionary.

Copy link
Contributor

@Riteo Riteo left a comment

Choose a reason for hiding this comment

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

Makes sense! Definitely improves readability :D

Copy link
Contributor

@Eoin-ONeill-Yokai Eoin-ONeill-Yokai left a comment

Choose a reason for hiding this comment

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

This looks great. It certainly helps the readability of the flags section for platform code.

@akien-mga
Copy link
Member

In general this makes a lot of sense, but I'm a bit concerned that it's compat breaking for third-party platform ports: consoles, homebrews, embedded devices, etc.

I can say from the W4 side that it's a change we're happy to carry over downstream, but I can't make that decision for all other third-parties, who may be trying to support multiple Godot minor releases for the same codebase (with a few occasionally #ifdefs checking the Godot version).

I just had a quick look and it seems possible to preserve compatibility downstream by doing a Godot version check like this:

def get_flags():
    import version

    if (version.major, version.minor) < (4, 3):
        return [
            ("arch", detect_arch()),
            ("supported", ["mono"]),
        ]
    else:
        return {
            "arch": detect_arch(),
            "supported": ["mono"],
        }

So I think it's fine for now, I'm not aware of downstream ports that would want to support multiple versions, but if some do, they can use a similar snippet to achieve that.

We should keep this in mind if we ever want to make get_flags() strongly typed to Dictionary, which may prevent such workaround. (So if you're a downstream port maintainer reading this and relying on such a workaround, please make yourself known.)

@akien-mga akien-mga modified the milestones: 4.x, 4.3 May 22, 2024
@akien-mga
Copy link
Member

Note, one other option would be to add compat code ourselves in SConstruct, handling the get_flags() results differently based on whether it's a list or a dictionary.

We had to do that in the past when change the expected arguments of the modules' can_build, as way too many thirdparty modules broke unnecessarily. Here the risk is more theoretical, thirdparty platforms are a lot less widespread than modules, so it may not be worth the added complexity in our SConstruct. Though we could hide the complexity away in methods.py and just call a method to convert the array to a dict.

@Repiteo Repiteo force-pushed the scons/platform-flags-dict branch from 3160ef9 to 896b003 Compare May 22, 2024 18:53
@Repiteo
Copy link
Contributor Author

Repiteo commented May 22, 2024

A compatibility check was easy enough to implement:

        platform_flags[x] = detect.get_flags()
        if isinstance(platform_flags[x], list):  # backwards compatibility
            platform_flags[x] = {flag[0]: flag[1] for flag in platform_flags[x]}

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks great!

@akien-mga akien-mga merged commit de49025 into godotengine:master May 23, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@Repiteo Repiteo deleted the scons/platform-flags-dict branch May 24, 2024 17:52
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.

5 participants