build: fix makefile script on windows by Hakerh400 · Pull Request #33136 · nodejs/node · GitHub

On Windows there is a program find.exe located in
C:\Windows\System32, which is usually in the PATH before
MSYS version of that program (required for running makefile).
The Windows version of the program uses different CLI syntax,
which results in errors like

File not found - *node_modules*

This commit specifies the full path to the program, which is also
properly handled by MSYS on Windows.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

@nodejs/build-files

CI: https://ci.nodejs.org/job/node-test-pull-request/31092/

I started a CI run. I think the problem here might be that not all systems have find(1) in /usr/bin, or it's not the flavor we want.

We should not hardcode absolute paths to commands in the Makefile.

The conventional way to allow overriding of commands in a Makefile is to use variables, e.g. for Python:

node/Makefile

Line 4 in 2cd7970

PYTHON ?= python

and then specify the variable when running make, e.g. make PYTHON=/my/path/python. You could then also get the configure script to determine values for the variables and write them to config.mk if you want to avoid specifying the variable every time:

node/configure.py

Lines 1781 to 1801 in 2cd7970

    
           # Not needed for trivial case. Useless when it's a win32 path. 
        
           if sys.executable != 'python' and ':\\' not in sys.executable: 
        
             config['PYTHON'] = sys.executable 
        
           if options.prefix: 
        
             config['PREFIX'] = options.prefix 
        
           if options.use_ninja: 
        
             config['BUILD_WITH'] = 'ninja' 
        
           config_lines = ['='.join((k,v)) for k,v in config.items()] 
        
           # Add a blank string to get a blank line at the end. 
        
           config_lines += [''] 
        
           config_str = '\n'.join(config_lines) 
        
           # On Windows there's no reason to search for a different python binary. 
        
           bin_override = None if sys.platform == 'win32' else make_bin_override() 
        
           if bin_override: 
        
             config_str = 'export PATH:=' + bin_override + ':$(PATH)\n' + config_str 
        
           write('config.mk', do_not_edit + config_str)

@richardlau PTAL

I started a CI run. I think the problem here might be that not all systems have find(1) in /usr/bin, or it's not the flavor we want.

That is no longer a problem. The absolute path is now used only on Windows, and MSYS itself hardcoded it into /usr/bin

CI: https://ci.nodejs.org/job/node-test-pull-request/31098/

CI: https://ci.nodejs.org/job/node-test-pull-request/31117/

On Windows there is a program "find.exe" located in C:\Windows\System32, which is usually in the PATH before MSYS version of that program (required for running makefile). The Windows version of the program uses different CLI syntax, which results in errors like "File not found - *node_modules*" This commit specifies the full path to the program, which is also properly handled by MSYS on Windows.

friendly ping

CI: https://ci.nodejs.org/job/node-test-pull-request/31521/

CI: https://ci.nodejs.org/job/node-test-pull-request/31531/ 💚

On Windows there is a program "find.exe" located in C:\Windows\System32, which is usually in the PATH before MSYS version of that program (required for running makefile). The Windows version of the program uses different CLI syntax, which results in errors like "File not found - *node_modules*" This commit specifies the full path to the program, which is also properly handled by MSYS on Windows. PR-URL: nodejs#33136 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Landed in 5e4c025

On Windows there is a program "find.exe" located in C:\Windows\System32, which is usually in the PATH before MSYS version of that program (required for running makefile). The Windows version of the program uses different CLI syntax, which results in errors like "File not found - *node_modules*" This commit specifies the full path to the program, which is also properly handled by MSYS on Windows. PR-URL: #33136 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Should this go back to v12.x? It'll need a manual backport if yes but feel free to remove the label if not!

On Windows there is a program "find.exe" located in C:\Windows\System32, which is usually in the PATH before MSYS version of that program (required for running makefile). The Windows version of the program uses different CLI syntax, which results in errors like "File not found - *node_modules*" This commit specifies the full path to the program, which is also properly handled by MSYS on Windows. PR-URL: #33136 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Apr 28, 2020

richardlau requested changes Apr 29, 2020

View reviewed changes

richardlau approved these changes Apr 29, 2020

View reviewed changes

BridgeAR approved these changes May 23, 2020

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label May 23, 2020

bnoordhuis approved these changes May 24, 2020

View reviewed changes

BridgeAR closed this May 25, 2020

Hakerh400 deleted the lint branch May 25, 2020 17:30

codebytere mentioned this pull request Jun 28, 2020

v14.5.0 proposal #34093

Merged

codebytere added the backport-requested-v12.x label Jul 8, 2020

codebytere mentioned this pull request Jul 13, 2020

v12.18.3 proposal #34343

Merged

