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

build: fix makefile script on windows #33136

Closed
wants to merge 1 commit into from
Closed

build: fix makefile script on windows #33136

wants to merge 1 commit into from

Conversation

Hakerh400
Copy link
Contributor

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-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Apr 28, 2020
@addaleax
Copy link
Member

@nodejs/build-files

@nodejs-github-bot
Copy link
Collaborator

@bnoordhuis
Copy link
Member

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.

Copy link
Member

@richardlau richardlau left a 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:

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)

@Hakerh400
Copy link
Contributor Author

@richardlau PTAL

@Hakerh400
Copy link
Contributor Author

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

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

friendly ping

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 23, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 24, 2020

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 25, 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.

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>
@BridgeAR
Copy link
Member

Landed in 5e4c025

@BridgeAR BridgeAR closed this May 25, 2020
@Hakerh400 Hakerh400 deleted the lint branch May 25, 2020 17:30
codebytere pushed a commit that referenced this pull request Jun 18, 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.

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>
@codebytere codebytere mentioned this pull request Jun 28, 2020
codebytere pushed a commit that referenced this pull request Jun 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.

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>
@codebytere
Copy link
Member

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

codebytere pushed a commit that referenced this pull request Jul 8, 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.

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>
@codebytere codebytere mentioned this pull request Jul 13, 2020
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. build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants