DM-6368: Allow Python 3 and do not assume SCons python is relevant #2
Conversation
timj
force-pushed
the
tickets/DM-6368
branch
2 times, most recently
from
076e6d2
to
8a120ec
Compare
| echo "check-python: Python v2.7 not found (version reported by '$PYTHON' is $PYTHON_VERSION)." 2>&1 | ||
| MINVER=2.7 | ||
| MINVER3=3.4 | ||
| if [ $(bc <<< "$PYTHON_VERSION < $MINVER || ($PYTHON_VERSION > 3.0 && $PYTHON_VERSION < $MINVER3)") -eq 1 ]; then |
There was a problem hiding this comment.
I had to really think about this one. Is this copied from stack exchange or some place else that can be linked?
There was a problem hiding this comment.
@PaulPrice had the same comment. bc seems to be the sanest way to do floating point comparisons. I did find an example somewhere when searching for "bash floating point comparisons" but I have no idea where that page is now. I'll add a comment stating that it's for floats.
There was a problem hiding this comment.
It's just a really old python interface that can't change since it's related to python versions. See: https://docs.python.org/2/library/sys.html#sys.version_info and https://docs.python.org/3/library/sys.html#sys.version_info
There was a problem hiding this comment.
The weird part to me is the script is bash instead of python.
There was a problem hiding this comment.
I thought @jhoblitt was talking about the bc usage not the sys.version_info usage.
There was a problem hiding this comment.
There's a lot of historical evolution of this script. The numpy and matplotlib version check scripts are Python. This one never has been, presumably because it's meant to be checking that python exists at all and then checking it's the right version. If you write it in python you have to also handle the difference in behavior when python is not there.
There was a problem hiding this comment.
Yeah that is resolved if you just make it a python script and use the tuple. But bc works... and there are a lot of C developers on the team. I'd never use it but if this is acceptable to the rest of DM then I can follow it.
Uses bc command for floating point comparison. Checks that minimum python3 version is 3.4 and minimum python2 version is 2.7.
We no longer know that the Python running SCons is the same as the Python that we want to use when building and running the code. This means the cfg file can no longer use sysconfig directly but must ask the PATH python for that information.
|
One high level comment that is addressed in this PR but I want to mention none the less. It's important to know when the code is breaking here. Especially for the unfortunate DM developer that isn't in the know and has a weird configuration. Specifically I have concerns with the Scons code which is likely to be reused in many places with arbitrary Scons configurations. All of that being said it works and there is no easy workaround. So it looks good. |
|
I've completely rewritten the version test. Please take another look. @jmatt are you talking about the python.cfg file crashing with bad python data? |
|
Excellent! I have no problem following that. |
Using shell scripts was becoming complicated given that testing for 3.x and 2.x independently and having to support 3.10 being newer than 3.9. Rewriting the logic in Python itself makes the testing more obvious.
No description provided.