Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.3k
build: fix makefile script on windows#33136
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
addaleax commented Apr 29, 2020
@nodejs/build-files |
nodejs-github-bot commented Apr 29, 2020
bnoordhuis commented Apr 29, 2020
I started a CI run. I think the problem here might be that not all systems have find(1) in |
richardlau left a comment • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
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.
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:
Line 4 in 2cd7970
| PYTHON ?= python |
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: Lines 1781 to 1801 in 2cd7970
| # Not needed for trivial case. Useless when it's a win32 path. | |
| ifsys.executable!='python'and':\\'notinsys.executable: | |
| config['PYTHON'] =sys.executable | |
| ifoptions.prefix: | |
| config['PREFIX'] =options.prefix | |
| ifoptions.use_ninja: | |
| config['BUILD_WITH'] ='ninja' | |
| config_lines= ['='.join((k,v)) fork,vinconfig.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=Noneifsys.platform=='win32'elsemake_bin_override() | |
| ifbin_override: | |
| config_str='export PATH:='+bin_override+':$(PATH)\n'+config_str | |
| write('config.mk', do_not_edit+config_str) |
Hakerh400 commented Apr 29, 2020
@richardlau PTAL |
Hakerh400 commented Apr 29, 2020
That is no longer a problem. The absolute path is now used only on Windows, and MSYS itself hardcoded it into |
nodejs-github-bot commented Apr 29, 2020
nodejs-github-bot commented Apr 30, 2020
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.
Hakerh400 commented May 17, 2020
friendly ping |
nodejs-github-bot commented May 23, 2020
nodejs-github-bot commented May 24, 2020 • edited by BridgeAR
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by BridgeAR
Uh oh!
There was an error while loading. Please reload this page.
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 <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
BridgeAR commented May 25, 2020
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 <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
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 <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
codebytere commented Jul 8, 2020
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 <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
On Windows there is a program
find.exelocated inC:\Windows\System32, which is usually in the PATH beforeMSYS version of that program (required for running makefile).
The Windows version of the program uses different CLI syntax,
which results in errors like
This commit specifies the full path to the program, which is also
properly handled by MSYS on Windows.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes