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: fix copy node executable in Windows #48624

Merged

Conversation

yoavain
Copy link
Contributor

@yoavain yoavain commented Jul 1, 2023

Bug description

The Windows where command looks for a pattern in the %Path% and lists all matches.
The first match is the one that gets executed when called.
The problem with the current code is that if where finds more than one match, the for loop will copy each of them.
The result will be having the last match instead of the first one.

I encountered this when I tried to create a single executable application via GitHub Actions.
I was using a simple action configuration file:

name: Node CI

jobs:
  build:

    runs-on: windows-latest

    strategy:
      matrix:
        node-version: [20.x]

    steps:
    - name: Setup NodeJS
      uses: actions/setup-node@v3.6.0
      with:
        node-version: ${{ matrix.node-version }}
        cache: npm

But still, after copying the node executable using

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

Log showed:

1 file(s) copied.
1 file(s) copied.

And running

hello.exe -v

Got:

v18.16.0

Fix description

node -e "require('fs').copyFileSync(process.execPath, 'hello.exe')"

Explanation:

  1. process.execPath gives the absolute pathname of the executable that started the Node.js process
  2. node -e evaluates the content inside the quotations
  3. require('fs') loads the fs module
  4. copyFileSync does the file copy

This solution should be valid for any other OS.
However, tested it on Windows only.

Windows where command lists all places it finds a pattern in Path.
The first one is the one that executes when called.
So the old code was overriding the first executable by any other match.
@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 Jul 1, 2023
Found a much easier solution for copying the node executable using native node.
Can probably be consolidated to all OSs.
@RaisinTen
Copy link
Contributor

Nice catch, thanks! Might make the docs simpler if we used the same command for both powershell and cmd. We could use the same thing for non-Windows too if we remove the .exe part.

@yoavain
Copy link
Contributor Author

yoavain commented Jul 3, 2023

Right, the same command can apply to both powershell and cmd.
Regarding non-windows, I can't decide what is a more "elegant" command...
WDYT?

@RaisinTen
Copy link
Contributor

I'm personally okay with both. Does anyone else have a preference? If not, I think it's fine to only update the Windows commands and unify both the cmd and powershell parts into the one you suggested.

Merged command for both PowerShell and Command Prompt
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. labels Jul 4, 2023
@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 6, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 6, 2023
@nodejs-github-bot nodejs-github-bot merged commit d9438cc into nodejs:main Jul 6, 2023
23 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in d9438cc

juanarbol pushed a commit that referenced this pull request Jul 13, 2023
Windows where command lists all places it finds a pattern in Path.
The first one is the one that executes when called.
So the old code was overriding the first executable by any other match.

PR-URL: #48624
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@juanarbol juanarbol mentioned this pull request Jul 13, 2023
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Windows where command lists all places it finds a pattern in Path.
The first one is the one that executes when called.
So the old code was overriding the first executable by any other match.

PR-URL: nodejs#48624
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Windows where command lists all places it finds a pattern in Path.
The first one is the one that executes when called.
So the old code was overriding the first executable by any other match.

PR-URL: nodejs#48624
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@ruyadorno
Copy link
Member

This commit does not land cleanly on v18.x-staging and will need manual backport in case we want it in v18.

@ruyadorno ruyadorno added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Sep 11, 2023
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. backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants