Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(compile): analyze modules in directory specified in --include #27296

Merged
merged 18 commits into from
Dec 12, 2024

Conversation

dsherret
Copy link
Member

@dsherret dsherret commented Dec 9, 2024

Working on this surfaced some bugs in the VFS implementation. I ended up changing the file system implementation to determine its root directory as the last step of building it instead of being the first step which makes it much more reliable.

Closes #26941
Closes #27023 (makes it obsolete)

}
VfsFileSubDataKind::ModuleGraph => {
virtual_file.module_graph_offset = offset;
virtual_file.module_graph_offset = offset_and_len;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test case I added surfaced a bug where we weren't storing the length of module graph only files meaning a file could be corrupted.

@dsherret
Copy link
Member Author

dsherret commented Dec 9, 2024

Not sure why windows is failing. Going to work on outputting the contents of the vfs and that might help.

@dsherret dsherret marked this pull request as draft December 10, 2024 00:27
@dsherret dsherret removed the request for review from bartlomieju December 10, 2024 19:35
@dsherret dsherret added the ci-draft Run the CI on draft PRs. label Dec 11, 2024
@dsherret dsherret marked this pull request as ready for review December 12, 2024 00:54
@dsherret dsherret removed the ci-draft Run the CI on draft PRs. label Dec 12, 2024
@@ -1,5 +1,4 @@
Compile file:///[WILDCARD]/node_modules_symlink_outside/main.ts to [WILDCARD]
Warning Symlink target is outside '[WILDCARD]node_modules_symlink_outside'. Inlining symlink at '[WILDCARD]node_modules_symlink_outside[WILDCARD]node_modules[WILDCARD]test.txt' to '[WILDCARD]target.txt' as file.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The root of the VFS is determined after everything is inside it, so this is now represented properly inside it.

@@ -3,8 +3,13 @@ Download http://localhost:4260/@denotest/esm-basic/1.0.0.tgz
Initialize @denotest/esm-basic@1.0.0
Check file:///[WILDCARD]/node_modules_symlink_outside/main.ts
Compile file:///[WILDCARD]/node_modules_symlink_outside/main.ts to [WILDLINE]
Warning Symlink target is outside '[WILDLINE]node_modules_symlink_outside'. Excluding symlink at '[WILDLINE]node_modules_symlink_outside[WILDLINE]node_modules[WILDLINE]symlink_dir' with target '[WILDLINE]some_folder'.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. The root of the VFS is determined after everything is inside it, so this is now represented properly inside it.

.chars()
.take(40)
.map(|c| c.len_utf8())
.sum::<usize>();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug in the wildcard code.

@@ -66,6 +66,9 @@
"tempDir": {
"type": "boolean"
},
"symlinkedTempDir": {
"type": "boolean"
},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already existed, but was missing in this schema.json file.

@@ -1,3 +1,2 @@
Deno.mkdirSync("data");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't do anything. Must have been leftover from something else.

.iter(),
),
);
log::debug!("Binary root dir: {}", root_dir_url);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now figure out the root after populating the VFS. This makes it much more reliable.

/// The root of the system above any drive letters.
WindowSystemRoot,
Path(PathBuf),
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows having a VFS that spans drive letters on windows.

@dsherret dsherret requested a review from bartlomieju December 12, 2024 01:29
cli/standalone/binary.rs Show resolved Hide resolved
Comment on lines 914 to 918
panic!(
"Unhandled scenario where a duplicate entry was found: {:?}",
existing
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be an error instead of a panic?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to panic instead of error because this would be a bug in Deno.

@dsherret dsherret enabled auto-merge (squash) December 12, 2024 14:50
@dsherret dsherret merged commit 4cfa340 into denoland:main Dec 12, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants