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

[BUG] Recent release introduced a breaking change on MingW #3450

Open
AlmogBaku opened this issue Jul 14, 2022 · 5 comments · May be fixed by pypa/distutils#160
Open

[BUG] Recent release introduced a breaking change on MingW #3450

AlmogBaku opened this issue Jul 14, 2022 · 5 comments · May be fixed by pypa/distutils#160
Labels
bug Needs Repro Issues that need a reproducible example. Needs Triage Issues that need to be evaluated for severity and status.

Comments

@AlmogBaku
Copy link

setuptools version

63.1.0

Python version

OS

Windows

Additional environment information

No response

Description

7134521#diff-33b1124f858029240f7b1bce31b093b3e054100b2450ac8b3c067cfe9e464670R343

This line introduced a breaking change. Although theoretically removing the dynamic library should behave as the same, it threw an error that was really hard to find (since it's the error is new and google not indexing it).

Expected behavior

  1. It should warn instead of raising an error
  2. More informative error message

How to Reproduce

  1. Use setuptools with MingW
  2. Add dynamic library

Output

don't know how to set runtime library search path on Windows
@lazka
Copy link
Contributor

lazka commented Jul 14, 2022

Does your package build with MSVC? Can you link to the source?

@abravalheri
Copy link
Contributor

abravalheri commented Jul 14, 2022

Hi @AlmogBaku , thank you very much for reporting this. Could you please improve the "how to reproduce" section? Ideally what we are looking for is a self-contained minimal example.

You can find some tips on how to do that on https://stackoverflow.com/help/minimal-reproducible-example.

@abravalheri abravalheri added the Needs Repro Issues that need a reproducible example. label Jul 14, 2022
@DWesl
Copy link
Contributor

DWesl commented Jul 18, 2022

The maintainers asked for a minimal example, that is, an example setup.py that will exhibit the problem. Your example requires a C extension module; I usually use Cython for examples for this type of report so I only have to write five lines of code or so. Since you have a date for when the behavior changed, you may want to show what python setup.py build_ext does with a version of setuptools before that date as well as what it does with a current release.

As a starting point:
setup.py

from setuptools import setup
from Cython.Build import cythonize
setup(ext_modules=cythonize(["hello.pyx"]))

hello.pyx

def hello():
    print("Hello")

On Cygwin, that patch should be giving me a warning (not an error) if I am running into the same problem. If I run python setup.py build_ext with setuptools<60, everything succeeds with no warning. No differences from your report so far. If I run the same command with the most recent setuptools, everything succeeds, still with no warning. That is a difference from your report, following the steps outlined in your "How to Reproduce" section exactly.

If I add the code from the test from the patch you marked as a problem in setup.py between the imports and the call to setup()

from distutils.cygwinccompiler import CygwinCCompiler
compiler = CygwinCCompiler()
compiler.runtime_library_dir_option('/foo')

then I get the warning that you would see as an error. If I install setuptools<63.1, I don't get the warning anymore; however, I did not actually use compiler anywhere, so I do not get the compiler error I would expect from passing -rpath=/foo to the linker (I have gotten it from other peoples' code, but I do not know what they did to convince setuptools to use the customized compiler).

The maintainers asking for a self-contained minimal example are basically asking what else you have to add to this code so that it:

  1. Does what you want with runtime_library_dir_option for setuptools<63.1
  2. Fails due to the new error checking with setuptools>=63.1

because setuptools works as expected for the use-cases they can think of (for example, I think something like the original Cython example is included in CI runs on Cython and possibly setuptools and distutils for every commit, and they would have complained if this were broken). The six-line setup.py proposed above should already fail with the new error on new setuptools, so you're halfway there, you just need to figure out what your code is doing with compiler to actually set the runtime library directory with setuptools<63.1.

I suspect the problem is closer to setuptools<60 vs setuptools>=60, where old versions would use the stdlib version of distutils, patched by the distribution maintainers so this problem didn't appear in most cases (linked from comments in distutils), and new versions that use setuptools's (unpatched) distutils, which cause either the compiler error or the python error mentioned here. If that's the case, I think there's an environment variable named something like SETUPTOOLS_USE_DISTUTILS=stdlib that should enable the old behavior (use the old patched distutils instead of the new unpatched setuptools-bundled distutils that distribution maintainers may not be seeing if they keep the last version of setuptools that works with numpy).

