client: mobile: accept config content#411
Conversation
kp-antonio-yang
changed the title
|
Code coverage summary for 36bb7e7: ✅ Region coverage 69% passes |
kp-antonio-yang
force-pushed
the
feat/mobile-read-conf
branch
5 times, most recently
from
1a1e2b1 to
2e036f1
Compare
| // NOTE for JsonSchema | ||
| // - Use feature gate not cfg, such that cli client can generate schema based | ||
| // on features for other target if needed |
There was a problem hiding this comment.
Can you share more about this? It has always been possible to cross compile to other targets
There was a problem hiding this comment.
A Windows/macOS GUI client can still be cross-compiled from a Linux machine. In that case, the developer would run the client on Linux to generate the JSON schema, which would then be used for the Windows/macOS build. This is just a note to keep in mind for future cross-compilation scenarios.
There was a problem hiding this comment.
Okay, but can you share more about why we should use feature gate instead of cfg? Do you mean we shouldn't use Rust's conditional compilation based on platform, but use cargo features for platform specific configs?
There was a problem hiding this comment.
Yeh, cfg with target on functions are good and common in our repo, and I did not against it, so I just add a note on the struct for the #[cfg(feature = ...)]exception.
This is a exceptional note in for the struct to show the JSON schema. Guide a developer need add fields and also considering to generate a schema for other platform.
There was a problem hiding this comment.
This is still unclear to me. Allow me to try to paraphrase:
Prefer `#[cfg(feature = "...")]` over target cfgs (e.g. `cfg(windows)`)
so CLI on any host can generate schema for other targets via cargo features.
Did I capture what you're trying to say?
There was a problem hiding this comment.
Theoretically, we could download artifacts from CI, run across multiple platforms, and store the JSON schema as artifacts. In practice however, this does not always hold up:
- The CLI cannot run on mobile clients, so there is no way to generate the JSON schema from an Android device. Putting the CLI on an Android device purely to produce a schema in the CI flow does not make sense. This also means there is no consistent approach to config and JSON schema generation if some fields are gated by target and others by feature.
- As mentioned in my earlier example, a fronted full stack developer cannot easily obtain the JSON schema for all platforms from a working feature branch on a fork repo without all platform supported CI.
There will likely be more edge cases along these lines. I believe keeping this simple makes the library significantly easier to use as an open source project, and these small non-ideal tradeoffs are worth tolerating.
The practical approach with the least friction for everyone is:
- Feature gates should be on the fields of the config struct.
- cfg target attributes are well suited for functions, as mentioned previously — just don't break the 1.
There was a problem hiding this comment.
Good point about not being able to generate a schema on mobile via CLI. But I'm still confused about what you're proposing. Can you see if the following matches your intent? Please feel free to write if none of them captures exactly what you mean
A. Swap one Rust gate for another
- Replace every occurrence of
#[cfg(windows)]with#[cfg(feature = "windows")] - That would involve adding a
windowsCargo feature. Schema is regenerated per feature combo on the host.
B. Drop the Rust gate on the field; use x-cfg only
- Target-varying fields stay unconditionally compiled;
#[schemars(extend("x-cfg" = "..."))]carries the platform meaning for the UI. - One schema covers all platforms, generated once from any host.
- Cargo features (
postquantum,debug, etc.) keep their#[cfg(feature = "...")]gates.
There was a problem hiding this comment.
Give the biggest tailoring power to developer via schema from ONE cli
B. Drop the Rust gate on the field of Config; use x-cfg only
- Target-varying fields stay unconditionally compiled; #[schemars(extend("x-cfg" = "..."))] carries the > platform meaning for the UI.
- One Schema cover all platforms generated from host which can run cli.
- Cargo features (postquantum, debug, etc.) keep their #[cfg(feature = "...")] gates.
What is biggest tailoring power?
As an open source project, the greatest power means the lowest barrier — easiest to use, with details that are clear without requiring code changes:
- No code modifications or special machines needed. The schema can target any platform with the desired feature flags (postquantum, debug, etc.).
- All details are discoverable from the schema itself without reading source code — conveyed via x-cfg, format, and other JSON schema attributes.
Why does tailoring power matter?
By lowering down the integration efforts, developers can easy to integrate to their tools, then use it, eventually contribute back. It is about interface, so it is consumer facing and it is spec. As a spec of lib or an opensource project here, the consumer is developer.
Off-topic for this PR but related — my mental model of feature vs cfg:
There is a set of functions sharing the same signature, where #[cfg(target)] automatically selects the correct implementation at compile time. This means a Windows developer and an Android developer working in the same project speak almost the same domain language:
struct Config {
#[cfg(feature="windows")]
#[schemars(extend("x-cfg" = "windows"))]
win_only_field: usize,
#[cfg(feature="android")]
#[schemars(extend("x-cfg" = "android"))]
android_only_field: usize,
// ...
}
fn main () {
let config = Config::load();
connect(config)
}
#[cfg(window)]
fn connect(config: Config) {
let Config {
win_only_field,
..
} = config;
if win_only_filed > 256 {
// ...
}
}
#[cfg(android)]
fn connect(config: Config) {
let Config {
android_only_field,
..
} = config;
let tun = Tun::new(android_only_field);
}There was a problem hiding this comment.
You lost me again. I have 2 questions for you and I would like you to answer them directly:
- Does the current comment, prefer
#[cfg(feature = "...")]over target cfgs (e.g.cfg(windows)), captures what you meant to say? - What's the primary technical reason behind your comment? You've listed several social reasons above, but I don't think they hold water.
There was a problem hiding this comment.
-
Yes, the preference is
#[cfg(feature = "...")]over target-based gates on Config struct fields. Other target gates like#[cfg(windows)]are not touched — this PR is specifically about mobile accepting config content, and other targets are out of scope here. Once other clients start adopting JSON schema, their PRs can modify the fields they need, following the pattern established here and guided by the note. -
It is true that there is no cli for mobile, and the mobile target needs a feature gate to generate the schema.
If there is no consensus on whether to prefer #[cfg(feature = "...")] over target gates as a general guide, we can leave that question open, remove the note for now, and let the mobile changes land as they are. We can revisit the broader convention in a follow-up once alignment is reached.
kp-antonio-yang
force-pushed
the
feat/mobile-read-conf
branch
2 times, most recently
from
23318d3 to
762bcdd
Compare
kp-antonio-yang
force-pushed
the
feat/mobile-read-conf
branch
from
762bcdd to
bcd8d45
Compare
xv-thomas-leong
left a comment
xv-thomas-leong
left a comment
There was a problem hiding this comment.
Some more comments
| // NOTE for JsonSchema | ||
| // - Use feature gate not cfg, such that cli client can generate schema based | ||
| // on features for other target if needed |
There was a problem hiding this comment.
This is still unclear to me. Allow me to try to paraphrase:
Prefer `#[cfg(feature = "...")]` over target cfgs (e.g. `cfg(windows)`)
so CLI on any host can generate schema for other targets via cargo features.
Did I capture what you're trying to say?
kp-antonio-yang
force-pushed
the
feat/mobile-read-conf
branch
2 times, most recently
from
975f7bb to
c6a6a9f
Compare
kp-antonio-yang
force-pushed
the
feat/mobile-read-conf
branch
from
c6a6a9f to
f0ee457
Compare
kp-antonio-yang
force-pushed
the
feat/mobile-read-conf
branch
from
b5d1f89 to
a457946
Compare
Pass field meta information of json schema to client, such that mobile clients can based on the meta information to show the field really works in mobile client - pass textarea format for larger String field - pass password format for credentials String field - pass x-cfg information - add example description for durations and socket address fields
By adding a config_content input, the mobile will easy to apply yaml configs as the same as cli client using, such that we can easy to treat all clients in a unify way, and also the OSS client can directly use dynamic UI, such that the overall maintenance efforts will be less.
The build and clean command in dev shell will always run on project root.
Remove rust prefix, because we do not have benefit from this, and it will still good to use in mobile client without issues.
Mobile client now using the same input to start the vpn network, and reduce the code complexity of client side.
Use the same field name for the user name of auth in global level (Config) and in connection level (ConnectionConfig)
kp-antonio-yang
force-pushed
the
feat/mobile-read-conf
branch
from
a457946 to
1abe436
Compare
|
Thanks for the reviews. Will add document under /doc for this in #433 |
2 participants
This PR splits the mobile interface commits out from #391, allowing the mobile project to iterate independently without being blocked by the ongoing internal config unification work.
Description
EventHandlers, which is also an interface in Koltlinusernamefield under server touser, and the same field name in global level)Motivation and Context
By decoupling the mobile project from the lightway project, the mobile client can move faster and more easily adopt a dynamic UI.
This PR is split from #391. It is for 3M part of overall flow chart, and the meanwhile the 2M is dropped.
How Has This Been Tested?
Following existing tests.
Types of changes
Checklist:
main