Improve error message when python 3 not available - Fix for #1385 #1392

lkarthee · 2020-04-22T01:36:17Z

In this PR, I have made python optional in CMakeLists.txt . I have tested locally by bumping the required version to 3.8 which is not existing on my Mac.

I have noticed that the below tests are run from python file - test/run-c-api-examples.py.

c_api_example(callback)
c_api_example(finalize)
c_api_example(global)
c_api_example(hello)
c_api_example(hostref)
c_api_example(multi)
c_api_example(memory)
c_api_example(reflect)
c_api_example(serialize)
c_api_example(start)
c_api_example(table)
c_api_example(trap)

Do we need to port run-c-api-examples.py to gtest file or cpp file?

WebAssembly#1385

sbc100 · 2020-04-22T02:50:09Z

Can't you already do this by building with -DBUILD_TESTS=OFF?

wabt's primary test runner ./test/run-tests.py is also written in python. If you don't have python the only possible tests you could run are the gtest unittests and i'm not sure it worth the extra option to allow just those tests to build built.

sbc100 · 2020-04-22T02:52:00Z

Also, if you do want to run tests, isn't it easy install python3 on mac?

lkarthee · 2020-04-22T05:33:31Z

Installing python3 is easy. I use pyenv to manage python versions. I wanted to contribute to this project, so I picked up #1385 with good-first-bug tag.

To test this PR I used a python version 3.8 which is not installed in my MacBook as I don't have Windows as mentioned in #1385.

All 5 CI checks have python3 enabled and run python tests - they won't test the scenario where python3 is not installed.

sbc100 · 2020-04-22T23:16:56Z

Would you mind instead improving the error message, e.g. "python3 is required to for wabt testing, please either install it or build with -DBUILD_TESTS=OFF".

lkarthee · 2020-04-23T00:28:41Z

I will modify the PR accordingly.

lkarthee · 2020-04-23T04:06:46Z

I have made required changes. See sample error message (ignore exact versions like 3.7.3 and 3.8 in the error message).

-- Could NOT find PythonInterp: Found unsuitable version "3.7.3", but required is at least "3.8" (found /usr/bin/python3)
CMake Error at CMakeLists.txt:587 (message):
  Python 3 is required for wabt testing.  Please install it.

  If you want to proceed without installing python 3, either add -no-tests to
  make target (example: make clang-debug-no-tests) or run cmake with
  -DBUILD_TESTS=OFF.


-- Configuring incomplete, errors occurred!

sbc100 · 2020-04-23T04:12:04Z

CMakeLists.txt

-    endif ()
+    else()
+      message(FATAL_ERROR "Python 3 is required for wabt testing. Please install it. \nIf you want to proceed without installing python 3, either add -no-tests to make target (example: make clang-debug-no-tests) or run cmake with -DBUILD_TESTS=OFF.")
+    endif(PYTHONINTERP_FOUND)


Seem like you could just put this error in an if an "if (NOT PYTHONINTERP_FOUND) .. endif()" above that the rest of the file would be left unchanged?

aardappel · 2020-04-23T18:37:37Z

Would be better if instead it displayed that warning and then turned tests off for you, rather than failing.
Tests are mostly for contributors to WABT, and should not concern people simply trying to build the binaries.

binji · 2020-04-23T19:13:17Z

I tend to agree with @aardappel here. @sbc100, does that seem OK to you?

sbc100 · 2020-04-23T20:06:22Z

Fine with me. We can set BUILD_TESTS to false is python is not found.

sbc100 · 2020-04-23T20:09:28Z

Or should we even just have BUILD_TESTS default to OFF?

binji · 2020-04-24T00:02:52Z

I like having it default to on, personally.

lkarthee · 2020-04-24T11:57:13Z

Should I revert the initial commit of this PR?

BUILD_TESTS default is ON (No change in existing code)
skip tests when python3 >= 3.5.

sbc100 · 2020-04-24T16:51:07Z

I think the conclusion is:

BUILD_TESTS defaults to ON
BUILD_TESTS gets forced to OFF if python is now found

lkarthee · 2020-04-24T17:35:11Z

skip tests when python3 >= 3.5.

Got it - Oops typo, it should have been < sign (in my previous comment).

sbc100 · 2020-04-26T01:59:38Z

CMakeLists.txt

@@ -464,6 +464,13 @@ if (NOT EMSCRIPTEN)
    )
  endif ()

+  # Python 3.5 is the version shipped in Ubuntu Xenial
+  find_package(PythonInterp 3.5)
+  if(NOT PYTHONINTERP_FOUND)


how about AND BUILD_TESTS == ON there?

Otherwise lgtm!

added this check.

lkarthee · 2020-04-27T01:51:29Z

Cmake prints a message when suitable version is not found. This will appear irrespective of BUILD_TEST is ON or OFF.

When BUILD_TESTS=OFF and python3 not found

.......
-- Check size of size_t - done
-- Could NOT find PythonInterp: Found unsuitable version "3.7.3", but required is at least "3.8" (found /usr/bin/python3)
-- Looking for pthread.h
.......

When BUILD_TESTS=ON and python3 not found

.......
-- Check size of size_t - done
-- Could NOT find PythonInterp: Found unsuitable version "3.7.3", but required is at least "3.8" (found /usr/bin/python3)
CMake Warning at CMakeLists.txt:471 (message):
  Skipping tests.  Python 3 is required for wabt testing.  Please install
  python3 to run tests.


-- Looking for pthread.h
.......

binji

lgtm, thanks for doing this!

making python optional for build.

152aa10

WebAssembly#1385

lkarthee mentioned this pull request Apr 22, 2020

Don't require Python in build (if not found, skip those targets) #1385

Closed

lkarthee added 2 commits April 23, 2020 07:45

Show error message about skipping tests if python3 is not available

f8263f0

Show error message about skipping tests if python3 is not available

c38edf6

lkarthee changed the title ~~making python optional for build - Fix for #1385~~ Improve error message when python 3 not available - Fix for #1385 Apr 23, 2020

Show error message about skipping tests if python3 is not available

4953dad

sbc100 reviewed Apr 23, 2020

View reviewed changes

Show error message about skipping tests if python3 is not available

9774135

Skipping tests when Python 3.5 or above is not found.

2a578b0

sbc100 reviewed Apr 26, 2020

View reviewed changes

Skipping tests when Python 3.5 or above is not found.

aa9a275

binji approved these changes Apr 27, 2020

View reviewed changes

binji merged commit 068f604 into WebAssembly:master Apr 27, 2020

lkarthee deleted the python_check_optional branch May 6, 2020 01:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error message when python 3 not available - Fix for #1385 #1392

Improve error message when python 3 not available - Fix for #1385 #1392

lkarthee commented Apr 22, 2020

sbc100 commented Apr 22, 2020

sbc100 commented Apr 22, 2020

lkarthee commented Apr 22, 2020

sbc100 commented Apr 22, 2020

lkarthee commented Apr 23, 2020

lkarthee commented Apr 23, 2020

sbc100 Apr 23, 2020

lkarthee Apr 23, 2020

aardappel commented Apr 23, 2020

binji commented Apr 23, 2020

sbc100 commented Apr 23, 2020 •

edited

sbc100 commented Apr 23, 2020

binji commented Apr 24, 2020

lkarthee commented Apr 24, 2020

sbc100 commented Apr 24, 2020

lkarthee commented Apr 24, 2020 •

edited

sbc100 Apr 26, 2020

lkarthee Apr 27, 2020

lkarthee commented Apr 27, 2020

binji left a comment

Improve error message when python 3 not available - Fix for #1385 #1392

Improve error message when python 3 not available - Fix for #1385 #1392

Conversation

lkarthee commented Apr 22, 2020

sbc100 commented Apr 22, 2020

sbc100 commented Apr 22, 2020

lkarthee commented Apr 22, 2020

sbc100 commented Apr 22, 2020

lkarthee commented Apr 23, 2020

lkarthee commented Apr 23, 2020

sbc100 Apr 23, 2020

Choose a reason for hiding this comment

lkarthee Apr 23, 2020

Choose a reason for hiding this comment

aardappel commented Apr 23, 2020

binji commented Apr 23, 2020

sbc100 commented Apr 23, 2020 • edited

sbc100 commented Apr 23, 2020

binji commented Apr 24, 2020

lkarthee commented Apr 24, 2020

sbc100 commented Apr 24, 2020

lkarthee commented Apr 24, 2020 • edited

sbc100 Apr 26, 2020

Choose a reason for hiding this comment

lkarthee Apr 27, 2020

Choose a reason for hiding this comment

lkarthee commented Apr 27, 2020

binji left a comment

Choose a reason for hiding this comment

sbc100 commented Apr 23, 2020 •

edited

lkarthee commented Apr 24, 2020 •

edited