[BUG] setuptools 69.4.0 breaks mpi4py installation

[dalcinl]

setuptools version

69.4.0

Python version

all

OS

POSIX

Additional environment information

No response

Description

Originally reported at mpi4py/mpi4py#484
Also reported here for another project: https://gitlab.com/petsc/petsc/-/issues/1579

The commit pypa/setuptools@a131f83 changed the return type of a publich method within a public class within a public module.

Expected behavior

pip install mpi4py should succeed with no traceback.

How to Reproduce

pip install mpi4py should be enough.

However, it cannot be reproduced easily as setuptools 69.4.0 was yanked for other reasons.

Output

File "/tmp/pip-build-env-qxqtzla9/overlay/lib/python3.10/site-packages/setuptools/_distutils/ccompiler.py", line 1232, in gen_lib_options
          lib_opts.extend(always_iterable(compiler.runtime_library_dir_option(dir)))
        File "/tmp/pip-install-u2viu3lx/mpi4py_70e5ecd7d41d4c9fb50ac4e92d5eb59f/conf/mpidistutils.py", line 68, in rpath_option
          if option.startswith('-R'):
      AttributeError: 'list' object has no attribute 'startswith'

[dalcinl]

Looks like the method causing the issue was always allowed to return either str or list[str], and setuptools just made use use of such freedom relatively recently changing from str to list[str].

Is there any change setuptools would accept the following:

return [
    # Force RUNPATH instead of RPATH
    "-Wl,--enable-new-dtags",
    "-Wl,-rpath," + dir
]

to this?

# Force RUNPATH instead of RPATH
return "-Wl,--enable-new-dtags,-rpath," + dir

[jaraco]

That change was introduced in #214 by @haampie with this rationale:

Also split -Wl,--enable-new-dtags and -Wl,-rpath,... into two separate arguments, which is more common, and more likely to be parsed correctly by compiler wrappers.

Whatever the fix is, it'll need to be contributed to distutils first and then merged here (changes directly to _distutils aren't allowed), but that can be done relatively easily and quickly. I'm transferring the issue there.

[jaraco]

changed the return type of a public method within a public class within a public module

Although Python conventions have become more rigorous in the past couple of decades, distutils is ancient and doesn't always conform to modern expectations about public/private visibility (FYI).

That said, you've also observed that this function previously did return strings or lists, and that was why in the review I didn't challenge the change of return type.

Is there any change setuptools would accept [a change back to a single string]?

Yes, maybe. My preference would be to return richer objects that more accurately reflect the semantic meaning of the operation. If --enable-new-dtags and -rpath are independent settings, I'd prefer for them to be specified independently and not bundled into a single string, especially if common usage is to supply them separately.

That said, I'd like not to be breaking our most popular downstream projects.

Here's what I propose:

• patch distutils/setuptools to restore the prior expectation with a comment about why the change
• with mpi4py and other affected projects, register and issue and add support for lists from this method
• after a certain period (6-12 months?), start relying on a new expectation that lists might be returned

[dalcinl]

From mpi4py side, I'm working on fixing the issue and making a new release. Therefore, a fix in setuptools's side would not be strictly required, as long as users do not try to install old versions.

I'm not sure what's setuptools policy in this case. Does setuptools care about breaking install of old package versions if there is a new (patch, micro) version that fixes the install issue?

If --enable-new-dtags and -rpath are independent settings

They are indeed independent, but related. However, I would still point out that with the current implementation, the --enable-new-dtags will be inserted for every runtime path if more than one rpath is passed to the linker, while only one --enable-new-dtags option is needed, it applies to all rpaths. Of course, this (admittedly minor) nit comes originally from distutils, so maybe you would just prefer to ignore it.

[jaraco]

Does setuptools care about breaking install of old package versions if there is a new (patch, micro) version that fixes the install issue?

Short answer is it depends, but generally speaking, Setuptools aims to support 99.9% of all packages currently in use. We want it to be rare for a user to have to pin to an older Setuptools to install an older package. First and foremost, we want users to feel comfortable and safe updating to the latest Setuptools version (so working at head/latest release is a viable option for most). That said, we'd also hope that users are also upgrading their other dependencies at the same cadence so that their use-case can quickly move into the future and not be reliant on older behaviors. How long do you think it would be before 99.9% of mpi4py users (that depend on builds from source) will have moved onto a new release with your fix?

[jaraco]

They are indeed independent, but related. However, I would still point out that with the current implementation, the --enable-new-dtags will be inserted for every runtime path if more than one rpath is passed to the linker, while only one --enable-new-dtags option is needed, it applies to all rpaths. Of course, this (admittedly minor) nit comes originally from distutils, so maybe you would just prefer to ignore it.

I'm slightly inclined to just ignore it, but I'm also open to suggestions (or PRs) for improvement.

[dalcinl]

How long do you think it would be before 99.9% of mpi4py users (that depend on builds from source) will have moved onto a new release with your fix?

Well, it is really hard to tell. From PyPI download stats here https://www.pepy.tech/projects/mpi4py (I believe these are sdist tarballs from PyPI only), looks like there are still around 3K downloads per day for version 3.1.4 that was released in Nov 2022 (16 months ago). It is worth noting that user may not really hit the issue, unless they are building things such that passing a rpath is needed, which I believe is not the usual case (that's the reason the issue went unnoticed until today for a setuptools change from Sep 2023).

I'm slightly inclined to just ignore it, but I'm also open to suggestions (or PRs) for improvement.

I'll take a look at setuptools tomorrow. I think there are better places to inject the --enable-new-dtags flags if any rpath is being using. Then we can get back to the old interface of returning just a string for rpath, and we should be all set. Of course, this could break setuptools users specifically intercepting options to filter out --enable-new-dtags, although I don't think anyone would like the old rpath semantics.

To summarize, there are three options for you to choose.

1. Leave setuptools as it is now. A few mpi4py users may eventually experience trouble if using an old release, but they will have a new patch release to upgrade.
2. Make setuptools return the longish and ugly rpath string option "-Wl,--enable-new-dtags,-rpath," + dir, and keep it that way for (a couple?) years to come.
3. Make setuptools return a short and nice rpath string option "-Wl,-rpath," + dir, and inject the "-Wl,--enable-new-dtags options elsewhere, for example as pre/post link flags in the compiler object, or directly at the link stage within the compiler.link_shared_object() method if any rpath is being passed in. As with any such changes, there is some slim possibility of breaking some other (likely obscure) use case, like filtering out the --enable-new-dtags flag.

I've presented you all the alternatives with pros and cons. I can contribute a PR for either (2) or (3). The decision on how to proceed is yours, just let me know.

[dalleyg]

In case it helps with testing, I ran into the problem today when running the CI tests on GitHub for a private repository. The CI tests run the mpi4py/setup-mpi@v1 GitHub action on ubuntu-latest, followed by the equivalent of python -m pip install mpi4py>=3.0.0.

[dalcinl]

In case it helps with testing

I keep getting reports about this failure. The issue also affected petsc4py and eventually slepc4py. Having a FOSS package in the PyPI top 1000 is not fun 🤦 I'm starting to think that the best course of action is to start by implementing as setuptools fix as per option (2) in my comment above #246 (comment).

[jaraco]

(that's the reason the issue went unnoticed until today for a setuptools change from Sep 2023).

To be sure, the change landed in distutils in February and only was merged into Setuptools this week, so the issue is very recent.

#247 implements (2) currently with a retention window of 6 months. I'll expand that to a year for good measure. Good chance it lingers longer than that.

I'll leave (3) for later consideration.