-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
feat(cli) - Runtime binary for Deno #8944
Conversation
This looks good, but could you split the PR so this PR only adds the |
Okay. I reverted some of the changes in Deno compile. I also move some of the code from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yos1p looks good, nice work!
That said I'm not sure if we should add a binary target to deno_runtime
, maybe instead we should add a second binary target to cli
? (I know I suggested to put it in the runtime
dir in the original issue, but I'm having doubts now). With this change concepts of standalone
binaries are split into cli
and runtime
and I believe it's preferable to have all of that code in cli
.
@ry WDYT?
Also we need to track the size of the "lite" binary in the benchmarks: https://deno.land/benchmarks#executable-size I'll be fine to add that in a separate PR though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - but can we bike shed on the name of executable a bit?
Generally we avoid dashes in filenames. "rt" is not very intelligible.
What about denolite ?
Good point, +1 for |
|
Thank you for review! Okay. I'll try to move the code and binary to be generated by cli crate.
I can do it in this PR. For the naming, I kind of prefer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good @yos1p.
When I run denort
in without an arguments I get a zero exit code (success) and no output. I think this is going to be confusing to people who don't know about this. It would be better to print an error message and have a non-zero exit code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to have a test of some sort... How will we know if this thing breaks?
I think we can use similar tests like standalone, but this can only be applied when we already implement compile with |
cli/denort.rs
Outdated
|
||
// TODO (yos1p) Specify better error message | ||
eprintln!( | ||
"{}: Runtime Error.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ry Any suggestions for the error message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yos1p this is fine for now, I will review this PR in more detail today/tomorrow and merge it. @lucacasonato and I will look into using this new binary target for standalone executables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"This executable is used internally by 'deno compile', it is not meant to be invoked directly."
@yos1p thank you for the PR, I really appreciate it, but @lucacasonato and I took a deeper look at it and we found out we needed to do a lot more changes. We've opened a new PR at #9041. Closing without merge. |
No worries. Looking forward for the new PR! |
deno_runtime
crate will produce lite binary: deno-rt in target folder (#8757)CC @bartlomieju @lucacasonato