-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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] Unprotected path for objects passed to windows linker #4645
Comments
Hi @mglisse. Is this a bug in setuptools or a feature request? Anyway, it would be nice if you can provide a simplified reproducer. If you could please have a look on this document and try to create a very small toy example that only uses setuptools itself without all the extra stuff in https://github.com/GUDHI/gudhi-devel/blob/master/src/python/setup.py.in#L30 and without the extra templating, that would be ideal. Footnotes
|
I guess that's up to you 😉
I don't think setup.py is explicitly passing such a string. All it gives is the source, which apparently starts with "D:"
I'll try, but as a linux user, it is not easy (have to locate a suitable windows VM, install what I need, etc, before I can even try to create a small reproducer), so I wanted to at least check if the description made the issue obvious to someone familiar with setuptools. |
Ok, in the empty directory
and xyz.cc that contains
Then I run
If on the other hand I use python 3.13.0rc2
|
@mglisse, one thing that I just noticed: in your reproducer you use an absolute path as source for the extension object and backslashes. However the setuptools docs explicitly says you should use a relative path, Unix-style:
https://setuptools.pypa.io/en/latest/userguide/ext_modules.html#extension-api-reference Can the problem that you are facing be explained by this mismatch? |
I think I only used forward slashes.
Ah... Thanks for the pointer.
That seems likely. We will see if we can adapt our scripts to use a relative path (it isn't trivial, setup.py is in a build directory, unlike the sources it references). |
Sorry, my bad, you are right.
Please feel free to propose a PR either in
Thank you very much for having a look on that! |
Maybe you can try |
Ideally, we would have the original sources in some read-only directory somewhere, and everything generated (setup.py by cmake, off_utils.cpp by cython, etc) in a build directory elsewhere. Asking that everything is together requires either writing to the source directory, or copying the sources to the build directory, that's doable but not the cleanest.
With 3.12, the object files are sent to a temporary directory that looks like the concatenation of
That's quite ugly, but possibly a good way to postpone having to rework our build system, thanks. |
From https://docs.python.org/3.13/library/os.path.html#os.path.isabs
Now looking at the code in distutils to produce the path of object files setuptools/setuptools/_distutils/ccompiler.py Lines 970 to 991 in 962835d
First it explicitly strips "C:". Then if what remains is an absolute path (changed in 3.13!) it strips one initial I find the new behavior of |
That is something that can be discussed with the maintainers of |
@jaraco, we might have to start testing on 3.13 to see if this change in behaviour of the stdlib affects other parts of setuptools/distutils and check if it will cause further disruption. |
I'm about to roll out 3.13 testing on all skeleton projects. jaraco/skeleton#141 jaraco/skeleton#146. |
That does give something "relative", but the code doesn't expect a relative path with a bunch of Monkey patching Meanwhile I filed pypa/distutils#297, hopefully it is simple (1-liner patch) and focused (1 (private) function does not do what its doc says) enough that they will be willing to patch it, even if the motivation comes from some unsupported use case. |
setuptools version
setuptools==73.0.1
Python version
Python 3.13
OS
Windows
Additional environment information
This happens on the conda-forge servers.
Description
An object file is passed to link.exe directly with its unix-like absolute path '/bld/gudhi_1725978620692/work/build/version/python/gudhi/off_utils.obj'. Since it starts with a '/', it is interpreted as an option and ignored.
(In the cl.exe command, the source filename is prefixed by
/TpD:
and the object by/Fo
, so there is no problem there)(We did not have this issue before because we always ended up with relative paths, which don't start with '/' and just work)
Expected behavior
It would be nice if the object was passed in a safe way. If there existed an option (tentatively called
/Input
here), it could pass/Input/path/to/file.obj
instead of just/path/to/file.obj
, but I didn't find it in the doc. More simply, it should be possible to sanitize the path (using functions from pathlib?) before using it, probably rewriting to\path\to\file.obj
.How to Reproduce
This is the build win_64_numpy2python3.13.____cp313 from
conda-forge/gudhi-feedstock#67
(you can get a look at setup.py at https://github.com/GUDHI/gudhi-devel/blob/master/src/python/setup.py.in#L30)
I don't know how to reproduce their exact setup, maybe using an obsolute path somewhere is important.
Output
(full output at https://dev.azure.com/conda-forge/84710dde-1620-425b-80d0-4cf5baca359d/_apis/build/builds/1024741/logs/121)
The text was updated successfully, but these errors were encountered: