chore(compile): prefer denort binary in target/ directory when available #27052
Conversation
Enhances the deno compile workflow for denort development by automatically checking for a denort binary in the same directory as the deno binary, provided both are located within a target/ directory. If found, this denort binary will be used. Key points: - The DENORT_BIN environment variable remains supported for explicitly specifying a custom denort binary path. - Includes additional logic to verify if the deno binary is a symlink pointing to an executable in the target/ directory.
irbull
force-pushed
the
denort_locate
branch
from
57a4b18
to
f6197fc
Compare
cli/standalone/binary.rs
Outdated
| env::current_exe().ok().and_then(|exec_path| { | ||
| if is_in_target(&exec_path) { | ||
| get_denort(exec_path) | ||
| } else if exec_path.is_symlink() { | ||
| fs::read_link(&exec_path).ok().and_then(|target_path| { | ||
| if is_in_target(&target_path) { | ||
| get_denort(target_path) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| } else { | ||
| None |
There was a problem hiding this comment.
Do you think we should only do this for "debug" builds, or check that the path includes target/debug? Or is there value in providing this for release builds too?
There was a problem hiding this comment.
Yeah, I sometimes do it for performance related stuff. I kind of wonder if we should do this even when not in the target directory, but this will be a nice change.
There was a problem hiding this comment.
Let's keep it for target/ for now and see how it works. We could broaden this in the future to always check for denort next to the deno executable.
Can you test this on Windows? I guess the rt file is called denort there too (without a file extension).
dsherret
changed the title
cli/standalone/binary.rs
Outdated
| fs::read_link(&exec_path).ok().and_then(|target_path| { | ||
| if is_in_target(&target_path) { | ||
| get_denort(target_path) | ||
| } else { | ||
| None | ||
| } | ||
| }) |
There was a problem hiding this comment.
What's the value of the symlink check? Maybe let's remove this symlink branch to keep it simpler (less things that can go wrong)?
There was a problem hiding this comment.
The symlink check determines if the deno executable is in fact just a sym link and if so, it checks that the symbolic link points to an executable in a target/ directory. This is useful if you link your deno executable from your cargo/bin directory to your target directory.
But you're right, a lot more can go wrong including file handling on different platforms. I've removed this check.
cli/standalone/binary.rs
Outdated
| env::current_exe().ok().and_then(|exec_path| { | ||
| if is_in_target(&exec_path) { | ||
| get_denort(exec_path) | ||
| } else if exec_path.is_symlink() { | ||
| fs::read_link(&exec_path).ok().and_then(|target_path| { | ||
| if is_in_target(&target_path) { | ||
| get_denort(target_path) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| } else { | ||
| None |
There was a problem hiding this comment.
Yeah, I sometimes do it for performance related stuff. I kind of wonder if we should do this even when not in the target directory, but this will be a nice change.
Co-authored-by: David Sherret <[email protected]> Signed-off-by: Ian Bull <[email protected]>
…ble (#27052) Enhances the deno compile workflow for denort development by automatically checking for a denort binary in the same directory as the deno binary, provided both are located within a target/ directory. If found, this denort binary will be used. Key points: - The DENORT_BIN environment variable remains supported for explicitly specifying a custom denort binary path. - Includes additional logic to verify if the deno binary is a symlink pointing to an executable in the target/ directory. --------- Signed-off-by: Ian Bull <[email protected]>
Enhances the deno compile workflow for denort development by automatically checking for a denort binary in the same directory as the deno binary, provided both are located within a target/ directory. If found, this denort binary will be used.
Key points: