nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.65k stars 29.62k forks source link

configure fails on linux for distributions using python 3 as default #9512

Closed shannon-nii closed 7 years ago

shannon-nii commented 8 years ago

Currently the configure script executes via "python", this works perfectly fine on systems that use python 2.x as their default python but it fails on systems that use python 3.x. Would it be possible/feasible to adjust it to execute via "python2" instead of "python"? Alternatively an updated configure script that works on 2.x and 3.x python would be good.

Python 3 error when trying to run configure: vpxclient_2016-11-08_13-37-44

Is there possibly just a missing module for python3?

silverwind commented 8 years ago

This also applies to Arch Linux on which python is actually python3. I think it'd be great if we could apply __future__ and friends to support both versions.

bnoordhuis commented 8 years ago

That won't help. configure does import gyp and gyp does not pretend to be python3-compatible. Changing the shebang to python2 breaks systems with no python2 binary. I think that includes OS X.

silverwind commented 8 years ago

Indeed, macOS has no python2, only python2.6 and python2.7. The version from homebrew does feature all the symlinks including python2, but I'm not sure we can require that.

As for gyp: There's https://bugs.chromium.org/p/gyp/issues/detail?id=36, but it's unlikely gyp will ever gain support for Python3 as it's pretty much a dead project from what I understand.

jbergstroem commented 8 years ago

There's a bunch of issues in the node-gyp repo that are similar to this. Unfortunately, the conclusion that @silverwind mentioned is likely what we can expect; upstream gyp won't get ported to python 3.

shannon-nii commented 8 years ago

manjaro is based on arch linux and it has python2 and python2.7 links This should be the same for arch linux but i'm not sure about other linux distributions. For now I run the configure script directly via python 2.x. Would it be possible to at least add a check for the python version and show an error message if python3 is detected with an advise to run the script via python2?

bnoordhuis commented 8 years ago

I don't think so. The python interpreter will throw syntax errors before the script even gets a chance to run.

The only way to work around that is to split off the main configure script into a separate script and leave only a loader stub that is legal python 2 + 3 but that seems like overcomplicating things when the python 2 prerequisite is listed in BUILDING.md.

silverwind commented 8 years ago

I think it should be possible to make the file parseable in Python 3 and print a error when python >= 3. This should work as long as Python does not evaluate import before execution.

For the except issue in the original post, we can apply this workaround.

kalrover commented 7 years ago

Can you guys help me check my first PR on node. If there is any problems, let me know.

zenhack commented 7 years ago

A couple ideas:

  1. We demand specifically python 2.7, and then do #!/usr/bin/env python2.7, or
  2. We put some logic like this at the top of the script:
#!/usr/bin/env sh

# Find a python 2 executable:
python_executable="`which python2"
if [ -z "$python_executable" ]; then
  python_executable="`which python2.7`"
fi
if [ -z "$python_executable" ]; then
  python_executable="`which python2.6`"
fi
if [ -z "$python_executable" ]; then
  printf 'Error: could not find python 2 executable\n' >&2
  exit 1
fi

# Copy the remainder of this script (which is python) to a new file and execute it with
# python instead of sh:
tmpfile="`mktemp`"
cat "$0" | tail -n +22 > "$tmpfile"
chmod +x "$tmpfile"
exec "$python_executable" "tmpfile" $@

(1) is certainly simpler.

zenhack commented 7 years ago

Αnother option is to special-case MacOS, doing something like:

case `uname` in
  darwin) python_executable="`which python2.7`" ;; # Or whatever uname returns on OS X; I don't have a live system handy.
  *) python_executable="`which python`";;
esac
silverwind commented 7 years ago

This is fixed by https://github.com/nodejs/node/pull/9657.

@zenhack generally good ideas, but I think the current solution is good enough as the user will know what to do from the error message. Spawning another python child process doesn't seem like the cleanest solution to me.

zenhack commented 7 years ago

So, this actually does't fix the problem:

$ python2 configure 
creating icu_config.gypi
* Using ICU in deps/icu-small
Using version-specific floating patch tools/icu/patches/58/source/i18n/digitlst.cpp
creating icu_config.gypi
{ 'target_defaults': { 'cflags': [],
                       'default_configuration': 'Release',
                       'defines': [],
                       'include_dirs': [],
                       'libraries': []},
  'variables': { 'asan': 0,
                 'coverage': 'false',
                 'debug_devtools': 'node',
                 'force_dynamic_crt': 0,
                 'gas_version': '2.26',
                 'host_arch': 'x64',
                 'icu_data_file': 'icudt58l.dat',
                 'icu_data_in': '../../deps/icu-small/source/data/in/icudt58l.dat',
                 'icu_endianness': 'l',
                 'icu_gyp_path': 'tools/icu/icu-generic.gyp',
                 'icu_locales': 'en,root',
                 'icu_path': 'deps/icu-small',
                 'icu_small': 'true',
                 'icu_ver_major': '58',
                 'node_byteorder': 'little',
                 'node_enable_d8': 'false',
                 'node_enable_v8_vtunejit': 'false',
                 'node_install_npm': 'true',
                 'node_module_version': 51,
                 'node_no_browser_globals': 'false',
                 'node_prefix': '/usr/local',
                 'node_release_urlbase': '',
                 'node_shared': 'false',
                 'node_shared_cares': 'false',
                 'node_shared_http_parser': 'false',
                 'node_shared_libuv': 'false',
                 'node_shared_openssl': 'false',
                 'node_shared_zlib': 'false',
                 'node_tag': '',
                 'node_use_bundled_v8': 'true',
                 'node_use_dtrace': 'false',
                 'node_use_etw': 'false',
                 'node_use_lttng': 'false',
                 'node_use_openssl': 'true',
                 'node_use_perfctr': 'false',
                 'node_use_v8_platform': 'true',
                 'openssl_fips': '',
                 'openssl_no_asm': 0,
                 'shlib_suffix': 'so.51',
                 'target_arch': 'x64',
                 'uv_parent_path': '/deps/uv/',
                 'uv_use_dtrace': 'false',
                 'v8_enable_gdbjit': 0,
                 'v8_enable_i18n_support': 1,
                 'v8_inspector': 'true',
                 'v8_no_strict_aliasing': 1,
                 'v8_optimized_debug': 0,
                 'v8_random_seed': 0,
                 'v8_use_snapshot': 'true',
                 'want_separate_host_toolset': 0,
                 'want_separate_host_toolset_mkpeephole': 0}}
creating config.gypi
creating config.mk
/bin/sh: python: command not found
gyp: Call to 'python -c "import sys; print sys.byteorder"' returned exit status 127 while in /home/isd/scratch/node/deps/v8/src/v8.gyp. while loading dependencies of /home/isd/scratch/node/node.gyp while trying to load /home/isd/scratch/node/node.gyp
Error running GYP

There are a whole bunch of other python scripts in the tree that also hard-code the name of the python executable, so they'd all have to be run with python2, and it isn't entirely straightforward to do that short of patching the source tree, since they aren't invoked directly by the user. Here's a list:

$ grep -r '^#!/usr/bin/env python$' .
./deps/cares/build/gcc_version.py:#!/usr/bin/env python
./deps/cares/gyp_cares:#!/usr/bin/env python
./deps/npm/node_modules/node-gyp/gyp/setup.py:#!/usr/bin/env python
./deps/npm/node_modules/node-gyp/gyp/pylib/gyp/MSVSSettings_test.py:#!/usr/bin/env python
./deps/npm/node_modules/node-gyp/gyp/pylib/gyp/easy_xml_test.py:#!/usr/bin/env python
./deps/npm/node_modules/node-gyp/gyp/pylib/gyp/generator/ninja_test.py:#!/usr/bin/env python
./deps/npm/node_modules/node-gyp/gyp/pylib/gyp/generator/msvs_test.py:#!/usr/bin/env python
./deps/npm/node_modules/node-gyp/gyp/pylib/gyp/generator/xcode_test.py:#!/usr/bin/env python
./deps/npm/node_modules/node-gyp/gyp/pylib/gyp/mac_tool.py:#!/usr/bin/env python
./deps/npm/node_modules/node-gyp/gyp/pylib/gyp/input_test.py:#!/usr/bin/env python
./deps/npm/node_modules/node-gyp/gyp/pylib/gyp/common_test.py:#!/usr/bin/env python
./deps/npm/node_modules/node-gyp/gyp/pylib/gyp/flock_tool.py:#!/usr/bin/env python
./deps/npm/node_modules/node-gyp/gyp/pylib/gyp/__init__.py:#!/usr/bin/env python
./deps/npm/node_modules/node-gyp/gyp/pylib/gyp/win_tool.py:#!/usr/bin/env python
./deps/npm/node_modules/node-gyp/gyp/buildbot/buildbot_run.py:#!/usr/bin/env python
./deps/npm/node_modules/node-gyp/gyp/gyp_main.py:#!/usr/bin/env python
./deps/npm/node_modules/node-gyp/gyp/tools/pretty_sln.py:#!/usr/bin/env python
./deps/npm/node_modules/node-gyp/gyp/tools/pretty_gyp.py:#!/usr/bin/env python
./deps/npm/node_modules/node-gyp/gyp/tools/graphviz.py:#!/usr/bin/env python
./deps/npm/node_modules/node-gyp/gyp/tools/pretty_vcproj.py:#!/usr/bin/env python
./deps/npm/node_modules/node-gyp/gyp/gyptest.py:#!/usr/bin/env python
./deps/v8/test/test262/list.py:#!/usr/bin/env python
./deps/v8/test/test262/archive.py:#!/usr/bin/env python
./deps/v8/tools/unittests/run_perf_test.py:#!/usr/bin/env python
./deps/v8/tools/js2c.py:#!/usr/bin/env python
./deps/v8/tools/find-commit-for-patch.py:#!/usr/bin/env python
./deps/v8/tools/stats-viewer.py:#!/usr/bin/env python
./deps/v8/tools/run-tests.py:#!/usr/bin/env python
./deps/v8/tools/grokdump.py:#!/usr/bin/env python
./deps/v8/tools/gyp_flag_compare.py:#!/usr/bin/env python
./deps/v8/tools/dump-cpp.py:#!/usr/bin/env python
./deps/v8/tools/run-deopt-fuzzer.py:#!/usr/bin/env python
./deps/v8/tools/trace-maps-processor.py:#!/usr/bin/env python
./deps/v8/tools/eval_gc_nvp.py:#!/usr/bin/env python
./deps/v8/tools/release/auto_push.py:#!/usr/bin/env python
./deps/v8/tools/release/test_scripts.py:#!/usr/bin/env python
./deps/v8/tools/release/script_test.py:#!/usr/bin/env python
./deps/v8/tools/release/mergeinfo.py:#!/usr/bin/env python
./deps/v8/tools/release/check_clusterfuzz.py:#!/usr/bin/env python
./deps/v8/tools/release/releases.py:#!/usr/bin/env python
./deps/v8/tools/release/create_release.py:#!/usr/bin/env python
./deps/v8/tools/release/auto_tag.py:#!/usr/bin/env python
./deps/v8/tools/release/search_related_commits.py:#!/usr/bin/env python
./deps/v8/tools/release/test_search_related_commits.py:#!/usr/bin/env python
./deps/v8/tools/release/merge_to_branch.py:#!/usr/bin/env python
./deps/v8/tools/release/push_to_candidates.py:#!/usr/bin/env python
./deps/v8/tools/release/common_includes.py:#!/usr/bin/env python
./deps/v8/tools/release/roll_merge.py:#!/usr/bin/env python
./deps/v8/tools/release/test_mergeinfo.py:#!/usr/bin/env python
./deps/v8/tools/release/auto_roll.py:#!/usr/bin/env python
./deps/v8/tools/release/git_recipes.py:#!/usr/bin/env python
./deps/v8/tools/android-run.py:#!/usr/bin/env python
./deps/v8/tools/mingw-generate-makefiles.sh:#!/usr/bin/env python
./deps/v8/tools/callstats.py:#!/usr/bin/env python
./deps/v8/tools/perf-to-html.py:#!/usr/bin/env python
./deps/v8/tools/run-valgrind.py:#!/usr/bin/env python
./deps/v8/tools/concatenate-files.py:#!/usr/bin/env python
./deps/v8/tools/generate_shim_headers/generate_shim_headers.py:#!/usr/bin/env python
./deps/v8/tools/try_perf.py:#!/usr/bin/env python
./deps/v8/tools/gcmole/download_gcmole_tools.py:#!/usr/bin/env python
./deps/v8/tools/gcmole/run-gcmole.py:#!/usr/bin/env python
./deps/v8/tools/gcmole/parallel.py:#!/usr/bin/env python
./deps/v8/tools/verify_source_deps.py:#!/usr/bin/env python
./deps/v8/tools/isolate_driver.py:#!/usr/bin/env python
./deps/v8/tools/ll_prof.py:#!/usr/bin/env python
./deps/v8/tools/run.py:#!/usr/bin/env python
./deps/v8/tools/process-heap-prof.py:#!/usr/bin/env python
./deps/v8/tools/test-server.py:#!/usr/bin/env python
./deps/v8/tools/dev/v8gen.py:#!/usr/bin/env python
./deps/v8/tools/presubmit.py:#!/usr/bin/env python
./deps/v8/tools/gen-postmortem-metadata.py:#!/usr/bin/env python
./deps/v8/tools/objdump-v8:#!/usr/bin/env python
./deps/v8/tools/run_perf.py:#!/usr/bin/env python
./deps/v8/tools/gc-nvp-to-csv.py:#!/usr/bin/env python
./deps/v8/tools/sanitizers/sancov_formatter.py:#!/usr/bin/env python
./deps/v8/tools/sanitizers/sancov_merger.py:#!/usr/bin/env python
./deps/v8/tools/sanitizers/sanitize_pcs.py:#!/usr/bin/env python
./deps/v8/tools/gc-nvp-trace-processor.py:#!/usr/bin/env python
./deps/v8/tools/external-reference-check.py:#!/usr/bin/env python
./deps/v8/tools/testrunner/local/testsuite_unittest.py:#!/usr/bin/env python
./deps/v8/tools/testrunner/local/statusfile_unittest.py:#!/usr/bin/env python
./deps/v8/tools/testrunner/local/pool.py:#!/usr/bin/env python
./deps/v8/tools/testrunner/local/pool_unittest.py:#!/usr/bin/env python
./deps/v8/tools/testrunner/server/daemon.py:#!/usr/bin/env python
./deps/v8/tools/disasm.py:#!/usr/bin/env python
./deps/v8/tools/jsfunfuzz/download_jsfunfuzz.py:#!/usr/bin/env python
./deps/v8/tools/generate-builtins-tests.py:#!/usr/bin/env python
./deps/v8/gypfiles/landmines.py:#!/usr/bin/env python
./deps/v8/gypfiles/detect_v8_host_arch.py:#!/usr/bin/env python
./deps/v8/gypfiles/get_landmines.py:#!/usr/bin/env python
./deps/v8/gypfiles/vs_toolchain.py:#!/usr/bin/env python
./deps/v8/gypfiles/coverage_wrapper.py:#!/usr/bin/env python
./deps/v8/gypfiles/gyp_v8:#!/usr/bin/env python
./deps/v8/gypfiles/download_gold_plugin.py:#!/usr/bin/env python
./deps/uv/gyp_uv.py:#!/usr/bin/env python
./deps/v8_inspector/third_party/markupsafe/bench/runbench.py:#!/usr/bin/env python
./deps/v8_inspector/third_party/WebKit/Source/platform/inspector_protocol/CheckProtocolCompatibility.py:#!/usr/bin/env python
./deps/v8_inspector/third_party/WebKit/Source/platform/inspector_protocol/ConcatenateProtocols.py:#!/usr/bin/env python
./deps/v8_inspector/third_party/jinja2/scripts/jinja2-debug.py:#!/usr/bin/env python
./deps/v8_inspector/third_party/jinja2/scripts/make-release.py:#!/usr/bin/env python
./deps/v8_inspector/src/inspector/build/rjsmin.py:#!/usr/bin/env python
./deps/v8_inspector/src/inspector/build/generate_protocol_externs.py:#!/usr/bin/env python
./deps/v8_inspector/src/inspector/build/compile-scripts.py:#!/usr/bin/env python
./deps/v8_inspector/src/inspector/build/check_injected_script_source.py:#!/usr/bin/env python
./deps/v8_inspector/src/inspector/PRESUBMIT.py:#!/usr/bin/env python
./test/gc/node_modules/weak/build/gyp-mac-tool:#!/usr/bin/env python
./tools/js2c.py:#!/usr/bin/env python
./tools/mkssldef.py:#!/usr/bin/env python
./tools/install.py:#!/usr/bin/env python
./tools/cpplint.py:#!/usr/bin/env python
./tools/configure.d/nodedownload.py:#!/usr/bin/env python
./tools/gyp/setup.py:#!/usr/bin/env python
./tools/gyp/pylib/gyp/MSVSSettings_test.py:#!/usr/bin/env python
./tools/gyp/pylib/gyp/easy_xml_test.py:#!/usr/bin/env python
./tools/gyp/pylib/gyp/generator/ninja_test.py:#!/usr/bin/env python
./tools/gyp/pylib/gyp/generator/msvs_test.py:#!/usr/bin/env python
./tools/gyp/pylib/gyp/generator/xcode_test.py:#!/usr/bin/env python
./tools/gyp/pylib/gyp/mac_tool.py:#!/usr/bin/env python
./tools/gyp/pylib/gyp/input_test.py:#!/usr/bin/env python
./tools/gyp/pylib/gyp/common_test.py:#!/usr/bin/env python
./tools/gyp/pylib/gyp/flock_tool.py:#!/usr/bin/env python
./tools/gyp/pylib/gyp/__init__.py:#!/usr/bin/env python
./tools/gyp/pylib/gyp/win_tool.py:#!/usr/bin/env python
./tools/gyp/buildbot/buildbot_run.py:#!/usr/bin/env python
./tools/gyp/gyp_main.py:#!/usr/bin/env python
./tools/gyp/tools/pretty_sln.py:#!/usr/bin/env python
./tools/gyp/tools/pretty_gyp.py:#!/usr/bin/env python
./tools/gyp/tools/graphviz.py:#!/usr/bin/env python
./tools/gyp/tools/pretty_vcproj.py:#!/usr/bin/env python
./tools/gyp/gyptest.py:#!/usr/bin/env python
./tools/run-valgrind.py:#!/usr/bin/env python
./tools/icu/shrink-icu-src.py:#!/usr/bin/env python
./tools/compress_json.py:#!/usr/bin/env python
./tools/gyp_node.py:#!/usr/bin/env python
./tools/test.py:#!/usr/bin/env python
./tools/genv8constants.py:#!/usr/bin/env python
./tools/check-imports.py:#!/usr/bin/env python
./tools/specialize_node_d.py:#!/usr/bin/env python
./configure:#!/usr/bin/env python

The Archlinux PKGBUILD just applies a sed command to the whole source tree:

https://git.archlinux.org/svntogit/community.git/tree/trunk/PKGBUILD?h=packages/nodejs#n27

I agree the solution I proposed isn't pretty, and I wouldn't want to apply it to many files. It also looks like a lot of these are in dependencies that node pulls in in-tree? Probably don't want to be patching those.

This probably ought to be re-opened, since the problem persists. I don't have another solution immediately popping into my head, but will think on the matter.

jbergstroem commented 7 years ago

@zenhack: only a small fraction of those files are used in build. The problem is still that python3 default installs (or python3 itself) is supported, so I think that operating systems/package managers that runs python3 as default (kudos btw) are better off handling the issue.

gibfahn commented 7 years ago

If ./configure were to create a symlink called python that pointed towards whatever was used to run it, and were to prepend the directory containing it to the path, then that would work right?

zenhack commented 7 years ago

@gibfahn, I think that would do the right thing.

Re: having systems handle the issue, I really don't think that's the right place in the stack to fix this. Arch right now has to patch a really excessive number of packages because of this kind of thing, and any other system that wants to default to python 3 will have to do the same. Doing it upstream seems much more sustainable.

zenhack commented 7 years ago

(The most elegant solution would be to write portable python, but that's a bit more work from where we are now -- though it's not hard when writing new code).

silverwind commented 7 years ago

What distribution is that that doesn't provide a python (symlink or not)?

gibfahn commented 7 years ago

@silverwind I think the problem is that if which python points to Python 3, then even if you run the main configure file with python2 configure, when subshells are spawned with the #!/usr/bin/env python, then they end up still running with python3, which doesn't work.

zenhack commented 7 years ago

Quoting Gibson Fahnestock (2016-11-30 17:34:41)

@silverwind I think the problem is that if python points to Python 3, then even if you run the main configure file with ./python2 configure, when subshells are spawned with the #!/usr/bin/env python, then they end up still running with python3, which doesn't work.

Exactly. On Archlinux, /usr/bin/python is python 3. Putting something that points to python 2 ahead of it in $PATH would do the trick.

gunar commented 7 years ago

For future reference, here's how I got it to work on Manjaro (~Arch):

mkdir /home/gcg/python2
ln -s /usr/bin/python2 /home/gcg/python2/python

Then I run tests with

PATH=/home/gcg/python2:$PATH make -j4 test

(credits go to @zenhack)

jbergstroem commented 7 years ago

@gunar: try PYTHON=/usr/bin/python2 ./configure instead.

silverwind commented 7 years ago

@jbergstroem maybe we should add this info on the error print?

https://github.com/nodejs/node/blob/76ba3bff2663176db2abaf80dd2cdc1682e92bdf/configure#L5

jbergstroem commented 7 years ago

@silverwind sounds good to me!

gunar commented 7 years ago

@jbergstroem your method did not work :-(

> PYTHON=/usr/bin/python2 ./configure
Please use either Python 2.6 or 2.7
> uname -a
Linux zeno 4.4.45-1-MANJARO #1 SMP PREEMPT Thu Jan 26 11:32:11 UTC 2017 x86_64 GNU/Linux

But thanks!

silverwind commented 7 years ago

Hmm, where's this PYTHON variable actually documented? Can't find it:

https://docs.python.org/3.7/using/cmdline.html#environment-variables

jbergstroem commented 7 years ago

Sorry; its used in Makefile.

silverwind commented 7 years ago

So we need to suggest PYTHON=path make too, right?

jbergstroem commented 7 years ago

@silverwind said: So we need to suggest PYTHON=path make too, right?

No; passing PYTHON=path to configure should write it to config.mk which gets included in Makefile. I was just providing a poor example.

silverwind commented 7 years ago

Alright, so PYTHON=/usr/bin/python2 /usr/bin/python2 configure? 😆

silverwind commented 7 years ago

Nevermind, I see 'PYTHON': sys.executable is already handed down. I'll land https://github.com/nodejs/node/issues/11375 later as-is.

Siecje commented 7 years ago

EDIT: I had a problem but when I updated to the latest 8.7.0 code it works.

gibfahn commented 7 years ago

If ./configure were to create a symlink called python that pointed towards whatever was used to run it, and were to prepend the directory containing it to the path, then that would work right?

So this was implemented in https://github.com/nodejs/node/pull/16058, so this should now finally work without having to set PYTHON, even if your python is a python3 (as long as you have a python2 on your system).