addaleax removed the backport-requested-v12.x label Sep 23, 2020

Dmarch28 mentioned this pull request Jun 19, 2022

[Snyk] Fix for 1 vulnerabilities Dmarch28/node#61

Open

snyk-bot mentioned this pull request Jun 19, 2022

[Snyk] Fix for 1 vulnerabilities aliceUnhinged613/node#61

Open

137717unity mentioned this pull request Jun 19, 2022

[Snyk] Fix for 1 vulnerabilities 137717unity/node#60

Open

baby636 mentioned this pull request Jun 19, 2022

[Snyk] Fix for 1 vulnerabilities baby636/node#58

Open

snyk-bot mentioned this pull request Jun 19, 2022

[Snyk] Fix for 1 vulnerabilities Rednickle/node-1#51

Open

kissofire mentioned this pull request Jun 19, 2022

[Snyk] Fix for 1 vulnerabilities kissofire/node#70

Open

snyk-bot mentioned this pull request Jun 19, 2022

[Snyk] Fix for 1 vulnerabilities wedataintelligence/node#36

Open

ron-a20 mentioned this pull request Jun 19, 2022

[Snyk] Fix for 1 vulnerabilities ron-a20/node#69

Open

molllyn1 mentioned this pull request Jun 20, 2022

[Snyk] Fix for 1 vulnerabilities molllyn1/node#51

Open

snyk-bot mentioned this pull request Jan 30, 2023

[Snyk] Fix for 1 vulnerabilities aliceUnhinged613/node#89

Open

wedataintelligence mentioned this pull request Jan 30, 2023

[Snyk] Fix for 1 vulnerabilities wedataintelligence/node#73

Open

snyk-bot mentioned this pull request Jan 30, 2023

[Snyk] Fix for 1 vulnerabilities kissofire/node#94

Open

molllyn1 mentioned this pull request Jan 31, 2023

[Snyk] Fix for 1 vulnerabilities molllyn1/node#76

Open

snyk-bot mentioned this pull request Jan 31, 2023

[Snyk] Fix for 1 vulnerabilities 137717unity/node#93

Open

baby636 mentioned this pull request Jan 31, 2023

[Snyk] Fix for 1 vulnerabilities baby636/node#88

Open

This was referenced Jan 31, 2023

[Snyk] Fix for 1 vulnerabilities ron-a20/node#93

Open

[Snyk] Fix for 1 vulnerabilities Rednickle/node-1#70

Open

	# Not needed for trivial case. Useless when it's a win32 path.
	if sys.executable != 'python' and ':\\' not in sys.executable:
	config['PYTHON'] = sys.executable

	if options.prefix:
	config['PREFIX'] = options.prefix

	if options.use_ninja:
	config['BUILD_WITH'] = 'ninja'

	config_lines = ['='.join((k,v)) for k,v in config.items()]
	# Add a blank string to get a blank line at the end.
	config_lines += ['']
	config_str = '\n'.join(config_lines)

	# On Windows there's no reason to search for a different python binary.
	bin_override = None if sys.platform == 'win32' else make_bin_override()
	if bin_override:
	config_str = 'export PATH:=' + bin_override + ':$(PATH)\n' + config_str

	write('config.mk', do_not_edit + config_str)

build: fix makefile script on windows by Hakerh400 · Pull Request #33136 · nodejs/node · GitHub

Uh oh!

Uh oh!

build: fix makefile script on windows - #33136

build: fix makefile script on windows#33136Hakerh400 wants to merge 1 commit intonodejs:masternodejs/node:masterfrom Hakerh400:lintCopy head branch name to clipboard

Conversation

Hakerh400 commented Apr 28, 2020

Checklist

Uh oh!

addaleax commented Apr 29, 2020

Uh oh!

nodejs-github-bot commented Apr 29, 2020

Uh oh!

bnoordhuis commented Apr 29, 2020

Uh oh!

richardlau left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hakerh400 commented Apr 29, 2020

Uh oh!

Hakerh400 commented Apr 29, 2020

Uh oh!

nodejs-github-bot commented Apr 29, 2020

Uh oh!

nodejs-github-bot commented Apr 30, 2020

Uh oh!

Hakerh400 commented May 17, 2020

Uh oh!

nodejs-github-bot commented May 23, 2020

Uh oh!

nodejs-github-bot commented May 24, 2020 • edited by BridgeAR Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented May 25, 2020

Uh oh!

codebytere commented Jul 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

build: fix makefile script on windows#33136
Hakerh400 wants to merge 1 commit into
nodejs:masterfrom
Hakerh400:lint

richardlau left a comment •
edited
Loading

nodejs-github-bot commented May 24, 2020 •
edited by BridgeAR
Loading