Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct a flaw in the Python 3 version checking #12073

Merged
merged 1 commit into from
Jun 9, 2021

Conversation

rincebrain
Copy link
Contributor

@rincebrain rincebrain commented May 18, 2021

(Note that there may still be another problem, since this explicitly is not the same failure case that #12045 is about.)

Motivation and Context

Ran into this failure while trying to reproduce #12045 .

(This change has a caveat - it uses the "packaging" module, which is a dependency of setuptools, but this is before we check for that. So it might be reasonable to prompt for the packaging module if it's absent. The alternative is reimplementing the version comparison logic ourselves, which seems somewhat heavy.)

Description

It turns out the ax_python_devel.m4 version check assumes that ("3.X+1.0" >= "3.X.0") is True in Python, which is not the case when X+1 is 10 or above and X is not. (Also presumably X+1=100 and ...)

So let's refactor the check to succeed then.

How Has This Been Tested?

The version check that failed before on 3.10 passes now on (Ubuntu 21.04's) 3.10, and still passes on my bullseye 3.7 system.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@behlendorf
Copy link
Contributor

The ax_python_devel.m4 macro's originally come from the autoconf archive and is used by a lot of projects. Are we sure this hasn't already been fixed upstream https://www.gnu.org/software/autoconf-archive/ax_python_devel.html? Briefly looking at the source it didn't look like it, but I didn't pull the latest version and try it.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label May 19, 2021
@rincebrain
Copy link
Contributor Author

rincebrain commented May 19, 2021

Quickly looking at it before I made this PR, it hadn't been updated since 2017, and still had the same "serial" number on it. It also doesn't mention anything about this in the changelog, and uses the same comparison, which you can readily show won't work.

Python 3.7.3 (default, Jan 22 2021, 20:04:44)
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> print("3.10.0" >= "3.4.0")
False

Regrettably, I see the test bots are telling me I can't assume that package is installed, so I get to reimplement version checking logic...or I could cheat and punt the burden of doing this correctly onto everyone writing checks, because:

>>> print("3.10.0" >= "3.04.0")
True

edit: No, that doesn't save us, because then:

>>> print("3.3.0" >= "3.04.0")
True

Time to get refactorin.

...and then I remembered I already wrote this logic in m4 for the alien version check.

Just to be certain, I tried the vanilla ax_python_devel.m4, and after rewriting the call to it, it still didn't work with 3.10.0.

@rincebrain
Copy link
Contributor Author

rincebrain commented May 19, 2021

Okay, I tried handling the edge cases manually, but there are a lot of options; then I discovered setuptools vendored their own copy of packaging (and their version module) years ago.

So I reordered the Python checks to check setuptools before the version, which is slightly odd, but not as odd as getting an impenetrable error from inside the version check, and then reshuffled things to use it.

I checked Ubuntu 18.04, Debian buster, and Centos 7, and all of their setuptools have the vendored module, so it should be good.

edit: After pushing, I realized the oldest things by far are the EOLed Debian bots, so time to test and find a workaround for those, as I'm presuming this is gonna fail...

Oh, I see, Centos 7 ships an ancient version of setuptools for Python 2, compared to the version they ship for Python 3, and unlike Debian-alikes, they don't mark the packaging package as a dependency of setuptools.

@rincebrain
Copy link
Contributor Author

rincebrain commented May 19, 2021

@behlendorf If I just...add the packaging package as a dependency for building, would that get rejected? Because that really feels like the right answer to this, rather than rolling a custom solution. Debian 8 and Centos 7 definitely both package it (the latter in EPEL, but we've depended on EPEL for DKMS already, so that doesn't feel like a dealbreaker?), and so I imagine do the rest.

(My bad, wrong package, Debian doesn't package "packaging" until oldstable. How...cumbersome.)

@rincebrain
Copy link
Contributor Author

Okay, I've got a solution working - for the platforms that don't package "packaging", distlib seems to be available ~everywhere. (If you're wondering why I'm not just using distlib - "packaging" implements the same PEPs, but is more actively maintained, whereas distlib doesn't even have a changelog or documentation update about the last 6 releases.)
(Why not distutils? As Python 3.10 prints out, "The distutils package is deprecated and slated for removal in Python 3.12")

I've tested this on:

  • Centos 7 x86_64 (Python 2.7 and 3.6, packaging only) (Centos 7 packages distlib, but only for 3.6; it worked)
  • Debian jessie x86_64 (Python 2.7 and 3.4, distlib only)
  • Debian buster x86_64 (Python 2.7 and 3.7, distlib and packaging)
  • FreeBSD 13.0 x86_64 (Python 3.7, distlib and packaging)
  • Ubuntu 18.04 x86_64 (Python 2.7 and 3.6, distlib and packaging)
  • Ubuntu 21.04 x86_64 (Python 3.9 and 3.10, distlib and packaging) (Python 2.7 is technically installable but most of the modules have been dropped)

Of course, this will probably still fail on all the bots without updates, which is why I opened openzfs/zfs-buildbot#229 to go along with it, though I'll still have to re-trigger the bots even if that PR gets merged...and I think some of the bots are persistent and will fail anyway.

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

This sounds like a pretty workable solution to me. I can go ahead and merge the buildbot change but you'll also need to add the new dependency to the .github/workflows/*.yaml files as part of this PR. We'll also want to update the documentation to add this to list of required packages to build.

https://openzfs.github.io/openzfs-docs/Developer%20Resources/Custom%20Packages.html
https://openzfs.github.io/openzfs-docs/Developer%20Resources/Building%20ZFS.html

@rincebrain
Copy link
Contributor Author

Huh, I had literally never looked in the .github directory. I can take care of both of those.

@rincebrain
Copy link
Contributor Author

Okay, openzfs/openzfs-docs#166 landed with the docs update, so once openzfs/zfs-buildbot#230 lands, I'll make some meaningless change and force-push to re-trigger the checks, which I expect to pass now.

@behlendorf
Copy link
Contributor

@rincebrain sorry about the delay, I've merged the buildbot change. There's one last issue which occurred to me, you're going to also need to add the new build dependency to the %if %{with pyzfs} section of the rpm/generic/zfs.spec.in file. Looks like you'll need to add a conditional for the BuildRequires: line to make sure you get the right dependency for CentOS 7 or 8.

@jwk404
Copy link
Contributor

jwk404 commented Jun 3, 2021

Are the changes requested here still required?

@rincebrain
Copy link
Contributor Author

I would assume so; I must have completely overlooked that comment, sorry, I'll go take care of it...

@rincebrain
Copy link
Contributor Author

There.

(I'm reasonably confident that the two failures are unrelated...)

@behlendorf
Copy link
Contributor

The kernel build failure is a little odd. Can you rebase this on the latest master, which hopefully should sort it out. Thanks for making the spec file fix.

It turns out the ax_python_devel.m4 version check assumes that
("3.X+1.0" >= "3.X.0") is True in Python, which is not when X+1
is 10 or above and X is not. (Also presumably X+1=100 and ...)

So let's remake the check to behave consistently, using the
"packaging" or (if absent) the "distlib" modules.

(Also, update the Github workflows to use the new packages.)

Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
@rincebrain
Copy link
Contributor Author

Sure, let's see how this goes (though if the kernel build fails again, I'm going to conclude something exciting is going on upstream, and go open an issue and look at it...)

@jwk404 jwk404 merged commit 08cd071 into openzfs:master Jun 9, 2021
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 9, 2021
It turns out the ax_python_devel.m4 version check assumes that
("3.X+1.0" >= "3.X.0") is True in Python, which is not when X+1
is 10 or above and X is not. (Also presumably X+1=100 and ...)

So let's remake the check to behave consistently, using the
"packaging" or (if absent) the "distlib" modules.

(Also, update the Github workflows to use the new packages.)

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: John Kennedy <john.kennedy@delphix.com>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes: openzfs#12073
@thesamesam
Copy link
Contributor

thesamesam commented Sep 10, 2021

FWIW, there's a patch upstream here: autoconf-archive/autoconf-archive#235. We've applied it in Gentoo and Fedora have as well.

@rincebrain
Copy link
Contributor Author

FWIW, there's a patch upstream here: autoconf-archive/autoconf-archive#235. We've applied it in Gentoo and Fedora have as well.

I don't think that patch corrects the flaw that e.g. "3.9.0" >= "3.10.0" is True in there, though I admit I have not grabbed the new version and run it, just looked at the patch and double-checked that:

>>> print("3.3" >= "3.4")
False
>>> print("3.10" >= "3.4")
False
>>> print("3.9" >= "3.10")
True
>>> print("3.9.0" >= "3.10")
True
>>> print("3.9.0" >= "3.10.0")
True
>>> print("3.9" >= "3.10.0")
True

rincebrain added a commit to rincebrain/zfs that referenced this pull request Oct 11, 2021
It turns out the ax_python_devel.m4 version check assumes that
("3.X+1.0" >= "3.X.0") is True in Python, which is not when X+1
is 10 or above and X is not. (Also presumably X+1=100 and ...)

So let's remake the check to behave consistently, using the
"packaging" or (if absent) the "distlib" modules.

(Also, update the Github workflows to use the new packages.)

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: John Kennedy <john.kennedy@delphix.com>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes: openzfs#12073
rincebrain added a commit to rincebrain/zfs that referenced this pull request Oct 11, 2021
It turns out the ax_python_devel.m4 version check assumes that
("3.X+1.0" >= "3.X.0") is True in Python, which is not when X+1
is 10 or above and X is not. (Also presumably X+1=100 and ...)

So let's remake the check to behave consistently, using the
"packaging" or (if absent) the "distlib" modules.

(Also, update the Github workflows to use the new packages.)

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: John Kennedy <john.kennedy@delphix.com>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes: openzfs#12073
tonyhutter pushed a commit that referenced this pull request Nov 1, 2021
It turns out the ax_python_devel.m4 version check assumes that
("3.X+1.0" >= "3.X.0") is True in Python, which is not when X+1
is 10 or above and X is not. (Also presumably X+1=100 and ...)

So let's remake the check to behave consistently, using the
"packaging" or (if absent) the "distlib" modules.

(Also, update the Github workflows to use the new packages.)

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: John Kennedy <john.kennedy@delphix.com>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes: #12073
tonyhutter pushed a commit that referenced this pull request Nov 1, 2021
It turns out the ax_python_devel.m4 version check assumes that
("3.X+1.0" >= "3.X.0") is True in Python, which is not when X+1
is 10 or above and X is not. (Also presumably X+1=100 and ...)

So let's remake the check to behave consistently, using the
"packaging" or (if absent) the "distlib" modules.

(Also, update the Github workflows to use the new packages.)

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: John Kennedy <john.kennedy@delphix.com>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes: #12073
ghost pushed a commit to truenas/zfs that referenced this pull request Nov 3, 2021
It turns out the ax_python_devel.m4 version check assumes that
("3.X+1.0" >= "3.X.0") is True in Python, which is not when X+1
is 10 or above and X is not. (Also presumably X+1=100 and ...)

So let's remake the check to behave consistently, using the
"packaging" or (if absent) the "distlib" modules.

(Also, update the Github workflows to use the new packages.)

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: John Kennedy <john.kennedy@delphix.com>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes: openzfs#12073
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Nov 13, 2021
It turns out the ax_python_devel.m4 version check assumes that
("3.X+1.0" >= "3.X.0") is True in Python, which is not when X+1
is 10 or above and X is not. (Also presumably X+1=100 and ...)

So let's remake the check to behave consistently, using the
"packaging" or (if absent) the "distlib" modules.

(Also, update the Github workflows to use the new packages.)

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: John Kennedy <john.kennedy@delphix.com>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Closes: openzfs#12073
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Code Review Needed Ready for review and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants