[move] Fix vfs relativization #16987
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| @@ -347,7 +339,7 @@ impl Compiler { | |||
|
|
|||
| source_text | |||
| .iter_mut() | |||
| .for_each(|(_, (path, _))| *path = relativize_path(&vfs_root, *path)); | |||
| .for_each(|(_, (path, _))| *path = vfs_to_original_path[path]); | |||
There was a problem hiding this comment.
This map access should not be unconditional as new paths can be generated between when the map is initialized and when it's used here, so not every path will have an entry in the map.
There was a problem hiding this comment.
yeah I found out as much from tests. Attempting to fix but it might blow up on windows
| let paths: Vec<Symbol> = if vfs_root.is_none() { | ||
| let paths: Vec<_> = paths.into_iter().map(|p| p.into()).collect(); | ||
| let paths: Vec<_> = paths.iter().map(|p| p.as_str()).collect(); | ||
| find_filenames_and_keep_specified(&paths, &mut find_move_files)? |
There was a problem hiding this comment.
In general, I want to get the compiler out of the business of finding files. It is a relic from before the package system. But its functionality is relied on a bit too much to rip out from this PR, though I totally can if y'all want me to.
There was a problem hiding this comment.
I'm a very large fan of ripping it out as soon as possible.
| // TODO this should really be done by the package system, with the interface files stuffed into the | ||
| // build/ directory for the package. This would give a more consistent location for errors. |
There was a problem hiding this comment.
Speaking of pre-package system relics... it would be really great to make this a package system feature someday
| pub fn set_compiler_vfs_root(mut self, vfs_root: VfsPath) -> Self { | ||
| assert!(self.compiler_vfs_root.is_none()); | ||
| self.compiler_vfs_root = Some(vfs_root); | ||
| self | ||
| } |
There was a problem hiding this comment.
I find this very unsatisfying, but am unsure what to do.
There was a problem hiding this comment.
This could be std::mem::replace where you hand back the old one and make the caller deal with it
| .collect::<Result<Vec<_>, anyhow::Error>>()?; | ||
|
|
||
| for path in targets.iter().chain(&deps) { | ||
| if !path.path.is_file().unwrap_or(false) { | ||
| debug_assert!( |
There was a problem hiding this comment.
Do we need the assertion since we are bailing anyways?
There was a problem hiding this comment.
I want the assertion so we get a panic instead of an Err in debug builds
| let vfs = p.to_vfs_path(&vfs_root)?; | ||
| vfs_to_original_path.insert(Symbol::from(vfs.path.as_str()), original); | ||
| Ok(vfs) | ||
| }) | ||
| .collect::<Result<Vec<_>, anyhow::Error>>()?; |
There was a problem hiding this comment.
Something to mention is there were some build issues in migration where two packages pointed to the same dependency via different relative paths, and those got reported as conflicting even though they were the same. I don't think we should fix that in this PR, but I suspect it may impact some of this code.
| let mut files = file_paths | ||
| .into_iter() | ||
| .map(path_to_string) | ||
| .collect::<anyhow::Result<Vec<String>>>()?; |
There was a problem hiding this comment.
Should these be filtered by is_file_desired?
There was a problem hiding this comment.
No, the intention of this function versus find_files is to keep any file specified.
Description
Test Plan
If your changes are not user-facing and do not break anything, you can skip the following section. Otherwise, please briefly describe what has changed under the Release Notes section.
Type of Change (Check all that apply)
Release notes
Fixed issues with
sui movenot working incmd.exeon Windows.