doc: add clients config setup flow#433
Conversation
|
Code coverage summary for 9e72773: ✅ Region coverage 69% passes |
kp-antonio-yang
force-pushed
the
doc/client-config
branch
from
6b1598a to
10db114
Compare
kp-antonio-yang
force-pushed
the
doc/client-config
branch
4 times, most recently
from
e3bbe81 to
4368088
Compare
kp-antonio-yang
force-pushed
the
doc/client-config
branch
2 times, most recently
from
d92647c to
8cdd62f
Compare
kp-samuel-tam
left a comment
kp-samuel-tam
left a comment
There was a problem hiding this comment.
It's a great write-up overall, the flow chart captures the layering really well. I would like to raise a few blockers.
| 6 ==> clientFn | ||
| ``` | ||
|
|
||
| As shown, `Config` is the single source of truth for all clients — all user inputs, whether from a UI or a file, |
There was a problem hiding this comment.
From this line onwards there's a clear disjoint between the convention you're documenting and how Config actually ships today. Looking at the merged PR #411:
wintun_file,enable_dpapi→#[cfg(windows)](target gate on field)route_mode,dns_config_mode,enable_pmtud,enable_tun_iouring,enable_inside_pkt_encoding→#[cfg(desktop)](custom cfg on field)enable_batch_receive→#[cfg(batch_receive)](custom cfg on field)
The doc says "feature gates belong on fields" but there's 8+ fields in the prior PR being documented use target/custom cfg on fields.
Even if we're aligned on the direction, the code needs to follow the rule before we declare it a developer practice in the docs.
There was a problem hiding this comment.
Yeh, not all the code following the design and only mobile does.
The reason it when we use json schema on windows/macos client and we will refactor it to follow this design. For now, we do the least change on the code when mobile introduce. And it will make really sense when developer working on that target and test it at the same time, so we do not touch these target now.
There was a problem hiding this comment.
The lingering concerns are a signal that the documentation itself isn't clear enough. Ideally, after reading your doc, people should walk away feeling that what you're proposing makes sense.
The current document covers the what in depth, but doesn't really address the why. This is especially important in your case because you're deviating from the standard, which naturally raises concerns. Addressing those concerns head-on is exactly what this change needs to do, and it's also why I suggested writing this up in the first place.
In a technical context, explaining the why well means laying out your motivation, the technical constraints you're working within, and how this design directly addresses those challenges. I'd suggest following this exact structure in your doc for clarity.
There was a problem hiding this comment.
Yes, the why is the important and making focus.
A introduction is added, and also the using scenario put more details about why the scope is boarder than before. This issue should not be a problem.
| fn main () { | ||
| let config = Config::load(); | ||
| client(config) | ||
| } | ||
|
|
||
| #[cfg(window)] | ||
| fn client(config: Config) { | ||
| let Config { | ||
| win_only_field, | ||
| .. | ||
| } = config; | ||
|
|
||
| if win_only_filed > 256 { | ||
| // ... | ||
| } | ||
| } | ||
|
|
||
| #[cfg(android)] | ||
| fn client(config: Config) { | ||
| let Config { | ||
| android_only_field, | ||
| .. | ||
| } = config; | ||
| let tun = Tun::new(android_only_field); | ||
| } |
There was a problem hiding this comment.
Looks like this pseudocode was copy-pasted from #411 (comment) with typos - #[cfg(window)] is missing the s, and win_only_filed should be win_only_field.
Also, this code actively encourages the paired use of #[cfg(windows)] and #[cfg(feature="windows")]. This is unsound because we can't ever guarantee they are in sync or supported, adding to the drift Thomas was trying to talk about.
There was a problem hiding this comment.
It will always need to enable the corresponding feature gate for targets with this design, like current mobile feature.
Also, we can make sure it always sync and supported via cargo make, and also that is the reason we have script or make file in the project.
There was a problem hiding this comment.
How about we make this less controversial by only applying this rule on mobile for now? i.e.:
- Mobile -> use
#[cfg(feature="mobile")]to allow schema generation on desktop - Desktop -> follow Rust's default convention
As you noted in the doc, desktop won't adopt this convention until the larger refactor lands. Any new desktop config added before then would deviate from the proposal anyway, which somewhat undercuts its value if we roll it out broadly now.
There was a problem hiding this comment.
The intent was never to lay down strict rules — it started as a brief three-line comment to guide developers when they need to revisit the JSON schema. As it evolved into a document, it made more sense to capture not just the current state but also the details we will likely encounter going forward, especially since the discussion is already open. Current document put introduction and easier for reader to have the focus.
There was a problem hiding this comment.
The intent was never to lay down strict rules
No, by writing down what people should do, you are setting rules, no matter what you call it.
If you want to keep your vision for the desktop config, you may put it in a section labeled "Future Plans", but it must be distinctive that it's not meant to be followed.
kp-antonio-yang
force-pushed
the
doc/client-config
branch
4 times, most recently
from
57667a2 to
5ce523f
Compare
|
Hi @kp-samuel-tam |
kp-antonio-yang
force-pushed
the
doc/client-config
branch
from
5ce523f to
7d85daa
Compare
| ## Use cases by developer persona | ||
|
|
||
| The general config centralizes all settings into a single `Config` struct and aligns the config generation flow across clients. However, only the mobile feature has been implemented with this design so far — existing clients may not adopt it until a larger refactor happens. | ||
| There are two possible directions going forward: |
There was a problem hiding this comment.
Could you clarify when to use each direction?
It's not clear what the differences are or which scenarios call for which approach. If they're effectively the same, let's pick one as the standard going forward.
There was a problem hiding this comment.
If we can reach a decision on Option 1, I can drop the redundant documentation and keep things lean. Once that lands, a follow-up PR can enforce the same design across the desktop clients.
Personally, I lean towards Option 1 with feature gates. Since we are already using Makefile.toml, passing --features is not an extra burden — it is a common pattern in our build flow for each target. However, since there seems to be resistance towards --feature=target, so I am leaving this as an open question for now.
| @@ -0,0 +1,144 @@ | |||
| # General Config | |||
There was a problem hiding this comment.
Suggesting a more concise title:
| # General Config | |
| # Client Config Design Rationale |
Can you also link this doc in lightway-client/src/config.rs, such that people are aware that there is a relevant doc here?
There was a problem hiding this comment.
It is General Config.
I should doc the server part, although it is really similar with cli part of the client.
There was a problem hiding this comment.
Then I would suggest Client/Server Config Design Rationale. The word "General" doesn't really mean anything.
I see you've extended the diagram to include server as well, but that has become a bit messy. Can you separate it into its own flow chart?
| 6 ==> clientFn | ||
| ``` | ||
|
|
||
| As shown, `Config` is the single source of truth for all clients — all user inputs, whether from a UI or a file, |
There was a problem hiding this comment.
The lingering concerns are a signal that the documentation itself isn't clear enough. Ideally, after reading your doc, people should walk away feeling that what you're proposing makes sense.
The current document covers the what in depth, but doesn't really address the why. This is especially important in your case because you're deviating from the standard, which naturally raises concerns. Addressing those concerns head-on is exactly what this change needs to do, and it's also why I suggested writing this up in the first place.
In a technical context, explaining the why well means laying out your motivation, the technical constraints you're working within, and how this design directly addresses those challenges. I'd suggest following this exact structure in your doc for clarity.
| fn main () { | ||
| let config = Config::load(); | ||
| client(config) | ||
| } | ||
|
|
||
| #[cfg(window)] | ||
| fn client(config: Config) { | ||
| let Config { | ||
| win_only_field, | ||
| .. | ||
| } = config; | ||
|
|
||
| if win_only_filed > 256 { | ||
| // ... | ||
| } | ||
| } | ||
|
|
||
| #[cfg(android)] | ||
| fn client(config: Config) { | ||
| let Config { | ||
| android_only_field, | ||
| .. | ||
| } = config; | ||
| let tun = Tun::new(android_only_field); | ||
| } |
There was a problem hiding this comment.
How about we make this less controversial by only applying this rule on mobile for now? i.e.:
- Mobile -> use
#[cfg(feature="mobile")]to allow schema generation on desktop - Desktop -> follow Rust's default convention
As you noted in the doc, desktop won't adopt this convention until the larger refactor lands. Any new desktop config added before then would deviate from the proposal anyway, which somewhat undercuts its value if we roll it out broadly now.
kp-antonio-yang
force-pushed
the
doc/client-config
branch
5 times, most recently
from
6d460c2 to
dddfeec
Compare
kp-antonio-yang
force-pushed
the
doc/client-config
branch
from
dddfeec to
3248fab
Compare
|
Please take a look at the updated doc when you get a chance. 🙏
On the options section — if we would rather not leave it open-ended in the doc, I am happy to implement either Option 1 or Option 2 in a follow-up PR. But we need to reach consensus here first before I proceed. |
kp-antonio-yang
force-pushed
the
doc/client-config
branch
from
3248fab to
7f719d8
Compare
- documents with flow about config changes in cli, mobile - add mermaid chart for the flow - add how to apply the flow when introducing a new client
Point out ther server flow is the same as client cli flow.
Add links of config.rs to design markdown document both server and client
kp-antonio-yang
force-pushed
the
doc/client-config
branch
from
7f719d8 to
72f5f31
Compare
The using case with user persona is updated, such that we can consider this topic in details.
kp-antonio-yang
force-pushed
the
doc/client-config
branch
from
72f5f31 to
861861d
Compare
Adds a future plan section to make it easier for developers to understand the recommended approach when applying JSON schema to additional targets.
In order to have better understaning, we use thesis format to avoid ambiguity.
3 participants
Description
Motivation and Context
The client code serves multiple clients, and developers need to handle a range of use cases. Having clear documentation is important for anyone working in this area.
This PR documents the work from the previous PRs — #386, #388, and #411. Further documentation will follow in future PRs as more work lands. Easy to read in web page.
How Has This Been Tested?
Document, no extra test needed
Types of changes
Checklist:
main