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

Add support for building extensions using MinGW compilers #184

Merged
merged 12 commits into from
Jun 28, 2024

Conversation

naveen521kk
Copy link
Contributor

See individual commits for details on what this PR changes.

A new version of #78
Fixes #34

cc @lazka

@naveen521kk naveen521kk force-pushed the add-mingw-support branch 3 times, most recently from c19bcd5 to 04bea17 Compare October 8, 2022 06:17
distutils/sysconfig.py Outdated Show resolved Hide resolved
distutils/sysconfig.py Outdated Show resolved Hide resolved
@jaraco
Copy link
Member

jaraco commented Nov 13, 2022

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.

@naveen521kk
Copy link
Contributor Author

Thanks, I would be able to work on this later this week.

@naveen521kk naveen521kk marked this pull request as draft November 13, 2022 17:33
@Karimsultan
Copy link

See individual commits for details on what this PR changes.

A new version of #78

Fixes #34

cc @lazka

@jaraco jaraco force-pushed the main branch 3 times, most recently from 12769e5 to d23e28a Compare November 5, 2023 15:55
@cr1901
Copy link

cr1901 commented Nov 6, 2023

@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' --plat-name must be one of ('win32', 'win-amd64', 'win-arm32', 'win-arm64') error.

Since the workaround is going away, soon this means I won't be able to install packages anymore.

@naveen521kk
Copy link
Contributor Author

naveen521kk commented Nov 7, 2023

@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' --plat-name must be one of ('win32', 'win-amd64', 'win-arm32', 'win-arm64') error.

Hey, I'm sorry, I kinda forgot about this PR. I'll spend some time this week to get this working.

@cr1901
Copy link

cr1901 commented Nov 8, 2023

@naveen521kk

Hey, I'm sorry, I kinda forgot about this PR.

No worries, I'm kinda happy you responded at all, tbh.

I'll spend some time this week to get this working.

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 pip installing pdm from the source tree b/c of the --plat-name errors (it works otherwise fine). Will be interested in whether this allows installing pdm from PyPI to succeed.

@naveen521kk naveen521kk force-pushed the add-mingw-support branch 8 times, most recently from 638496c to ccfef91 Compare November 9, 2023 13:22
@jaraco jaraco closed this Jun 27, 2024
@jaraco jaraco reopened this Jun 27, 2024
@jaraco
Copy link
Member

jaraco commented Jun 27, 2024

I've resolved the conflicts and I'd like to test the changes, but GitHub is rejecting me pushing the resolution.

 distutils add-mingw-support @ git push
fatal: The upstream branch of your current branch does not match
the name of your current branch.  To push to the upstream branch
on the remote, use

    git push origin HEAD:refs/pull/184/head

To push to the branch of the same name on the remote, use

    git push origin HEAD

To avoid automatically configuring an upstream branch when its name
won't match the local branch, see option 'simple' of branch.autoSetupMerge
in 'git help config'.

 distutils add-mingw-support [128] @ git push origin HEAD:refs/pull/184/head
Enumerating objects: 99, done.
Counting objects: 100% (99/99), done.
Delta compression using up to 12 threads
Compressing objects: 100% (66/66), done.
Writing objects: 100% (68/68), 10.07 KiB | 10.07 MiB/s, done.
Total 68 (delta 51), reused 0 (delta 0), pack-reused 0 (from 0)
remote: Resolving deltas: 100% (51/51), completed with 25 local objects.
To https://github.com/pypa/distutils
 ! [remote rejected]     HEAD -> refs/pull/184/head (deny updating a hidden ref)
error: failed to push some refs to 'https://github.com/pypa/distutils'

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.

@jaraco
Copy link
Member

jaraco commented Jun 27, 2024

That failed because there was a latent reference to setup.cfg. Corrected in 416ef6a.

@jaraco
Copy link
Member

jaraco commented Jun 27, 2024

And another revision to replace testing with test. 5fb38a1.

@jaraco
Copy link
Member

jaraco commented Jun 27, 2024

Tests are now failing during collection:

______________ ERROR collecting distutils/tests/test_install.py _______________
ImportError while importing test module 'D:/a/distutils/distutils/distutils/tests/test_install.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
distutils/tests/test_install.py:16: in <module>
    from distutils.util import is_mingw
E   ImportError: cannot import name 'is_mingw' from 'distutils.util' (D:/a/_temp/msys64/mingw64/lib/python3.11/distutils/util.py)
___________ ERROR collecting distutils/tests/test_mingwccompiler.py ___________
ImportError while importing test module 'D:/a/distutils/distutils/distutils/tests/test_mingwccompiler.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
distutils/tests/test_mingwccompiler.py:3: in <module>
    from distutils.util import split_quoted, is_mingw
E   ImportError: cannot import name 'is_mingw' from 'distutils.util' (D:/a/_temp/msys64/mingw64/lib/python3.11/distutils/util.py)
___________ ERROR collecting distutils/tests/test_unixccompiler.py ____________
ImportError while importing test module 'D:/a/distutils/distutils/distutils/tests/test_unixccompiler.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
distutils/tests/test_unixccompiler.py:10: in <module>
    from distutils.util import _clear_cached_macosx_ver
E   ImportError: cannot import name '_clear_cached_macosx_ver' from 'distutils.util' (D:/a/_temp/msys64/mingw64/lib/python3.11/distutils/util.py)

Clearly the wrong 'distutils' is getting imported. I don't know why.

@jaraco
Copy link
Member

jaraco commented Jun 27, 2024

@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)?

@abravalheri
Copy link
Contributor

Clearly the wrong 'distutils' is getting imported. I don't know why.

What is the order in sys.meta_path? Anything weird there?

@lazka
Copy link
Contributor

lazka commented Jun 27, 2024

@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)?

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.

@lazka
Copy link
Contributor

lazka commented Jun 27, 2024

Clearly the wrong 'distutils' is getting imported. I don't know why.

I can reproduce locally. I'll try to figure out why.

@lazka
Copy link
Contributor

lazka commented Jun 27, 2024

What is the order in sys.meta_path? Anything weird there?

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'>]

@lazka
Copy link
Contributor

lazka commented Jun 27, 2024

Removing this line makes things work again:

--import-mode importlib

@naveen521kk
Copy link
Contributor Author

@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)?

I've pulled those changes. I'll also check out why the wrong distutils are imported.

What is the order in sys.meta_path? Anything weird there?

Not sure if there's anything weird there but this is what I get.

$ python
Python 3.11.9 (main, Apr 12 2024, 09:55:31)  [GCC 13.2.0 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys.meta_path
[<_virtualenv._Finder object at 0x000001745b6ffe50>, <class '_frozen_importlib.BuiltinImporter'>, <class '_frozen_importlib.FrozenImporter'>, <class '_frozen_importlib_external.PathFinder'>, <class '__editable___distutils_0_1_dev4112_g5fb38a1_d20240627_finder._EditableFinder'>]
>>>

@jaraco
Copy link
Member

jaraco commented Jun 27, 2024

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.

@lazka
Copy link
Contributor

lazka commented Jun 27, 2024

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.

@jaraco
Copy link
Member

jaraco commented Jun 27, 2024

When I was testing with pytest 8.2, where _pytest.pathlib.resolve_pkg_root_and_module_name, invoked when import-mode=importlib, doesn't rely on sys.path at all. On Pytest 8.0.2, the logic is very different. Let me see if I can trace why and how the import works on my macOS machine.

@lazka
Copy link
Contributor

lazka commented Jun 27, 2024

I now tried current main on Ubuntu 22.04, using Python 3.10, and it fails the same way, importing the wrong distutils.

python3 -m venv venv
source venv/bin/activate
pip install -e '.[test]'
pytest distutils/tests

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.

@jaraco
Copy link
Member

jaraco commented Jun 28, 2024

Aha. I see the difference now. It's the invocation of pytest distutils/tests.

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 bash -c pytest, the collection completes and the tests run (with a few failures not yet seen).

I think here's what's happening:

When passing distutils/tests to pytest, it causes distutils/* not to be collected, and thus falls back to sys.path for resolving the import distutils. If not passing distutils/tests, pytest will traverse ./distutils and because it's also run with --doctest-modules, the modules in distutils/* get imported during collection.

Indeed, if I disable the line with --doctest-modules and then run tests with pytest, the same collection failure occurs. The same problem exists even after restoring pytest 8.2+.

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 -k <pattern> to downselect tests after collection (so doctests are always collected even if not run).

What about [adding some config]?

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 distutils/tests)?

lazka added 2 commits June 28, 2024 08:35
…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.
@lazka
Copy link
Contributor

lazka commented Jun 28, 2024

How about for now, let's update the test execution to run the same way as it runs for other jobs (without specifying distutils/tests)?

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

@jaraco
Copy link
Member

jaraco commented Jun 28, 2024

Looks great. Thanks!

@jaraco jaraco merged commit 99ca390 into pypa:main Jun 28, 2024
18 of 20 checks passed
@lazka
Copy link
Contributor

lazka commented Jun 28, 2024

Thanks!

If there is any problems/issues feel free to ping us

@naveen521kk naveen521kk deleted the add-mingw-support branch June 28, 2024 13:14
jaraco added a commit that referenced this pull request Jun 28, 2024
Add support for building extensions using MinGW compilers
@jaraco jaraco mentioned this pull request Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Supporting MSYS2-MINGW python
6 participants