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

Add Support for Config Inheritance #440

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add Support for Config Inheritance #440

wants to merge 5 commits into from

Conversation

hohle
Copy link
Contributor

@hohle hohle commented Feb 5, 2025

Adds support for a top-level parent key in configuration files which will merge the contents of that path with the contents of the current file. The merge rules are the same as specifying multiple configuration files at the command line with the "parent" config being loaded first.

This also moves configuration loading out into the splat.conf module so configuration loading can be used as a library by other projects.

Adds support for a top-level `parent` key in configuration files which
will merge the contents of that path with the contents of the current
file. The merge rules are the same as specifying multiple configuration
files at the command line with the "parent" config being loaded first.

This also moves configuration loading out into the `splat.conf` module
so configuration loading can be used as a library by other projects.
@hohle
Copy link
Contributor Author

hohle commented Feb 5, 2025

This is mostly an RFC/POC. If the project is interested in this approach, I'm happy to work through any other changes or design considerations. This came out of some pain points specifically in sotn-decomp, but seems general enough for any multi-target project.

As sotn-decomp has grown, we have dozens of splat configs for similar overlays. While investigating upgrading to 0.32.2, we found most of these would need to have a new key added. Adding this to the build system is possible, but making it a first class feature reduces some other redundant work.

We also have a lot of custom tooling for overlays for finding duplicates, reporting progress, matching symbols, etc., etc., which all either duplicate our splat config in some form or read the raw YAML and interpret it directly. Exposing the loading the configuration allows convenience features like this (and later variable string expansion) to be used in other projects without duplicating or simulating the logic.

@hohle
Copy link
Contributor Author

hohle commented Feb 5, 2025

Will look at the failures in a bit. fixed types and formatting

Copy link
Collaborator

@AngheloAlf AngheloAlf left a comment

Choose a reason for hiding this comment

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

What's the difference between this configuration option and passing multiple yamls in the cli?

@hohle
Copy link
Contributor Author

hohle commented Feb 5, 2025

What's the difference between this configuration option and passing multiple yamls in the cli?

It's mostly semantic, but the purpose is to avoid duplicating not just config but also build details across lots of tools and systems. Specifying a parent allows the config to define the relationship between other config files rather than external tooling and aids in making normalized splat config easier to use in other tools. It also explicitly communicates to other devs the relationship between config.

Specifying on the command line can produce the same output, and even model the same concept, however, it is required that you know at execution time what the correct order of the config files is to specify. Adding an explicit parent provides a formal order for merging configs (like classical inheritance) while only needing one of the leaf configs as CLI input. This is nice for modeling a project with many of the same type of overlays and a large matrix of versions.

This also retains the functionality of passing multiple configs. After the inheritance path is resolved, values can still be overridden by subsequent config files. That has value for overriding things at execution time. I could see this being useful for modifying options for particular build targets (debug, coverage, reporting, etc.) as opposed to being part of the structural layout of the project.

This work is based on what I've seen on sotn-decomp which will have between 60 and 170 targets depending on the version. The inheritance tree for sotn-decomp may become somewhat deep:

  • splat.yaml - project global options, base_path, compiler, general settings, etc.
  • splat.$console.yaml - platform, section order, etc.
  • splat.$version.yaml - symbol name format, base symbol table file
  • splat.$version.$type.yaml - common starting segment (hypothetical)
  • splat.$version.$type.$override.yaml - specific paths, segments, additional symbols tables

Here's a small example of how that would change 2 of the 16 overlays we currently have for stages for one version of the game - Xeeynamo/sotn-decomp@master...hohle:sotn-decomp:splat-hierarchy

This was able to be changed without having to touch any build tools or other scripts.

@ethteck
Copy link
Owner

ethteck commented Feb 6, 2025

Thanks a lot for this PR and your thorough explanation. I'm trying to wrap my head around it, and it might be a suitable way to implement the multi-version support I was considering adding soon. I think I'm a little unclear how this would work if you want to edit/insert/delete segments themselves between different versions. Is there an example of that, that you have working?

@hohle
Copy link
Contributor Author

hohle commented Feb 6, 2025

This doesn't affect the merge logic at all, so lists are still append only.

I did a POC to try to leverage YAML anchors defined in a parent and then used in the child, however, both files are loaded independently, so the anchors are not available when the child is loaded. Some work could be done during loading to make that possible by implementing a custom scanner, but then the YAML becomes invalid.

I'm coming at this almost exclusively from the lens of sotn-decomp, and may be a bit myopic. If there's another project you have in mind I can do a POC for that as well.

@hohle
Copy link
Contributor Author

hohle commented Feb 6, 2025

The inheritance thing is taking over most of the conversation, but the other component, which is providing support for loading splat config as a library could be pulled out into a separate PR and provides value to us as well.

I'm not sure conf.initialize is the best way to expose that to other projects, so any feedback there would be helpful as well.

@ethteck
Copy link
Owner

ethteck commented Feb 7, 2025

The config-as-a-library thing is a clear benefit and maybe it would be nice to have that as a separate PR if you don't mind. I'll need a tad more time to test and think about the other stuff, but I would be happy merging that in quickly.

conf.load ? Initialize is fine, but I tend to prefer shorter names personally

@hohle
Copy link
Contributor Author

hohle commented Feb 13, 2025

Sounds good. I'll send out another PR for splat.util.conf.

@hohle
Copy link
Contributor Author

hohle commented Feb 13, 2025

#442

@ethteck
Copy link
Owner

ethteck commented Feb 17, 2025

@hohle when you're ready for us to check this one out again, let us know. no rush though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants