-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
build: fix makefile script on windows #33136
Conversation
@nodejs/build-files |
I started a CI run. I think the problem here might be that not all systems have find(1) in |
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. | |
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 |
That is no longer a problem. The absolute path is now used only on Windows, and MSYS itself hardcoded it into |
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 |
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>
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>
On Windows there is a program
find.exe
located 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