Perform sanity checks with Python3 #25

yillkid · 2018-08-18T14:24:40Z

Switch test suites invocation to Python3.

jserv · 2018-08-18T14:39:30Z

README.md

@@ -10,6 +10,7 @@ Most use cases for micropayments involve a single user or device interacting
 repeatedly with a few vendors.

 ## Prerequisites
+`Iota-swarm-node` is compatible with Python 3.5.


The wording is misleading. Instead, python 3.5+ is required as PyOTA configures. You have to emphasize on the rationale in plain English descriptions, otherwise nobody can be aware of it.

jserv · 2018-08-18T14:41:43Z

mk/python.mk

@@ -0,0 +1,12 @@
+PYTHON = python3
+PYTHON := $(shell which $(PYTHON))
+ifndef PYTHON


Add extra condition if python indicated by $PATH is exactly of version 3. Think of the case that somebody built Python3 from source and the generated executable is python.

jserv · 2018-08-18T14:42:03Z

mk/python.mk

+PYTHON = python3
+PYTHON := $(shell which $(PYTHON))
+ifndef PYTHON
+$(error "python3 is required.")


Since Python 3.5+ is required, you have to validate its version.

jserv · 2018-08-18T14:42:41Z

mk/python.mk

+PY_CHECK_MOD_IOTA := $(shell $(PYTHON) -c "import iota" 2>/dev/null && \
+                       echo 1 || echo 0)
+ifneq ("$(PY_CHECK_MOD_IOTA)","1")
+    $(error "skip $@ because PyIOTA is not installed.")


It is PyOTA or iota.lib.py.

The error message is wrong because it does not "skip". Instead, we would take actions if both Python3 and PyOTA are absent. You shall dump informative diagnosis messages.

jserv · 2018-08-18T14:50:02Z

mk/python.mk

+PY_CHECK_MOD_IOTA := $(shell $(PYTHON) -c "import iota" 2>/dev/null && \
+                       echo 1 || echo 0)
+ifneq ("$(PY_CHECK_MOD_IOTA)","1")
+    $(error "skip $@ because PyOTA is not installed.")


Inform users of the diagnosis and information about trouble shooting. Change the wording from "skip" to something really making sense.

jserv · 2018-08-18T14:54:10Z

TODO: Validate Python installation from the perspectives of the following:

python3 package: Don't assume Debian/Ubuntu packages are available. Instead, you have to be aware of the case that somebody always build python3 from source (and you should try as well).
The version of python3: only 3.5 or later are known to work.
The availability of PyOTA: you have to mention and alert users if no package is installed.

jserv

Improve Python version validation.

jserv · 2018-08-20T08:45:50Z

mk/python.mk

+PY_CHECK_MOD_IOTA := $(shell $(PYTHON) -c "import iota" 2>/dev/null && \
+                       echo 1 || echo 0)
+ifneq ("$(PY_CHECK_MOD_IOTA)","1")
+    $(error "dependency error $@ because PyOTA is not installed, to install the latest version: pip install pyota")


Display error messages in multiple lines.

jserv · 2018-08-20T11:22:34Z

mk/python.mk

@@ -0,0 +1,12 @@
+PYTHON = python3.5


The detection is not correct. If python is built from source, only file python is generated. You shall validate through the output of python --version.

The coverage about Python 3.7 is not addressed yet.

You shall always try to build Python from source and check how its directory structure is.

jserv · 2018-08-20T11:23:23Z

README.md

@@ -10,6 +10,7 @@ Most use cases for micropayments involve a single user or device interacting
 repeatedly with a few vendors.

 ## Prerequisites
+`iota-swarm-node`, writing with Python3+ compatible code as well as depending on [Pyota](https://github.com/iotaledger/iota.lib.py) is theoretically available for bot Python 3.5 and 3.6; yet, our testing result indicates that the environment for Python 3.6 is invalid in this case. Details as [Pyota issue#203](https://github.com/iotaledger/iota.lib.py/issues/203).


Fix the grammar again. It is not "writing with".

You might have to consider virtualenv or pythonbrew.

"bot Python 3.5 and 3.6" ?

The explanation should be enforced to validation logic.

jserv · 2018-08-20T11:29:38Z

mk/python.mk

+PY_CHECK_MOD_IOTA := $(shell $(PYTHON) -c "import iota" 2>/dev/null && \
+                       echo 1 || echo 0)
+ifneq ("$(PY_CHECK_MOD_IOTA)","1")
+    $(error "dependency error $@ because PyOTA is not installed, to install the latest version: pip3 install pyota")


If Python executable is restricted, the validation of pip would be considered.

jserv · 2018-08-22T03:39:50Z

README.md

@@ -10,7 +10,7 @@ Most use cases for micropayments involve a single user or device interacting
 repeatedly with a few vendors.

 ## Prerequisites
-`iota-swarm-node`, writing with Python3+ compatible code as well as depending on [Pyota](https://github.com/iotaledger/iota.lib.py) is theoretically available for bot Python 3.5 and 3.6; yet, our testing result indicates that the environment for Python 3.6 is invalid in this case. Details as [Pyota issue#203](https://github.com/iotaledger/iota.lib.py/issues/203).
+`iota-swarm-node`, compatible with Python3+ as well as depending on [Pyota](https://github.com/iotaledger/iota.lib.py) is theoretically available for both Python 3.5 and 3.6; yet, our testing result indicates that the environment for Python 3.6 is invalid in this case. Details as [Pyota issue#203](https://github.com/iotaledger/iota.lib.py/issues/203).


It is PyOTA instead of Pyota.

jserv · 2018-08-22T03:47:27Z

mk/python.mk

+    $(error "Python is not executable, try to check the PATH environment variable or install $(PYTHON_COMPATIBLE) package.")
+endif # PYTHON
+
+ifdef PYTHON


You don't have to check $(PYTHON) since you already exclude its absence.

jserv · 2018-08-22T03:48:46Z

mk/python.mk

+
+    ifdef PYTHON_COMPATIBLE_PATH
+        $(error "Found $(PYTHON_COMPATIBLE) in current environment, but it should be setting as \
+            default interpreter, try to create a symbolic link for $(PYTHON_COMPATIBLE_PATH)")


Don't suggest to make symbolic link. Instead, configure via environment variables.

jserv · 2018-08-22T03:49:07Z

mk/python.mk

 endif
+endif # PYTHON
+
+ifndef PYTHON


Ditto. Focus on detailed items.

jserv · 2018-08-22T03:49:53Z

mk/python.mk

 PY_CHECK_MOD_IOTA := $(shell $(PYTHON) -c "import iota" 2>/dev/null && \
                       echo 1 || echo 0)
 ifneq ("$(PY_CHECK_MOD_IOTA)","1")
    $(error "dependency error $@ because PyOTA is not installed, to install the latest version: pip3 install pyota")
 endif
+endif # PYTHON


Avoid nested ifdef and endif pair as possible as you can.

jserv · 2018-08-22T03:51:22Z

mk/python.mk

+        $(error "Found $(PYTHON_COMPATIBLE) in current environment, but it should be setting as \
+            default interpreter, try to create a symbolic link for $(PYTHON_COMPATIBLE_PATH)")
+    else
+        $(error "$(shell python -V):Invalid Python version, only available for $(PYTHON_COMPATIBLE).")


It is NOT Invalid Python version. Instead, only Python 3.5 is known to work with PyOTA. You have to address.

jserv · 2018-08-22T03:52:13Z

mk/python.mk

-PYTHON := $(shell which $(PYTHON))
+# check Python version
+PYTHON := $(shell which "python")
+PYTHON_COMPATIBLE = python3.5


Rename it to PYTHON35 to reflect its functionality.

jserv · 2018-08-22T03:53:29Z

mk/python.mk

+    PY_CHECK_VERSION := $(shell python -c "print(__import__('sys').version_info[0:2])")
+
+ifneq ($(PY_CHECK_VERSION), (3, 5))
+    PYTHON_COMPATIBLE_PATH := $(shell which $(PYTHON_COMPATIBLE))


Don't rely on new var PYTHON_COMPATIBLE_PATH. Instead, override PYTHON since you always block the one which is not accepted by each qualification.

jserv · 2018-08-22T05:33:08Z

README.md

@@ -10,6 +10,7 @@ Most use cases for micropayments involve a single user or device interacting
 repeatedly with a few vendors.

 ## Prerequisites
+`iota-swarm-node`, compatible with Python3+ as well as depending on [Pyota](https://github.com/iotaledger/iota.lib.py) is theoretically available for both Python 3.5 and 3.6; yet, our testing result indicates that the environment for Python 3.6 is invalid in this case. Details as [Pyota issue#203](https://github.com/iotaledger/iota.lib.py/issues/203).


Correct the wording: "invalid"

Remove "theoretically". Instead, it would be "expectedly" or something alike.

Replace Pyota issue#203 with PyOTA issue #203. Be aware of the spelling.

jserv · 2018-08-22T05:33:44Z

mk/python.mk

+    str('.') + str(__import__('sys').version_info.minor) )")
+
+ifneq ($(PY_CHECK_VERSION), $(PYTHON_COMPATIBLE_VERSION))
+


Avoid this blank line.

jserv · 2018-08-22T05:35:06Z

mk/python.mk

+PYTHON := $(shell which "python")
+PYTHON_COMPATIBLE_VERSION = 3.5
+
+ifndef PYTHON


If python is not present, try python3. Then, if version is mismatching, try python3.5.

jserv · 2018-08-22T05:36:09Z

mk/python.mk

+        $(error "Found python$(PYTHON_COMPATIBLE_VERSION) in current environment, but it should be setting as \
+            default interpreter, try to set $(PYTHON_COMPATIBLE_PATH) in PATH environment variable.")
+    else
+        $(error "$(shell python -V):Unsupported python version, only available for Python$(PYTHON_COMPATIBLE_VERSION).")


Replace python with $(PYTHON).

yillkid · 2018-08-23T06:42:18Z

Use macros in GNU macro to simplify detection flow.

Could you explain more about that?

yillkid · 2018-08-23T08:15:45Z

Can you handle the case that both python3.5 and python3.6 are available in $PATH?

The $(PYTHON) assign priority is python --> python3 --> python(favored version), if both python3.5, python3.6 are available in $PATH and symbolic link python is linked with python3.5, it will work. If not, an error message Found python$(PY_FAVORED_VER), but it should be set as default interpreter, try to set $(PYTHON) in PATH environment variable. will be trigged.

jserv · 2018-08-24T10:06:59Z

README.md

@@ -10,6 +10,7 @@ Most use cases for micropayments involve a single user or device interacting
 repeatedly with a few vendors.

 ## Prerequisites
+`iota-swarm-node`, compatible with Python3+ as well as depending on [Pyota](https://github.com/iotaledger/iota.lib.py) is theoretically available for both Python 3.5 and 3.6; yet, our testing result indicates that the environment for Python 3.6 is invalid in this case. Details as [Pyota issue#203](https://github.com/iotaledger/iota.lib.py/issues/203).


Remove "as well".

Rewrite the abobe wording to

"This package depends in PyOTA, which is known to work with Python 3.5. However, before PyOTA issue #203 is resolved, Python 3.6/3.7 will be excluded for running this package. The build system would detect and validate the environment in advance."

jserv

Reduce the indentation. We shall always make detection script slight and elegant.

jserv · 2018-08-24T10:11:30Z

mk/python.mk

+PY_FAVORED_VER = 3.5
+PYTHON := $(shell which "python")
+ifndef PYTHON
+    PYTHON := $(shell which "python3")


Don't indent here.

jserv · 2018-08-24T10:11:37Z

mk/python.mk

+endif
+
+ifndef PYTHON
+    PYTHON := $(shell which "python$(PY_FAVORED_VER)")


jserv · 2018-08-24T10:11:43Z

mk/python.mk

+endif
+
+ifndef PYTHON
+    $(error "Python is not executable, try to check the PATH environment variable or \


jserv · 2018-08-24T10:12:03Z

mk/python.mk

+PY_CHECK_VERSION := $(shell $(PYTHON) -V)
+
+ifeq "$(filter $(PY_FAVORED_VER).%, $(PY_CHECK_VERSION))" ""
+    PYTHON := $(shell which "python"$(PY_FAVORED_VER))


Reduce the indentation for pretty code snip.

jserv · 2018-08-24T10:12:12Z

mk/python.mk

+PY_CHECK_MOD_IOTA := $(shell python -c "import iota" 2>/dev/null && \
+                       echo 1 || echo 0)
+ifneq ("$(PY_CHECK_MOD_IOTA)","1")
+    PIP := $(shell pip -V)


jserv · 2018-08-24T10:12:55Z

mk/python.mk

+ifndef PIP_CHECK_VERSION
+    PIP_FAVORED_VER = 3
+endif
+    $(error "PyOTA package doesn't pre-configured, install the latest version: pip$(PIP_FAVORED_VER) install pyota")


jserv · 2018-08-24T10:17:19Z

Check dcurl for illustrating macro usage with GNU make: https://github.com/DLTcollab/dcurl/blob/dev/Makefile#L32

jserv · 2018-08-24T10:21:01Z

Once macros are appropriately facilitated, no more nested conditional syntax (such as ifdef and endif) will be taken, that implies compacted and dedicated script, thinking perspective of maintenance.

jserv · 2018-08-27T09:56:32Z

README.md

@@ -10,7 +10,7 @@ Most use cases for micropayments involve a single user or device interacting
 repeatedly with a few vendors.

 ## Prerequisites
-`iota-swarm-node`, compatible with Python3+ as well as depending on [Pyota](https://github.com/iotaledger/iota.lib.py) is theoretically available for both Python 3.5 and 3.6; yet, our testing result indicates that the environment for Python 3.6 is invalid in this case. Details as [Pyota issue#203](https://github.com/iotaledger/iota.lib.py/issues/203).
+This package depends in PyOTA, which is known to work with Python 3.5. However, before PyOTA issue [#203](https://github.com/iotaledger/iota.lib.py/issues/203) is resolved, Python 3.6/3.7 will be excluded for running this package. The build system would detect and validate the environment in advance.


depends "on", not "in".

jserv · 2018-08-27T09:58:02Z

mk/python.mk

-endif
+PYTHON := $(shell which "python"$(PY_FAVORED_VER))
+ifdef PYTHON
+$(error "Found python$(PY_FAVORED_VER), but it should be set as \


Do you think it is valid according to the error message?

jserv · 2018-08-27T09:59:57Z

mk/python.mk

+PIP_CHECK_VERSION := $(findstring python 3, $(PIP) && echo $@ || echo 0)
+endif
+
+ifneq ("$(PIP)", "1")


Use ifdef rather than ifneq or ifeq for consistency.

jserv · 2018-08-27T10:00:16Z

mk/python.mk

+endif
+
+ifneq ("$(PIP)", "1")
+$(error "PIP package doesn't pre-configured, try to install it.")


Fix grammar error.

jserv · 2018-08-27T10:01:02Z

mk/python.mk

-ifndef PIP_CHECK_VERSION
-    PIP_FAVORED_VER = 3
+ifeq ("$(PY_CHECK_MOD_IOTA)", "0")
+$(error "PyOTA package doesn't pre-configured, install the latest version: pip$(PIP_FAVORED_VER) install pyota")


Ditto. Fix grammar.

Using pip is not the only way to install Python packages. Sometimes, package maintainer might prepare prebuilt one or recipes that describe how to install Python packages. You can leave the message: "please ask your administrator to install the package 'pyota' or build from source: https://github.com/iotaledger/iota.lib.py"

jserv · 2018-08-27T10:08:18Z

mk/python.mk

 endif

 ifndef PYTHON
-    $(error "Python is not executable, try to check the PATH environment variable or \
-        install python$(PY_FAVORED_VER) package.")
+$(error "Python is not executable, try to check the PATH environment variable or \


It is ambiguous to say "Python is not executable". Use precise words.

jserv · 2018-08-27T10:09:20Z

mk/python.mk

+ifndef PYTHON
+PYTHON := $(shell which "python3")
+endif
+


No blank line here.

jserv · 2018-08-27T10:10:00Z

mk/python.mk

+ifndef PYTHON
+$(error "Python is not executable, try to check the PATH environment variable or \
+install python$(PY_FAVORED_VER) package.")
+endif # $(shell which "python")


Remove non-effective "$(shell which "python")".

Let's say, "no valid Python interpreter is found".

jserv · 2018-08-27T10:11:29Z

mk/python.mk

+$(error "Found python$(PY_FAVORED_VER), but it should be set as \
+default interpreter, try to set $(PYTHON) in PATH environment variable.")
+else
+$(error "$(shell python -V):Unsupported python version, only available for Python$(PY_FAVORED_VER).")


Explain the exact reason and point unresolved issue(s) here.

Validate ifdef-else statement. Logic error.

jserv · 2018-08-27T10:12:11Z

mk/python.mk

+else
+$(error "$(shell python -V):Unsupported python version, only available for Python$(PY_FAVORED_VER).")
+endif # PYTHON
+endif # "$(filter ...


Ditto. You don't have to mention "$(filter ..." once you remove nested conditions.

jserv · 2018-10-31T06:51:20Z

@yillkid, you should create a new branch as workspace for commit "Store transaction before broadcast
". Otherwise, this pull request would be polluted.

wusyong · 2018-11-17T05:56:43Z

README.md

@@ -10,6 +10,7 @@ Most use cases for micropayments involve a single user or device interacting
 repeatedly with a few vendors.

 ## Prerequisites
+This package depends on PyOTA, which is known to work with Python 3.5. However, before PyOTA issue [#203](https://github.com/iotaledger/iota.lib.py/issues/203) is resolved, Python 3.6/3.7 will be excluded for running this package. The build system would detect and validate the environment in advance.


PyOTA doesn't support Python 3.7 because it has package Filters which doesn't either.

Add Python3.5 prerequisites in README

d6ec40b

jserv reviewed Aug 18, 2018

View reviewed changes

jserv changed the title ~~Perform sanity checks on Python3~~ Perform sanity checks with Python3 Aug 18, 2018

Perform sanity checks on Python3

f837ad5

yillkid force-pushed the dev branch from 991795b to f837ad5 Compare August 18, 2018 14:47

jserv reviewed Aug 18, 2018

View reviewed changes

Revise known to work version of Python in README

5e75e2e

yillkid force-pushed the dev branch from 73b6509 to 5e75e2e Compare August 20, 2018 08:07

Complement the error message when no Pyota installed

6692705

yillkid force-pushed the dev branch from b9778f0 to 6692705 Compare August 20, 2018 09:34

jserv suggested changes Aug 20, 2018

View reviewed changes

jserv reviewed Aug 20, 2018

View reviewed changes

Revise prerequisites in README

365e066

jserv reviewed Aug 22, 2018

View reviewed changes

Add Python executeable and compatible in sanity checks

cb5ad42

jserv reviewed Aug 22, 2018

View reviewed changes

mk/python.mk

endif

endif # PYTHON

ifndef PYTHON

Copy link

Collaborator

jserv Aug 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto. Focus on detailed items.

jserv reviewed Aug 22, 2018

View reviewed changes

Refractory sanity checks

0b1bbf8

jserv reviewed Aug 22, 2018

View reviewed changes

Add sanity checks rules

91f89af

Add pip version detect rule

81ffc18

jserv reviewed Aug 24, 2018

View reviewed changes

jserv closed this Aug 24, 2018

jserv reopened this Aug 24, 2018

Refractory sanity checks

98eccb6

jserv reviewed Aug 27, 2018

View reviewed changes

mk/python.mk

endif

ifneq ("$(PIP)", "1")

$(error "PIP package doesn't pre-configured, try to install it.")

Copy link

Collaborator

jserv Aug 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix grammar error.

Fix typo

e74d2ee

jserv reviewed Aug 27, 2018

View reviewed changes

mk/python.mk

ifndef PYTHON

PYTHON := $(shell which "python3")

endif

Copy link

Collaborator

jserv Aug 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blank line here.

jserv reviewed Aug 27, 2018

View reviewed changes

Store transaction before broadcast

81a82b4

wusyong reviewed Nov 17, 2018

View reviewed changes

		str('.') + str(__import__('sys').version_info.minor) )")

		ifneq ($(PY_CHECK_VERSION), $(PYTHON_COMPATIBLE_VERSION))

Perform sanity checks with Python3 #25

Are you sure you want to change the base?

Perform sanity checks with Python3 #25

Conversation

yillkid commented Aug 18, 2018 • edited Loading

Choose a reason for hiding this comment

jserv Aug 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jserv commented Aug 18, 2018

jserv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yillkid commented Aug 23, 2018 • edited Loading

yillkid commented Aug 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jserv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jserv commented Aug 24, 2018

jserv commented Aug 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jserv commented Oct 31, 2018

Choose a reason for hiding this comment

yillkid commented Aug 18, 2018 •

edited

Loading

jserv Aug 18, 2018 •

edited

Loading

yillkid commented Aug 23, 2018 •

edited

Loading

jserv commented Aug 24, 2018 •

edited

Loading