Including the version of setuptools where your code was last known to work (by checking timestamps on your code's release tarballs against setuptools release dates or by checking each setuptools version in turn (binary search can speed that up if you have endpoints); the former tends to give "the old version of our code worked with 58.2, the new version of our code is now broken with 63.3", the latter can give "our current code works with 63.0, but not with 63.1") can help maintainers develop hypotheses about what changes are likely to cause problems; the most likely answer changes if the code works on setuptools==62 (rules out my hypothesis) or setuptools==63.2 (rules out yours; unlikely since you started with 63.1 instead of 63.3).

A bit of background on that patch: I think rpath is how ELF (the Linux executable format) tells the runtime dynamic-library loader where to look for dependencies of that dynamic library, basically an alternative to LD_LIBRARY_PATH. Windows does not have LD_LIBRARY_PATH, only PATH, which is also used for finding dynamic libraries. PE/COFF (the Windows executable format) does not have an attribute or field similar to rpath; however, Windows implicitly adds things to PATH so just putting all the dynamic libraries in the same directory seems to be the closest analogue.

@DWesl
Copy link
Contributor

DWesl commented Jul 26, 2022

With setuptools==62.0.0 and setup.py looking like this:

from setuptools import setup, Extension
from Cython.Build import cythonize

setup(
    ext_modules=cythonize(
        Extension("hello", sources=["hello.pyx"], runtime_library_dirs=["/spam"])
    )
)

I get

$ python setup.py build_ext --force
running build_ext
building 'hello' extension
INFO: C compiler: gcc -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall

INFO: compile options: '-I/usr/include/python3.9 -c'
INFO: gcc: hello.c
INFO: gcc -shared -Wl,--enable-auto-image-base build/temp.cygwin-3.3.5-x86_64-3.9/hello.o -L/usr/lib/python3.9/config -L/usr/lib -Wl,--enable-new-dtags,-R/spam -lpython3.9 -o build/lib.cygwin-3.3.5-x86_64-3.9/hello.cpython-39-x86_64-cygwin.dll
/usr/lib/gcc/x86_64-pc-cygwin/11/../../../../x86_64-pc-cygwin/bin/ld: unrecognized option '--enable-new-dtags'
/usr/lib/gcc/x86_64-pc-cygwin/11/../../../../x86_64-pc-cygwin/bin/ld: use the --help option for usage information
collect2: error: ld returned 1 exit status
error: Command "gcc -shared -Wl,--enable-auto-image-base build/temp.cygwin-3.3.5-x86_64-3.9/hello.o -L/usr/lib/python3.9/config -L/usr/lib -Wl,--enable-new-dtags,-R/spam -lpython3.9 -o build/lib.cygwin-3.3.5-x86_64-3.9/hello.cpython-39-x86_64-cygwin.dll" failed with exit status 1

which is the messy compiler error message the new exception is designed to hide and clarify.

@AlmogBaku, what do you get with this setup.py, the hello.pyx given above, and setuptools==62?

Alternately, does your original issue still occur with setuptools==62 or SETUPTOOLS_USE_DISTUTILS=stdlib or if you manually edit your setuptools installation to revert the patch you blame?

@DWesl
Copy link
Contributor

DWesl commented Jul 31, 2022

Alternate framing of this issue: pypa/distutils#150 should have gone with the Cygwin patch that inspired it for consistency with Darwin/OSX rather than raising a DistutilsPlatformError (perhaps extending the patch to cover whatever platform the issue occurred on, maybe MSYS2?), or should at least have mentioned the option runtime_library_dirs in the error message so there's something to grep setup.py for.

@AlmogBaku, do you agree with this framing of your complaint?

DWesl added a commit to DWesl/distutils that referenced this issue Jul 31, 2022
Extend the error message for attempting to call `runtime_library_dir_option` on Windows with the most likely way I have found to trigger the call, creating an `Extension` that specifies `runtime_library_dirs`.

Hopefully this will give users something to search for in their `setup.py` and a starting point for what to change.  Inspired by pypa/setuptools#3450.
DWesl added a commit to DWesl/setuptools that referenced this issue Jul 31, 2022
May want a closer match to the exception message to make it easier to search, but this should help.

Inspired by pypa#3450.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Needs Repro Issues that need a reproducible example. Needs Triage Issues that need to be evaluated for severity and status.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants