Fix virtual document names in release builds too #1021
Conversation
| // Collect and sort entries alphabetically to match RustEmbed iteration order. | ||
| // https://github.com/posit-dev/positron/issues/11591#issuecomment-3816838107 | ||
| let mut entries: Vec<_> = std::fs::read_dir(directory)? | ||
| .filter_map(|entry| match entry { | ||
| Ok(entry) => Some(entry), | ||
| Err(err) => { | ||
| log::error!("Can't read directory entry: {err:?}"); | ||
| None | ||
| }, | ||
| }) | ||
| .collect(); | ||
| entries.sort_by_key(|entry| entry.path()); | ||
|
|
||
| for entry in entries { | ||
| match entry { | ||
| Ok(entry) => import_file(&entry.path(), src, env)?, | ||
| Err(err) => log::error!("Can't load modules from file: {err:?}"), | ||
| }; | ||
| import_file(&entry.path(), src, env)?; |
There was a problem hiding this comment.
We've got a mix of ? and log + continue style behavior here
The modules feel very important to me, I wonder if log::error!("Can't read directory entry: {err:?}") should really be turned into a ? that propagates upwards rather than logging + continue?
There was a problem hiding this comment.
hmm I think I agree with your sentiment that we're currently too lenient in our init routine.
I've implemented your suggestion and turned the upstream logging into a loud panic. Note that this only concerns the debug path for local module files. We're still lenient in release builds, we should probably fail early there too. WDYT?
Also note that the hot-reloading path still only logs, otherwise we'd get panic for things like syntax errors.
There was a problem hiding this comment.
We're still lenient in release builds, we should probably fail early there too.
I do agree that even in release builds we should probably fail loudly as long as its fairly informative logging.
Modules just feel so critical to ark working, that a loud panic feels right?
There was a problem hiding this comment.
We now panic consistently if modules fail to load or initialise.
| // Collect and sort entries alphabetically to match RustEmbed iteration order. | ||
| // https://github.com/posit-dev/positron/issues/11591#issuecomment-3816838107 | ||
| let mut entries: Vec<_> = std::fs::read_dir(directory)? | ||
| .filter_map(|entry| match entry { | ||
| Ok(entry) => Some(entry), | ||
| Err(err) => { | ||
| log::error!("Can't read directory entry: {err:?}"); | ||
| None | ||
| }, | ||
| }) | ||
| .collect(); | ||
| entries.sort_by_key(|entry| entry.path()); | ||
|
|
||
| for entry in entries { | ||
| match entry { | ||
| Ok(entry) => import_file(&entry.path(), src, env)?, | ||
| Err(err) => log::error!("Can't load modules from file: {err:?}"), | ||
| }; | ||
| import_file(&entry.path(), src, env)?; |
There was a problem hiding this comment.
We're still lenient in release builds, we should probably fail early there too.
I do agree that even in release builds we should probably fail loudly as long as its fairly informative logging.
Modules just feel so critical to ark working, that a loud panic feels right?
lionel-
force-pushed
the
bugfix/non-srcref-labels
branch
from
9345c3b to
04fa753
Compare
lionel-
force-pushed
the
bugfix/non-srcref-labels
branch
from
04fa753 to
29e7fa8
Compare
2 participants
Addresses posit-dev/positron#11591
I had actually fixed this in #1003 by adding (what I thought was) a new
as_label()function extracted from rlang:ark/crates/ark/src/modules/positron/calls_deparse.R
Line 12 in 8c819ef
Turns out, there was already a much more bare bones
as_label()in the repo:ark/crates/ark/src/modules/positron/utils.R
Line 66 in 8c819ef
Here's the thing:
When loading the R files from disk, as happens in a debug build, the new
as_label()overwrote the old one. So my new implementation was working as expected.When loading the R files from the binary blob, as happens in a release build, rust-embed loads the files alphabetically (https://docs.rs/crate/rust-embed-utils/8.11.0/source/src/lib.rs#19), and the new impl gets overwritten by the old one!
This had me very confused for a while!
This PR removes the dangling
as_label(), and adjusts the modules loading mechanism to match rust-embed so this doesn't happen again in the future.