Fix and clarify cache file handling #4325
Conversation
| pub fn load(&self, file: GleamFile) -> Result<Input> { | ||
| let name = file.module_name.clone(); | ||
| let source_mtime = self.io.modification_time(&file.path)?; | ||
|
|
||
| let read_source = |name| self.read_source(path, name, source_mtime); | ||
| let read_source = |name| self.read_source(file.path.clone(), name, source_mtime); |
There was a problem hiding this comment.
I didn't spend too much time trying to get rid of these two clones, but there might be something we could still optimize. I thought I'd get overall feedback first before going too detailed.
| if (!inputs.contains_key(&module)) { | ||
| tracing::debug!(%module, "module_removed"); | ||
| CacheFiles::new(&self.artefact_directory, &module).delete(&self.io); |
There was a problem hiding this comment.
This line is the fix for the temporarily deleted files not being always recompiled.
| .with_extension("cache_meta"); | ||
|
|
||
| let _ = self.io.delete_file(&meta_path); | ||
| CacheFiles::new(&self.artefact_directory, &cached.name).delete(&self.io); |
There was a problem hiding this comment.
Here we delete more files than before.
| fn module_name(path: &Utf8Path, dir: &Utf8Path) -> EcoString { | ||
| // /path/to/project/_build/default/lib/the_package/src/my/module.gleam | ||
|
|
||
| // my/module.gleam | ||
| let mut module_path = path | ||
| .strip_prefix(dir) | ||
| .expect("Stripping package prefix from module path") | ||
| .to_path_buf(); | ||
|
|
||
| // my/module | ||
| let _ = module_path.set_extension(""); | ||
|
|
||
| // Stringify | ||
| let name = module_path.to_string(); | ||
|
|
||
| // normalise windows paths | ||
| name.replace("\\", "/").into() | ||
| } |
There was a problem hiding this comment.
This was moved, and made private so that it wouldn't be used with cache files (which caused some extra recompilation because we thought the modules were removed).
| fn is_gleam_path(path: &Utf8Path, dir: &Utf8Path) -> bool { | ||
| use regex::Regex; | ||
| use std::cell::OnceCell; | ||
| const RE: OnceCell<Regex> = OnceCell::new(); | ||
|
|
||
| RE.get_or_init(|| { | ||
| Regex::new(&format!( | ||
| "^({module}{slash})*{module}\\.gleam$", | ||
| module = "[a-z][_a-z0-9]*", | ||
| slash = "(/|\\\\)", | ||
| )) | ||
| .expect("is_gleam_path() RE regex") | ||
| }) | ||
| .is_match( | ||
| path.strip_prefix(dir) | ||
| .expect("is_gleam_path(): strip_prefix") | ||
| .as_str(), | ||
| ) | ||
| } |
There was a problem hiding this comment.
This was just moved, as it wasn't used anywhere else.
| fn module_name(dir: &Utf8Path, path: &Utf8Path) -> EcoString { | ||
| // /path/to/artefact/dir/[email protected]_meta | ||
|
|
||
| // [email protected]_meta | ||
| let mut module_path = path | ||
| .strip_prefix(dir) | ||
| .expect("Stripping package prefix from module path") | ||
| .to_path_buf(); | ||
|
|
||
| // my@module | ||
| let _ = module_path.set_extension(""); | ||
|
|
||
| // my/module | ||
| module_path.to_string().replace("@", "/").into() |
There was a problem hiding this comment.
This is the corrected and modified version of getting the module name from any cache file.
| assert!(response.result.is_ok()); | ||
| assert_eq!(response.result, Ok(())); |
There was a problem hiding this comment.
I got this test failing as a result of a typo, and the output wasn't too helpful. This makes it a lot better, so I thought I'd commit it.
| @@ -1715,3 +1682,143 @@ impl<'a> Inputs<'a> { | |||
| Ok(()) | |||
| } | |||
| } | |||
|
|
|||
| /// A Gleam source file (`.gleam`) and the module name deduced from it | |||
There was a problem hiding this comment.
For everything downwards from here, I'm not sure if this is the best module, or the best location in the source file to put these.
Functional changes:
Refactoring:
replace("/", "@"). Add two versions of it: One for erlang module names, and one for cache file names. My understanding is that they just happen to do the same thing, so I duplicated it. If there's some reason the cache file names must match the erlang module names, then we should probably change it further.replace("/", "@")related to cache files, group these operations together asCacheFiles, as there was further duplication of the same logic in different parts of the code base.GleamFileI was originally going to refactor first separately, but it was hard to keep doing the wrong thing, and since the actual fixes are quite small, I thought it might be better if I just outlined them in the PR.