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.

Sign up for GitHub

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

Jump to bottom

fix(compile): do not error embedding same symlink via multiple methods #27015

Merged
merged 1 commit into from
Nov 25, 2024
+32 −2
Merged

fix(compile): do not error embedding same symlink via multiple methods #27015

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cli/standalone/virtual_fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ impl VfsBuilder {
let dir = self.add_dir(path.parent().unwrap())?;
let name = path.file_name().unwrap().to_string_lossy();
match dir.entries.binary_search_by(|e| e.name().cmp(&name)) {
Ok(_) => unreachable!(),
Ok(_) => Ok(()), // previously inserted
Copy link
Contributor

Choose a reason for hiding this comment

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

In the case of a --include in deno compile, if the same sym link is inserted twice (which can happen if a parent and child directory are both included), which path will it be found under? I guess with this code change, it will only be found under the first path, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

If you insert a parent and then insert the child, the child would overwrite the child within the parent (we could be smarter about knowing if a parent was previously inserted to skip the work, but I think it's not worth it)

Copy link
Contributor

@irbull irbull Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For complex virtual file systems, you may actually want both entries. I hit this problem when trying to add a node_modules directory to a deno compile (probably not a good idea anyways). But I think that is a much bigger problem than what needs to be solved here. Thanks for the fix.

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'm not sure what you mean be including both entries. Wouldn't there only be one symlink?

Copy link
Contributor

Choose a reason for hiding this comment

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

Given a file structure like this:

.
├── assets
│   └── my_static_asset.txt
├── folder
│   ├── assets -> ../assets
│   └── sub_folder
│       └── assets -> ../../assets
└── print_asset.ts

and deno compile --include folder/ --include folder/sub_folder/ print_asset.ts

Would I expect:
I could read the my_static_assets under:
import.meta.dirname + "/folder/sub_folder/assets/my_static_asset.txt"
import.meta.dirname + "/folder/assets/my_static_asset.txt"

The same symlink is found twice when that directory is recursed. I'm not sure if that works anyways.

Copy link
Contributor

@irbull irbull Nov 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I pulled down this PR and gave it a spin.

  1. It does fix the Panic 🚀
  2. With the structure above, it compiles but the symbolic links are not resolved if --include folder/ is provided. That is, given the structure above and:
deno compile --include folder/ print_asset.ts

Then the following fails to resolve:

const content = await Deno.readTextFile(import.meta.dirname + "/folder/assets/my_static_asset.txt");

But this works fine if:

deno compile --include . print_asset.ts

is used. However, this wasn't working before this change-set, so I would consider that separate issue.

That's a long way of saying 👍 by me, and I can file an another issue for the more complex layout if you'd like.

Copy link
Member Author

@dsherret dsherret Nov 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In this case, you need to also include the assets directory, otherwise only the symlinks are included in the folder directory.

> $env:DENORT_BIN="V:\deno\target\debug\denort.exe" ; V:/deno/target/debug/deno compile --include folder --include folder/sub_folder print_asset.ts && ./print_asset.exe                 
Compile file:///V:/scratch/print_asset.ts to print_asset.exe
error: Uncaught (in promise) NotFound: path not found: readfile '.../folder/sub_folder/assets/my_static_asset.txt': path not found: readfile 'C:\Users\david\AppData\Local\Temp\deno-compile-print_asset.exe\scratch/folder/sub_folder/assets/my_static_asset.txt': path not found
    at Object.readTextFileSync (ext:deno_fs/30_fs.js:765:10)
    at file:///C:/Users/david/AppData/Local/Temp/deno-compile-print_asset.exe/scratch/print_asset.ts:1:18
> $env:DENORT_BIN="V:\deno\target\debug\denort.exe" ; V:/deno/target/debug/deno compile --include assets --include folder --include folder/sub_folder print_asset.ts && ./print_asset.exe
Compile file:///V:/scratch/print_asset.ts to print_asset.exe
console.log(1);

console.log(1);

Copy link
Contributor

Choose a reason for hiding this comment

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

+1. Yep, that fixes things. I'm not sure if it's obvious that the targets of the sym links need to be included, but that's probably a good thing in the end.

I wrote a small tool that prints the contents of the import.meta.dirname and with this change-set applied things are much better. 👍 from me, but maybe Bartek can give this a final review. Thanks again for the fix.

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 opened these issues for the future:

Copy link
Contributor

Choose a reason for hiding this comment

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

I found another unrelated issue while testing this (this fails before this change-set is applied, so it shouldn't hold up this work). I filed an issue: #27024

Err(insert_index) => {
dir.entries.insert(
insert_index,
Expand All @@ -314,9 +314,9 @@ impl VfsBuilder {
.collect::<Vec<_>>(),
}),
);
Ok(())
}
}
Ok(())
}

pub fn into_dir_and_files(self) -> (VirtualDirectory, Vec<Vec<u8>>) {
Expand Down
27 changes: 27 additions & 0 deletions tests/specs/compile/include/symlink_twice/__test__.jsonc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
{
"tempDir": true,
"steps": [{
"args": "run -A setup.js",
"output": "[WILDCARD]"
}, {
"if": "unix",
"args": "compile --allow-read=data --include . --output main link.js",
"output": "[WILDCARD]"
}, {
"if": "unix",
"commandName": "./main",
"args": [],
"output": "1\n",
"exitCode": 0
}, {
"if": "windows",
"args": "compile --allow-read=data --include . --output main.exe link.js",
"output": "[WILDCARD]"
}, {
"if": "windows",
"commandName": "./main.exe",
"args": [],
"output": "1\n",
"exitCode": 0
}]
}
3 changes: 3 additions & 0 deletions tests/specs/compile/include/symlink_twice/setup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Deno.mkdirSync("data");
Deno.writeTextFileSync("index.js", "console.log(1);");
Deno.symlinkSync("index.js", "link.js");