Add source fetching settings - #1370
Conversation
|
Positron tests running at posit-dev/positron#15226 |
This comment was marked as outdated.
This comment was marked as outdated.
ee6fec6 to
2cc8147
Compare
juliasilge
left a comment
There was a problem hiding this comment.
Thank you so much for working on this, and the doc page is a really nice addition! I did some manual testing in Positron and I think the environment variable works exactly as advertised and the LSP setting works mid-session too (flipping it back on re-dispatches the packages Oak already knows about). What doesn't work is the setting at startup. Given your timing constraints, what do you think about one of these two options:
-
Ship the env var only for now. Drop the
GLOBAL_SETTINGSentry and the matching section ofconfiguration-oak.md, and we get the CI benefit right away with nothing user-facing that half-works. We can open a follow-up issue for the setting. -
If you have time, calling
update_config()fromhandle_initialized()may be all it takes, though I haven't tried it and there may be a reason it isn't there already.
| }, | ||
| }, | ||
| Setting { | ||
| key: "oak.sourceFetching.enable", |
There was a problem hiding this comment.
Also, the environment variable is OAK_ENABLE_SOURCE_FETCHING which is user-facing and inverts the order relative to the setting. Renaming is free right now and awkward once it ships, so it seems worth it to me. (ruff and ty do keep this pairing aligned, and the PR description cites them for the precedence rule, so there may be an argument for matching them here too.)
There was a problem hiding this comment.
oops, we've used enable for a long time in ark/positron-r:
ark/crates/ark/src/lsp/config.rs
Line 23 in db054eb
I was going for consistency with rust-analyzer. I don't mind that much, so renamed to enabled. The existing variable will be deprecated once Oak gains a corresponding switch.
| // flip this mid-session. Turning it back on picks up every package | ||
| // we've seen so far. Already resolved against the environment by | ||
| // `apply_env_overrides()`. | ||
| if !config.enable_source_fetching { |
There was a problem hiding this comment.
This gate is correct, but I think it runs too late to deliver what the setting is meant to do. With "oak.sourceFetching.enable": false in settings.json, reloading the window and opening no R file at all still produces:
[Info] Fetching sources for package base, 1 pending
[Info] Fetching sources for package datasets, 2 pending
[Info] Fetching sources for package grDevices, 3 pending
[Info] Fetching sources for package graphics, 4 pending
[Info] Fetching sources for package methods, 5 pending
[Info] Fetching sources for package stats, 6 pending
[Info] Fetching sources for package utils, 7 pending
Those are all Priority::Base, so they take the branch at sources.rs:67 and call insert_r(), which downloads the r-source.tar.zst release archive on a cold cache. The setting is off and we're still hitting the network at startup, which is the specific thing it's meant to prevent, if I am understanding this correctly.
The cause is that update_config() has exactly one caller, did_change_configuration at state_handlers.rs:397. Nothing pulls the client's settings during initialize or initialized. But initialize dispatches the workspace scan right away (state_handlers.rs:101-108), so the first schedule() runs while config.oak is still OakConfig::default(), i.e. enable_source_fetching: true. The client's false arrives afterwards, if it arrives at all.
It compounds, because anything fetched in that window is marked Finished and then skipped forever by the contains_key check just below. That skip is a bare continue with no log line, so the package silently never appears again. I hit this while testing the mid-session toggle documented in configuration-oak.md. I believe tibble was fetched during startup, and flipping the setting off and back on never re-dispatched it, with nothing in the output channel explaining why. That took a while to work out from the outside.
Worth noting the env var doesn't have this problem because WorldState::new() resolves it eagerly; the env var is protected against it while the LSP setting isn't. That asymmetry isn't in the docs, which present the two as equivalent apart from precedence.
Would it work to call update_config() from handle_initialized(), before or alongside the initial scan dispatch, so the client's settings are in hand for the first schedule()? Alternatively, hold off source scheduling until the first config response lands? As it is, I don't think false in settings.json prevents the startup fetch.
7c39a1a to
8f44a70
Compare

Follow up to #1369 for posit-dev/positron#15211.
Disable source fetching on CI by default. No need to impact the network and cause LSP flickering as new information comes in on CI.
Add a user-facing LSP setting
oak.sourceFetching.enable, and a corresponding env varOAK_ENABLE_SOURCE_FETCHING. The env-var can be used on CI to opt back in the source fetching mechanism. The env-var always has precedence over the LSP setting, which follows ruff/ty.Start an Oak documentation page that describes the source fetching and cache and how to configure it.
Positron Release Notes
New Features
Bug Fixes