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

removal of LegacyVersion broke ax_python_dev.m4 #14297

Merged
merged 1 commit into from
Jan 5, 2023

Conversation

ahrens
Copy link
Member

@ahrens ahrens commented Dec 16, 2022

Motivation and Context

The 22.0 release of the python packaging package removed the LegacyVersion trait, causing ZFS to no longer compile: pypa/packaging#407

Description

This commit replaces the sections of ax_python_dev.m4 that rely on LegacyVersion with updated implementations from the upstream autoconf-archive.

I saw that we had made some changes to this file, so I didn't replace it entirely. But I also didn't totally understand our changes, so I'm hoping someone can confirm that this is the right approach.

How Has This Been Tested?

Since github updated their Ubuntu 20.04 environment yesterday, build fails in this environment. With this PR, it succeeds.

OpenZFS uses github's ubuntu 22.04 actions runner, and is therefore currently unaffected. However, I suspect it's only a matter of time before github's 22.04 environment is updated to use the 22.0 release of the python packaging package, at which point it will fail to compile.

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:

@rincebrain
Copy link
Contributor

rincebrain commented Dec 17, 2022

The reason we have changes that aren't in upstream, IIRC, is that upstream's version comparison tools, last I checked, are just wrong.

In particular, checking if 3.10.X >= 3.9.X would return false, at the time.

If upstream no longer has that bug, we could just take theirs verbatim. But that was the problem that originally motivated local changes, to the best of my recollection.

(I'll sit down and review it later tonight, but just wanted to add that for context.)

e: Oh, yes, there was that, and also the glue for handling both packaging and distlib, I believe.

EOD`
ac_supports_python_ver=`$PYTHON -c "import sys; \
ver = sys.version.split ()[[0]]; \
print (ver >= '2.1.0')"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this logic has the same flaw this was patched to fix, but because the comparison is specifically with 2.1.0, it doesn't come up.

e.g.

>>> a
'3.9.2'
>>> b
'2.1.0'
>>> c
'3.10.0'
>>> a > c
True
>>> c > b
True

They're assuming string comparison of versions works like humans would intuitively do the comparison, and that's just not true.

I believe that I specifically used LegacyVersion because there was some reason the non-Legacy one had problems, like it didn't handle things like pre or rc suffixes on versions or something to that effect.

Oh, right, because Version doesn't handle anything that doesn't pass PEP 440, was why. I thought I recalled there being some case in the CI or my own testbeds that got bitten by that, or maybe some distro naming that broke it, and that was why.

I'm not sure what we'd like to do, tbh - the most "correct" solution would be to use non-Legacy version, or to fallback to non-Legacy if it's absent, but if we don't have a concrete example of why it broke, maybe we don't care to add the additional complication.

(I also remembered why I didn't upstream this - it adds an external dependency, whereas their solution "only" breaks when the string width is inconsistent between versions and has no such dependency.)

def __ge__(self, s):
return self.vpy >= self.vtup(s)
EOF
ac_supports_python_ver=`$PYTHON -c "import ax_python_devel_vpy; \
Copy link
Contributor

@rincebrain rincebrain Dec 17, 2022

Choose a reason for hiding this comment

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

Ah, yes, you could do this, but I think this is still wrong because e.g. "pre3" means "before 0" for comparisons.

Dunno if we care about that edge case versus just going with this, though. I'd really like a note above the first comparison warning people not to just try increasing it without using this more complex logic, as I could easily see us deciding to change it to compare to 3.5 and being burned...

edit: ah, but pre isn't a thing Python usually uses. a1, b1, or dev1 are, though...

Copy link
Contributor

@rincebrain rincebrain left a comment

Choose a reason for hiding this comment

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

I was going to say I don't really mind the edge cases I pointed out maybe breaking, but the more I think about it, the more I would expect a small flood of bug reports from people on rolling distros which run nightly things for fun.

Of course, in favor of not cluttering things, we could "just" add an override --ignore-python-version-check and let people play with fire if they run unstable distros, then ship unmodified upstream again.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Dec 19, 2022
@siris
Copy link

siris commented Dec 20, 2022

@ahrens @rincebrain FYI we have hit this issue in Funtoo Linux. We are tracking this in Funtoo Bug https://bugs.funtoo.org/browse/FL-10856
Also, I have taken @ahrens changes to config/ax_python_devel.m4 as of today and patched it against zfs-2.1.7 source code plus have verified a full configure+install works fine on a Funtoo next install using Python 3.7.16 and 3.9.16

@behlendorf
Copy link
Contributor

Of course, in favor of not cluttering things, we could "just" add an override --ignore-python-version-check and let people play with fire if they run unstable distros, then ship unmodified upstream again.

Given the ongoing trouble we've had with detecting the version this strikes me as the best approach. It handles what should be the common case and provides an option for those doing something custom. I'm inclined to start by pulling in the unmodified upstream source and then following it up with a commit to add the override.

The 22.0 release of the python `packaging` package removed the
`LegacyVersion` trait, causing ZFS to no longer compile.

This commit replaces the sections of `ax_python_dev.m4` that rely on
`LegacyVersion` with updated implementations from the upstream
`autoconf-archive`.

Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
@behlendorf behlendorf merged commit b72efb7 into openzfs:master Jan 5, 2023
@behlendorf
Copy link
Contributor

Merged to resolve the outstanding build issue. It can be refined as needed.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jan 5, 2023
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Jan 13, 2023
The 22.0 release of the python `packaging` package removed the
`LegacyVersion` trait, causing ZFS to no longer compile.

This commit replaces the sections of `ax_python_dev.m4` that rely on
`LegacyVersion` with updated implementations from the upstream
`autoconf-archive`.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
Closes openzfs#14297
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Jan 18, 2023
The 22.0 release of the python `packaging` package removed the
`LegacyVersion` trait, causing ZFS to no longer compile.

This commit replaces the sections of `ax_python_dev.m4` that rely on
`LegacyVersion` with updated implementations from the upstream
`autoconf-archive`.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
Closes openzfs#14297
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Jan 19, 2023
The 22.0 release of the python `packaging` package removed the
`LegacyVersion` trait, causing ZFS to no longer compile.

This commit replaces the sections of `ax_python_dev.m4` that rely on
`LegacyVersion` with updated implementations from the upstream
`autoconf-archive`.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
Closes openzfs#14297
usaleem-ix pushed a commit to truenas/zfs that referenced this pull request Jan 24, 2023
The 22.0 release of the python `packaging` package removed the
`LegacyVersion` trait, causing ZFS to no longer compile.

This commit replaces the sections of `ax_python_dev.m4` that rely on
`LegacyVersion` with updated implementations from the upstream
`autoconf-archive`.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
Closes openzfs#14297
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 3, 2023
The 22.0 release of the python `packaging` package removed the
`LegacyVersion` trait, causing ZFS to no longer compile.

This commit replaces the sections of `ax_python_dev.m4` that rely on
`LegacyVersion` with updated implementations from the upstream
`autoconf-archive`.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
Closes openzfs#14297
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants