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

Revised Config #776

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

Conversation

ambiguousname
Copy link
Member

@ambiguousname ambiguousname commented Feb 4, 2025

Put config language in tool/config.rs, and it can currently be set through a config.toml file (which can be customized on the command line).

Some TODOs here:

  • Shared attributes (like lib_name)
    • Logic in Kotlin for overriding these shared attributes
  • Setting config options through the command line
  • Setting config options through attributes
    • Revise system to not use inner attributes (we cannot make rustc ignore inner attributes when compiling)
    • Get feedback on the attributes config system (the current version is just to prove that we can hook it up to everything else).
    • Attribute validation for only top-level types
    • Expand to work recursively in submodules?
  • Fix demo_gen config
  • Automatic override for shared config based on current backend
  • Consistent naming pattern (serde(rename_all="kebab-case")?)
  • Add tests for parsing (and overrides)
  • Documentation

Fixes #772

@ambiguousname ambiguousname added enhancement New feature or request A-tool-UX Area: The UX of the tool itself A-attributes Area: configurability via attributes labels Feb 4, 2025
@ambiguousname ambiguousname marked this pull request as ready for review February 4, 2025 22:47
@ambiguousname ambiguousname marked this pull request as draft February 4, 2025 22:48
@ambiguousname
Copy link
Member Author

ambiguousname commented Feb 8, 2025

Main functionality is in, mostly looking for feedback on:

Config struct

We okay with how the struct is set up and how each backend handles accessing it?

Specifically, pay close attention to how there are two lib_name fields, one belonging to SharedConfig and the other to KotlinConfig. There's logic in the kotlin backend to use either SharedConfig or override, and I want to make sure if we're okay having the backend developers handle this kind of overriding logic themselves.

Attribute system

Can't use a custom inner attribute, they're considered unstable at the moment.

Currently planning on a system like:

#[diplomat::config(some.option = yes)]
struct Config;

// Modules can also have config structs:
pub mod some_other_thing;
/* some_other_thing.rs
#[diplomat::config(option.one = 42)]
struct SomeOtherConfig {}
*/

#[diplomat::bridge]
#[diplomat::config(other.options = sure)]
mod ffi {
  // No configs here
}

I think restricting the config attribute to structs at the top-most level helps with clarity and organization of where config attributes are supposed to go. Didn't want to iterate on this too much until I got some feedback on usage.

Merging system

Feels a little convoluted, but I'm not sure there are many better alternatives? Right now we have to convert every Config struct in to toml::value::Table, then BACK into a Config. Every time we convert between types however, it involves converting the struct into bytes and then back from bytes. So it's just a lot of conversion, don't know if I see a way around it though.

@ambiguousname ambiguousname marked this pull request as ready for review February 8, 2025 17:53
Copy link
Contributor

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

Mostly shaped correctly, will do in depth review later

@@ -38,6 +39,9 @@ pub struct Attrs {
/// not see the impl blocks.
pub attrs: Vec<DiplomatBackendAttr>,

/// Backend specific configuration attributes:
pub config_attrs: Vec<DiplomatBackendConfigAttr>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Thought: these shouldn't be allowed on random types, we should require they go on a toplevel type in lib.rs (with whatever name they get).

@Manishearth
Copy link
Contributor

Can't use a custom inner attribute, they're considered unstable at the moment.

Yeah I think I noted this in the issue.

and I want to make sure if we're okay having the backend developers handle this kind of overriding logic themselves

I would love for the override to be automatic, but I'll look closer at the code.

Feels a little convoluted, but I'm not sure there are many better alternatives?

Tbh not a big deal to have a set_key method that manually passes config values down. I don't think we're going to have tons of these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: configurability via attributes A-tool-UX Area: The UX of the tool itself enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace language-specific config files with a top-level key-value config system
2 participants