-
Notifications
You must be signed in to change notification settings - Fork 71
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
Add support for building extensions using MinGW compilers #184
Conversation
c19bcd5
to
04bea17
Compare
I notice there are conflicts. I tried to resolve the conflicts, but I don't seem to have access to the fork, so I've pushed a couple of commits to the add-mingw-support-jaraco branch, resolving the conflict. Feel free to pull those changes in. |
Thanks, I would be able to work on this later this week. |
12769e5
to
d23e28a
Compare
@naveen521kk What is the status of this PR? I've been switching to PDM for a lot of my work, and installing a number of packages break with the good ol' Since the workaround is going away, soon this means I won't be able to install packages anymore. |
Hey, I'm sorry, I kinda forgot about this PR. I'll spend some time this week to get this working. |
6918256
to
77277a5
Compare
No worries, I'm kinda happy you responded at all, tbh.
Excellent, tyvm. I'm happy to test locally, although I'm not sure how I would replace the vendored version in my Python with this version for testing purposes. Right now I'm still |
638496c
to
ccfef91
Compare
I've resolved the conflicts and I'd like to test the changes, but GitHub is rejecting me pushing the resolution.
I'm not sure what "deny updating a hidden ref" means. I've pushed the commit (1b38bc9) to the add-mingw-support branch to see if tests pass. |
That failed because there was a latent reference to setup.cfg. Corrected in 416ef6a. |
And another revision to replace |
Tests are now failing during collection:
Clearly the wrong 'distutils' is getting imported. I don't know why. |
@naveen521kk Can you pull 5fb38a1 over to your branch so we can get the CI to update here and then investigate why the wrong distutils is getting imported (but only for the mingw jobs)? |
What is the order in |
From what I see you succeeded in pushing it before writing this (?? https://github.com/msys2-contrib/distutils/tree/add-mingw-support). I can also rebase the whole thing on top of main if wanted. |
I can reproduce locally. I'll try to figure out why. |
distutils/tests/test_install.py:8: in <module>
assert 0, sys.meta_path
E AssertionError: [<_pytest.assertion.rewrite.AssertionRewritingHook object at 0x000001a442ffe510>, <_virtualenv._Finder object at 0x000...mportlib_external.PathFinder'>, <class '__editable___distutils_0_1_dev4112_g5fb38a1_d20240627_finder._EditableFinder'>] |
Removing this line makes things work again: Line 5 in 4fd2d56
|
I've pulled those changes. I'll also check out why the wrong distutils are imported.
Not sure if there's anything weird there but this is what I get.
|
I encountered similar errors in collection in #259, which I worked around by pinning pytest. And I can see the pin has taken effect, as pytest 8.0.2 is being used. Yet, somehow the import mode is still finding the wrong distutils. I did a deep dive on the issue in pytest-dev/pytest#12490. I still have no clue how the msys environment is somehow reviving that issue. |
How is that supposed to work anyway? With the pytest setting the current dir isn't in sys.path anymore. The stdlib pathfinder is before the editable finder in sys.meta_path, so I kinda expect it to pick up the wrong distutils, no? I need to check in a Linux with Python 3.11 somehow, I only have 3.12 without distutils around. |
When I was testing with pytest 8.2, where |
I now tried current main on Ubuntu 22.04, using Python 3.10, and it fails the same way, importing the wrong distutils.
I wonder what tox does differently there. What about: ? diff --git a/pytest.ini b/pytest.ini
index b53e0d93..35409603 100644
--- a/pytest.ini
+++ b/pytest.ini
@@ -4,6 +4,7 @@ addopts=
--doctest-modules
--import-mode importlib
consider_namespace_packages=true
+pythonpath= .
filterwarnings=
## upstream
this fixes things for both Ubuntu and mingw. |
Aha. I see the difference now. It's the invocation of I created this dockerfile to replicate the behavior. from jaraco/multipy-tox:jammy
RUN git clone https://github.com/msys2-contrib/distutils
WORKDIR distutils
RUN git checkout add-mingw-support
RUN py -3.10 -m venv .venv
RUN py -m pip install -e .[test]
# simulate activating the venv:
ENV PATH=.venv/bin:$PATH
CMD pytest distutils/tests If I run that docker image, it fails with the same issues in CI. If I instead run docker with the command I think here's what's happening: When passing Indeed, if I disable the line with It seems that the reason I haven't encountered this issue previously or with other skeleton-based projects is because I rarely ever direct the collection by providing a subpath, but instead I use
I really try to avoid having to add config, especially to a single project. I'd like to be able to rely on defaults and best practices to make the project under test importable. I guess distutils is special here, because although it's pip-installed, a pip-installed package doesn't take precedence over a stdlib module. How about for now, let's update the test execution to run the same way as it runs for other jobs (without specifying |
…rted distutils currently doesn't support pytest collection that doesn't start at least at the distutils dir or above (and not distutils/tests) since it requires the local distutils being imported before the tests are run, otherwise the stdlib distutils takes precedence. Adjust the pytest call to not pass a path to work around this. Since pytest currently fails to skip collecting venvs with mingw python (see pytest-dev/pytest#12544) move the venv to /tmp instead.
MSYS2 has stopped installing gcc compatibility binaries in clang environments by default some time ago, and distutils is currently hardcoded to look for "gcc", while only cc/c++ and clang/clang++ are in PATH. Work around for now by explicitely setting CC/CXX to override the defaults. Idealy distutils would try to look harder for a valid compiler before giving up, but this can be improved in the future.
Done, see the commits for details. It seems a bit brittle since this import side effect no longer seems to be there with pytest 8.2.0 for example, but fine with me. If you update pytest in the future I'd be happy to help debug Windows specific issues. For future reference, this makes the tests pass with pytest 8.2.0 here: diff --git a/pytest.ini b/pytest.ini
index b53e0d93..14188d07 100644
--- a/pytest.ini
+++ b/pytest.ini
@@ -3,7 +3,7 @@ norecursedirs=dist build .tox .eggs
addopts=
--doctest-modules
--import-mode importlib
-consider_namespace_packages=true
+pythonpath= .
filterwarnings=
## upstream |
Looks great. Thanks! |
Thanks! If there is any problems/issues feel free to ping us |
Add support for building extensions using MinGW compilers
See individual commits for details on what this PR changes.
A new version of #78
Fixes #34
cc @lazka