FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

doc: add copy node executable guide on windows by yjl9903 · Pull Request #47665 · nodejs/node · GitHub

/ node Public

doc: add copy node executable guide on windows - #47665

Closed
yjl9903 wants to merge 0 commit into
nodejs:mainfrom
yjl9903:patch-1
Closed

doc: add copy node executable guide on windows#47665
yjl9903 wants to merge 0 commit into
nodejs:mainfrom
yjl9903:patch-1

Conversation

yjl9903 commented Apr 22, 2023
edited
Loading

Copy link
Copy Markdown
Contributor

Hello.

In single-executable, copyied node executable on windows should be named with extension .exe, this PR added some guides about it.

Related discussion nodejs/single-executable#65

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/single-executable

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Apr 22, 2023

Copy link
Copy Markdown
Member

Thanks for the PR!

We're using the hello name without the extension for the Windows commands in a couple more places in the documentation. Would be nice if we could update those too -

Also, do you mind using doc instead of docs for the first commit? The commit validator in CI would fail otherwise and this would require a manual landing then. It's fine if you don't wanna do it, I can handle it while landing. :)

yjl9903 commented Apr 22, 2023

Copy link
Copy Markdown
Contributor Author

The step reference of copying node executable may be wrong, and I also fix it in 00454f5

yjl9903 commented Apr 22, 2023

Copy link
Copy Markdown
Contributor Author

Another question is in some windows with powershell, the default encoding is UTF-16. That is to say in the step 1 and 2, the encoding of echoed hello.js and sea-config.json is UTF-16. I am not sure whether it will cause some unintended errors. (I have no environment to test it now)

Copy link
Copy Markdown
Member

I think UTF-16 too should be okay because we have a test that uses a UTF-16 character in the SEA -

console.log('Hello, world! 😊');

RaisinTen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

RaisinTen added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. single-executable Issues and PRs related to single-executable applications labels Apr 22, 2023

Fyko commented Apr 23, 2023

Copy link
Copy Markdown

I reckon where.exe node also gets the job done

yjl9903 commented Apr 23, 2023
edited
Loading

Copy link
Copy Markdown
Contributor Author

I reckon where.exe node also gets the job done

It works in cmd.

I also searched equivalent of Bash's $(), and write the following command to copy node, but I am not sure whether there is a better or eleganter command in windows cmd.

for /F "tokens=*" %n IN ('where.exe node') DO @(copy "%n" hello.exe)

Copy link
Copy Markdown
Member

@nodejs/platform-windows any thoughts on ^?

Copy link
Copy Markdown
Member

@yjl9903 FWIW, if this works on cmd, I'm in favor of using this command for the Windows docs because then we'll have a command that would work both on cmd as well as powershell on Windows. If someone else knows of a more elegant way of doing this, maybe that could be done in a separate PR later?

yjl9903 commented Apr 26, 2023

Copy link
Copy Markdown
Contributor Author

@yjl9903 FWIW, if this works on cmd, I'm in favor of using this command for the Windows docs because then we'll have a command that would work both on cmd as well as powershell on Windows. If someone else knows of a more elegant way of doing this, maybe that could be done in a separate PR later?

Done.

ovflowd left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Nice changes! Thank you for the first contribution :)

yjl9903 commented Apr 28, 2023

Copy link
Copy Markdown
Contributor Author

#47722 merged to this PR

Copy link
Copy Markdown
Member

@yjl9903 unfortunately, our tooling doesn't support merge commits yet, so you might need to go for a rebase instead

yjl9903 force-pushed the patch-1 branch 2 times, most recently from e1178b8 to e2f9442 Compare April 28, 2023 18:29
yjl9903 closed this Apr 28, 2023

yjl9903 commented Apr 28, 2023

Copy link
Copy Markdown
Contributor Author

Oh, I think I am not very familiar with how to do rebasing... How to reopen this PR @RaisinTen ?

RaisinTen commented Apr 29, 2023
edited
Loading

Copy link
Copy Markdown
Member

@yjl9903 could you try checking if https://gist.github.com/robertpainsi/2c42c15f1ce6dab03a0675348edd4e2c helps? So IIUC, you would need to run:

git push -f origin b31d587dc8c1b0f459243d2b96a17a7175c996cf:patch-1
# Reopen the PR.
git push -f origin 45c4698fa56d28579009de9d78a1f41b372689ec:patch-1

Alternatively, you could also try to click on "Create pull request" in main...yjl9903:node:patch-1, which would help you create a new pull request with the current list of commits that are present on your branch.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. doc Issues and PRs related to the documentations. single-executable Issues and PRs related to single-executable applications

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants


Back | FazBrowse Home | New Git URL