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

doc: add copy node executable guide on windows #47665

Closed
wants to merge 0 commits into from

Conversation

yjl9903
Copy link
Contributor

@yjl9903 yjl9903 commented Apr 22, 2023

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

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/single-executable

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Apr 22, 2023
@RaisinTen
Copy link
Contributor

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
Copy link
Contributor Author

yjl9903 commented Apr 22, 2023

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

@yjl9903
Copy link
Contributor Author

yjl9903 commented Apr 22, 2023

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)

@RaisinTen
Copy link
Contributor

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! 😊');

Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

LGTM

@RaisinTen RaisinTen added author ready PRs that have at least one approval, no pending requests for changes, 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
Copy link

Fyko commented Apr 23, 2023

I reckon where.exe node also gets the job done

@yjl9903
Copy link
Contributor Author

yjl9903 commented Apr 23, 2023

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)

@RaisinTen
Copy link
Contributor

@nodejs/platform-windows any thoughts on ^?

@RaisinTen
Copy link
Contributor

@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
Copy link
Contributor Author

yjl9903 commented Apr 26, 2023

@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.

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

Nice changes! Thank you for the first contribution :)

@yjl9903
Copy link
Contributor Author

yjl9903 commented Apr 28, 2023

#47722 merged to this PR

@RaisinTen
Copy link
Contributor

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

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

yjl9903 commented Apr 28, 2023

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

@RaisinTen
Copy link
Contributor

RaisinTen commented Apr 29, 2023

@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.

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 pending requests for changes, 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