setuptools 64 broke editable build fallback when no compiler is found. #8458
Comments
CaselIT
added
sqlalchemy.ext
|
have you tested on 1.4? I notice cython in the script output above. |
|
also can you clarify that this is only with -e from a source install, not a regaulr pip with the .tar.gz? if so, this is nearly a non-issue |
|
yes, it's the same, the issue is not cython, but the fact that we try compiling then fallback to pure python |
correct, normal, non editable install works ok |
|
this patch fixes it, (should probably be improved in checking better the system exit case) diff --git a/setup.py b/setup.py
index 604958922..847897d58 100644
--- a/setup.py
+++ b/setup.py
@@ -1,5 +1,7 @@
import os
+from pathlib import Path
import platform
+import shutil
import sys
from setuptools import __version__
@@ -126,8 +128,17 @@ def run_setup(with_cext):
)
kwargs["ext_modules"] = []
-
- setup(cmdclass=cmdclass, distclass=Distribution, **kwargs)
+ dist_dir = None
+ if "--dist-dir" in sys.argv:
+ kwargs["script_args"] = script_args = sys.argv[1:]
+ index = script_args.index("--dist-dir") + 1
+ dist_dir = script_args[index]
+ script_args[index] = dist_dir + str(with_cext)
+ setup(cmdclass=cmdclass, distclass=Distribution, **kwargs)
+ if dist_dir:
+ if Path(dist_dir).is_dir():
+ shutil.rmtree(dist_dir)
+ Path(dist_dir + str(with_cext)).rename(dist_dir)
if not cpython:
@@ -152,10 +163,8 @@ elif os.environ.get("DISABLE_SQLALCHEMY_CEXT"):
"Plain-Python build succeeded.",
)
else:
- try:
- run_setup(True)
- except BuildFailed as exc:
+ def retry(exc):
if os.environ.get("REQUIRE_SQLALCHEMY_CEXT"):
status_msgs(
"NOTE: Cython extension build is required because "
@@ -179,3 +188,10 @@ else:
"speedups are not enabled.",
"Plain-Python build succeeded.",
)
+
+ try:
+ run_setup(True)
+ except BuildFailed as exc:
+ retry(exc)
+ except SystemExit as exc:
+ retry(exc)that said, it's very hacky and it's likely not overly robust. Mike do you think it makes sense opening an issue on setuptools to ask for advide? |
|
we are already subclassing the build_ext command, so I need to understand the problem better before I would know if we have any chance of them helping us. are the same run() / build_extension() methods being called? what's |
|
The change seems to be how the editable build is run. Now it works more or less like this:
|
|
OK exit it called because...it's a subprocess? What im trying to understand is in the parent process, are we still inside of build_extension() ? because if their spawned process has an issue and an error code, they should still propagate an exception we can catch locally? what happens in the parent process? |
|
The commands are mostly run using this: https://github.com/pypa/setuptools/blob/ba3995e5705a22e13bb5d2231ac22c77e4417747/setuptools/_distutils/core.py#L193-L217
it exits. The main change I gather is that before what again I'm not 100% of the above, sice I have not read the pep 660 change. Maybe at the next meeting we can better speak about it. Personally I think that something like we are doing with setup.py (ie trying to build extensions and falling back on pure python) is not something they are supporting, at least for editable builds, with the current changes and we should instead use something else. I don't know what the something else is, so I would appreciate guidance on it |
OK that's bug on their end maybe? We need an exception that can be caught. Does it exit via SystemExit that we can catch? (edit: or maybe we just change our approach)
I'm sure it's not!
does cython have any guidance for this? Maybe we can just check up front for valid cython install before anything happens, then we just run either native or non-native. if native breaks, they have a broken cython install and that's not our problem, we are just differentiating between "no build tools or cython installed" vs "build tools and cython are present". I am sure there's a way to test for this that's "good enough". |
I think this is a viable thing. I think that the supported way would be doing something like this https://setuptools.pypa.io/en/latest/build_meta.html#dynamic-build-dependencies-and-other-build-meta-tweaks |
|
OK can you find out from them |
|
Mike Bayer has proposed a fix for this issue in the main branch: propose removal of "build fallback" https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4062 |
|
as discussed in the meeting I think we will be fine if we just remove everything from our setup.py and just build normally. It's only windows users that really hit this issue and they will have a wheel file on pypi. |
|
FWIW, this workaround lets me build editable on Windows with setuptools 65.3.0: |
|
this |
|
Commit 282d86d has updated the setup on the main branch. I'll leave it open since I plan to update also on the 1.4 setup, that has a similar functionality but is using the c files |
|
Do we want to do this for 1.4? I was thinking not. the change to setup.py is dramatic, it's plausible that people have CI /packaging setups that are finely tuned to our current setup.py. people have not reported this as an issue |
|
from an installer prospective nothing has changed. Unless they where parsing the error message in case of a failed compilation? |
|
an example of a change is, if the C compilation fails for just one module, the other three still build. now you have a SQLAlchemy installation where three of the four C libraries are present, and the other is pure Python. That's a combination for which we have no test support. In 1.4 it would likely work fine, but for example in 2.0, the thing I did here and then here would cause the library to not be importable, since we only tested that "cyextension.util" were working. 1.4 is very deep into its release series so IMO it's important we don't do any non-critical architectural changes at this point. |
Right, I think we need to change that part since the current setup.py could leave things broken. |
yes agree, in my point is just that in 1.4 we have something else going on, and past very early 1.4 releases, major reworks of that stuff are not warranted unless there were some blocking issue, like a new setuptools release that flat out wouldn't build at all, something like that. |
|
ok for 1.4, we can keep as is until we don't have a reason to change it. Any idea on how to share the list of cython modules so _has_cy and setup can share it? |
|
I dunno, you could scan the directory, it might be better just to make a unit test that tests things, like scans the directory and makes sure has_cy loaded them all, something like that |
|
fixed in 282d86d |
Currently running
pip install -e .on a machine without compiler breaks the build.This is most likely due to pypa/setuptools#3488
step to reproduce:
The output is something like
At the moment the normal install is working, so running
pip install .works, but it may be the next thing that breaks.From what I gather the issue is that the build is not run in a subprocess, so the raised exception is SystemExit. Catching SystemExit and retrying in that case does not fix the issue
The text was updated successfully, but these errors were encountered: