mirror of
https://github.com/rojo-rbx/rojo.git
synced 2026-10-23 05:51:39 +00:00
Fix syncback process to handle directory paths and class transitions in clean mode
- Resolved an issue where syncback failed for projects with `$path` pointing to single files by ensuring only directory paths are scanned for orphans. - Implemented a function to create missing directories in clean mode, allowing restoration of deleted directories. - Enhanced class transition handling in clean mode to allow recoverable transitions between Folder and Script types. - Updated logging and error handling to improve clarity during the syncback process.
This commit is contained in:
parent
fe1ac1dd95
commit
2e6f909a71
5 changed files with 194 additions and 27 deletions
|
|
@ -94,13 +94,34 @@ This skipped ANY file whose parent directory was in `added_paths`, but adding a
|
|||
|
||||
**Affected tests**: `clean_removes_duplicate_extensions`, `clean_removes_multiple_duplicates`
|
||||
|
||||
### Bug 5: File Paths Added to Directory Scan (FIXED)
|
||||
|
||||
**Symptom**: Projects with `$path` pointing to single files (not directories) were failing syncback.
|
||||
|
||||
**Root cause**: The `collect_paths_from_project` function was adding all `$path` entries to the scan list, but only directories should be scanned for orphans.
|
||||
|
||||
**Fix applied**: Changed to only add `$path` entries that are directories (`resolved.is_dir()`).
|
||||
|
||||
---
|
||||
|
||||
## Test Results Summary (Current)
|
||||
## Test Results Summary (Final)
|
||||
|
||||
**Passing: 15 tests** - orphan removal, basic clean mode behavior
|
||||
**Full test suite: 179 passed, 16 failed**
|
||||
|
||||
**Failing: 11 tests** - class change scenarios, ambiguous files, meta files, combination tests
|
||||
**Clean Mode Stress Tests: 15 passed, 11 failed**
|
||||
|
||||
- Passing: All orphan removal tests, basic clean mode behavior
|
||||
- Failing: Class change scenarios (3), ambiguous files (2), meta/extension issues (3), combination tests (3)
|
||||
|
||||
**Pre-existing Failures (not from this work): 5 tests**
|
||||
|
||||
- `syncback_roundtrip::infer_service_name`
|
||||
- `syncback_roundtrip::init_meta_class_name`
|
||||
- `syncback_roundtrip::init_meta_properties`
|
||||
- `syncback_roundtrip::rbxmx_ref`
|
||||
- `syncback::ignore_trees_removing`
|
||||
|
||||
These were failing before the current changes and are separate issues.
|
||||
|
||||
---
|
||||
|
||||
|
|
|
|||
|
|
@ -28,6 +28,52 @@ use super::{
|
|||
GlobalOptions,
|
||||
};
|
||||
|
||||
/// In clean mode, ensures all $path directories exist before loading the project.
|
||||
/// This allows syncback to restore deleted directories.
|
||||
fn ensure_project_paths_exist(project_path: &Path, vfs: &Vfs) -> anyhow::Result<()> {
|
||||
// Load the project file (handles both file and directory paths)
|
||||
let project = Project::load_initial_project(vfs, project_path)
|
||||
.with_context(|| format!("Failed to load project file: {}", project_path.display()))?;
|
||||
|
||||
let base_path = project.folder_location();
|
||||
|
||||
// Recursively find all $path entries in the project tree
|
||||
fn collect_paths(node: &crate::project::ProjectNode, base: &Path, paths: &mut Vec<PathBuf>) {
|
||||
if let Some(ref path_node) = node.path {
|
||||
let full_path = if path_node.path().is_absolute() {
|
||||
path_node.path().to_path_buf()
|
||||
} else {
|
||||
base.join(path_node.path())
|
||||
};
|
||||
paths.push(full_path);
|
||||
}
|
||||
for child in node.children.values() {
|
||||
collect_paths(child, base, paths);
|
||||
}
|
||||
}
|
||||
|
||||
let mut paths_to_check = Vec::new();
|
||||
collect_paths(&project.tree, base_path, &mut paths_to_check);
|
||||
|
||||
// Create missing directories
|
||||
for path in paths_to_check {
|
||||
if !path.exists() {
|
||||
log::debug!(
|
||||
"Clean mode: creating missing $path directory: {}",
|
||||
path.display()
|
||||
);
|
||||
std::fs::create_dir_all(&path).with_context(|| {
|
||||
format!(
|
||||
"Failed to create directory for $path: {}",
|
||||
path.display()
|
||||
)
|
||||
})?;
|
||||
}
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
const UNKNOWN_INPUT_KIND_ERR: &str = "Could not detect what kind of file was inputted. \
|
||||
Expected input file to end in .rbxl, .rbxlx, .rbxm, or .rbxmx.";
|
||||
|
||||
|
|
@ -139,6 +185,12 @@ impl SyncbackCommand {
|
|||
// watcher errors shouldn't terminate the process
|
||||
let vfs = Vfs::new_oneshot();
|
||||
|
||||
// In clean mode, ensure all $path directories exist before loading
|
||||
// the project. This allows syncback to restore deleted directories.
|
||||
if !self.incremental {
|
||||
ensure_project_paths_exist(&path_old, &vfs)?;
|
||||
}
|
||||
|
||||
let project_start_timer = Instant::now();
|
||||
let session_old = ServeSession::new(vfs, path_old.clone())?;
|
||||
log::debug!(
|
||||
|
|
|
|||
|
|
@ -27,6 +27,34 @@ use crate::{
|
|||
|
||||
use super::snapshot_from_vfs;
|
||||
|
||||
/// Checks if a class transition is recoverable in clean mode.
|
||||
///
|
||||
/// Recoverable transitions:
|
||||
/// - Folder -> Script (creates init.luau)
|
||||
/// - Script -> Folder (removes init.luau)
|
||||
/// - Script -> Script (changes init.server/client.luau type)
|
||||
fn can_transition_class(old_class: &str, new_class: &str) -> bool {
|
||||
let is_script = |c: &str| matches!(c, "ModuleScript" | "Script" | "LocalScript");
|
||||
let is_folder = |c: &str| c == "Folder";
|
||||
|
||||
// Folder can become a script (by creating init file)
|
||||
if is_folder(old_class) && is_script(new_class) {
|
||||
return true;
|
||||
}
|
||||
|
||||
// Script can become Folder (by removing init file - handled by orphan removal)
|
||||
if is_script(old_class) && is_folder(new_class) {
|
||||
return true;
|
||||
}
|
||||
|
||||
// Script can change type (e.g., ModuleScript -> LocalScript)
|
||||
if is_script(old_class) && is_script(new_class) {
|
||||
return true;
|
||||
}
|
||||
|
||||
false
|
||||
}
|
||||
|
||||
pub fn snapshot_project(
|
||||
context: &InstanceContext,
|
||||
vfs: &Vfs,
|
||||
|
|
@ -344,14 +372,26 @@ pub fn syncback_project<'sync>(
|
|||
while let Some((node, old_inst, new_inst)) = node_queue.pop_front() {
|
||||
log::debug!("Processing node {}", old_inst.name());
|
||||
if old_inst.class_name() != new_inst.class {
|
||||
anyhow::bail!(
|
||||
"Cannot change the class of {} in project file {}.\n\
|
||||
Current class is {}, it is a {} in the input file.",
|
||||
old_inst.name(),
|
||||
project_path.display(),
|
||||
old_inst.class_name(),
|
||||
new_inst.class
|
||||
);
|
||||
// In clean mode, allow recoverable class transitions
|
||||
// (e.g., Folder -> ModuleScript by creating init.luau)
|
||||
let can_recover = can_transition_class(old_inst.class_name().as_str(), new_inst.class.as_str());
|
||||
if !snapshot.data.is_incremental() && can_recover {
|
||||
log::debug!(
|
||||
"Clean mode: allowing class transition {} -> {} for {}",
|
||||
old_inst.class_name(),
|
||||
new_inst.class,
|
||||
old_inst.name()
|
||||
);
|
||||
} else {
|
||||
anyhow::bail!(
|
||||
"Cannot change the class of {} in project file {}.\n\
|
||||
Current class is {}, it is a {} in the input file.",
|
||||
old_inst.name(),
|
||||
project_path.display(),
|
||||
old_inst.class_name(),
|
||||
new_inst.class
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
// TODO handle meta.json5 files in this branch. Right now, we perform
|
||||
|
|
@ -372,20 +412,65 @@ pub fn syncback_project<'sync>(
|
|||
base_path.join(node_path)
|
||||
};
|
||||
|
||||
let middleware = match Middleware::middleware_for_path(
|
||||
let mut middleware = match Middleware::middleware_for_path(
|
||||
snapshot.vfs(),
|
||||
&project.sync_rules,
|
||||
&full_path,
|
||||
)? {
|
||||
Some(middleware) => middleware,
|
||||
// The only way this can happen at this point is if the path does
|
||||
// not exist on the file system or there's no middleware for it.
|
||||
None => anyhow::bail!(
|
||||
"path does not exist or could not be turned into a file Rojo understands: {}",
|
||||
full_path.display()
|
||||
),
|
||||
None => {
|
||||
// Path doesn't exist on filesystem. In clean mode, we can
|
||||
// determine the middleware from the new instance and let
|
||||
// syncback create the necessary files/directories.
|
||||
if !snapshot.data.is_incremental() {
|
||||
// Determine middleware based on new instance class
|
||||
let inferred_middleware = match new_inst.class.as_str() {
|
||||
"ModuleScript" => Middleware::ModuleScriptDir,
|
||||
"Script" => Middleware::ServerScriptDir,
|
||||
"LocalScript" => Middleware::ClientScriptDir,
|
||||
"Folder" => Middleware::Dir,
|
||||
// For other classes, default to Dir
|
||||
_ => Middleware::Dir,
|
||||
};
|
||||
log::debug!(
|
||||
"Clean mode: path {} doesn't exist, inferring {:?} middleware for class {}",
|
||||
full_path.display(),
|
||||
inferred_middleware,
|
||||
new_inst.class
|
||||
);
|
||||
inferred_middleware
|
||||
} else {
|
||||
anyhow::bail!(
|
||||
"Rojo project referred to a file using $path that could not be turned \
|
||||
into a Roblox Instance by Rojo.\n\
|
||||
File $path: {}",
|
||||
full_path.display()
|
||||
)
|
||||
}
|
||||
}
|
||||
};
|
||||
|
||||
// In clean mode, when the filesystem has a directory (Dir middleware)
|
||||
// but the new instance is a script, we need to use the appropriate
|
||||
// script-dir middleware to create the init file and restore the script.
|
||||
if !snapshot.data.is_incremental() && middleware == Middleware::Dir {
|
||||
let script_middleware = match new_inst.class.as_str() {
|
||||
"ModuleScript" => Some(Middleware::ModuleScriptDir),
|
||||
"Script" => Some(Middleware::ServerScriptDir),
|
||||
"LocalScript" => Some(Middleware::ClientScriptDir),
|
||||
_ => None,
|
||||
};
|
||||
if let Some(new_middleware) = script_middleware {
|
||||
log::debug!(
|
||||
"Clean mode: overriding Dir->{:?} middleware for {} (class {})",
|
||||
new_middleware,
|
||||
full_path.display(),
|
||||
new_inst.class
|
||||
);
|
||||
middleware = new_middleware;
|
||||
}
|
||||
}
|
||||
|
||||
descendant_snapshots.push(
|
||||
snapshot
|
||||
.with_new_path(full_path.clone(), new_inst.referent(), Some(old_inst.id()))
|
||||
|
|
|
|||
|
|
@ -230,17 +230,20 @@ pub fn syncback_loop_with_stats(
|
|||
// not just instigating_source metadata (which may point to project file).
|
||||
let mut dirs_to_scan: Vec<PathBuf> = Vec::new();
|
||||
|
||||
// Helper to recursively collect $path entries from project tree
|
||||
// Helper to recursively collect $path DIRECTORY entries from project tree
|
||||
// Only directories need to be scanned for orphan detection; single files
|
||||
// don't contain orphans that need to be removed.
|
||||
fn collect_paths_from_project(
|
||||
node: &crate::project::ProjectNode,
|
||||
base_path: &Path,
|
||||
paths: &mut Vec<PathBuf>,
|
||||
) {
|
||||
// If this node has a $path, add it
|
||||
// If this node has a $path that points to a directory, add it
|
||||
if let Some(ref path_node) = node.path {
|
||||
let resolved = base_path.join(path_node.path());
|
||||
log::trace!("Found $path in project: {}", resolved.display());
|
||||
if resolved.exists() && !paths.contains(&resolved) {
|
||||
// Only add directories - single files don't need orphan scanning
|
||||
if resolved.is_dir() && !paths.contains(&resolved) {
|
||||
log::trace!("Found $path directory in project: {}", resolved.display());
|
||||
paths.push(resolved);
|
||||
}
|
||||
}
|
||||
|
|
@ -336,12 +339,8 @@ pub fn syncback_loop_with_stats(
|
|||
for dir in &dirs_to_scan {
|
||||
if dir.is_dir() {
|
||||
scan_directory(dir, &mut paths, &ignore_patterns, project_path);
|
||||
} else if dir.exists() {
|
||||
// It's a file, add it directly
|
||||
if is_valid_path(&ignore_patterns, project_path, dir) {
|
||||
paths.insert(dir.clone());
|
||||
}
|
||||
}
|
||||
// Note: We only add directories to dirs_to_scan now, so no else branch needed
|
||||
}
|
||||
|
||||
log::debug!("Scanned {} existing paths from filesystem", paths.len());
|
||||
|
|
|
|||
|
|
@ -30,6 +30,16 @@ pub struct SyncbackData<'sync> {
|
|||
pub(super) stats: &'sync SyncbackStats,
|
||||
}
|
||||
|
||||
impl<'sync> SyncbackData<'sync> {
|
||||
/// Returns whether syncback is running in incremental mode.
|
||||
/// In incremental mode, existing file structure is preserved.
|
||||
/// In clean mode (non-incremental), all children are treated as new.
|
||||
#[inline]
|
||||
pub fn is_incremental(&self) -> bool {
|
||||
self.incremental
|
||||
}
|
||||
}
|
||||
|
||||
pub struct SyncbackSnapshot<'sync> {
|
||||
pub data: SyncbackData<'sync>,
|
||||
pub old: Option<Ref>,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue