FluidityProject / fluidity

Fluidity
http://fluidity-project.org
Other
362 stars 113 forks source link

Incorrect check for Python3 version #355

Closed tmbgreaves closed 2 years ago

tmbgreaves commented 2 years ago

In the autoconf-generated section of configure there's a problematic version comparison:

https://github.com/FluidityProject/fluidity/blob/86ccd0171513197a174653dfb1e80c086058b3d2/configure#L6141

This is fine until 3.10, at which point it fails, with "3.10" >= "3.6" returning false.

The check should probably be using parse_version from pkg_resources, ie:

parse_version("3.10") >= parse_version('3.6')

I can't immediately think of a quick and sane way to deal with this as it's not in our code, but from autoconf.

Thoughts?

tmbgreaves commented 2 years ago

This is biting me as I'm trying to get a build working using latest anaconda3, which is giving me python3 3.10

gnikit commented 2 years ago

Wouldn't something like this do the trick?

import sys
PY_MAJOR = int(sys.version_info[0])
PY_MINOR = int(sys.version_info[1])

and then compare them individually?

tmbgreaves commented 2 years ago

That sounds good - substituting in for:

https://github.com/FluidityProject/fluidity/blob/86ccd0171513197a174653dfb1e80c086058b3d2/configure.in#L212

?

stephankramer commented 2 years ago

So it seem we have our own version of AC_PYTHON_DEVEL (defined in aclocal.m4) and it does do some more sanity checks (also for having python libs to link against) - so we can't entirely get rid of it. The failing test is on line 725 of aclocal.m4 where the $1 is substituted with the argument >= '3.6' that we give it. It's a little hacky (but this is an awful mess already), but you could just hard-code the version we check for in aclocal.m4 So use what @gnikit suggested (yours is nicer but adds a dependency on packaging) and instead of using $1, you end with print(PY_MAJOR >=3 and PY_MINOR>=6) . Add some large comment and remove the argument (or pass an empty []) in the AC_PYTHON_DEVEL call so we don't think it's actually used. Sorry have to shoot off now....

Patol75 commented 2 years ago

I do not know if this is useful given that I do not fully understand what autotools does, but I had re-written/updated the Python section of configure for one of the PR I have opened, and it seemed to work. Have a look here.

angus-g commented 2 years ago

I think we should just insert the latest version of the macro into aclocal.m4: http://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=blob_plain;f=m4/ax_python_devel.m4

Patol75 commented 2 years ago

Not too different from mine! Haha