Opened 2 years ago
Closed 2 years ago
#27389 closed defect (fixed)
Fix src/build/cythonized/build/cythonized directory
Reported by: | jdemeyer | Owned by: | |
---|---|---|---|
Priority: | blocker | Milestone: | sage-8.7 |
Component: | build | Keywords: | |
Cc: | Merged in: | ||
Authors: | Jeroen Demeyer | Reviewers: | John Palmieri |
Report Upstream: | N/A | Work issues: | |
Branch: | a5c7119 (Commits, GitHub, GitLab) | Commit: | a5c7119f5488a8b34e09fa037aca9b26b3133d5e |
Dependencies: | Stopgaps: |
Description (last modified by )
Inside $SAGE_ROOT
, I see a file like
src/build/cythonized/build/cythonized/sage/libs/eclib/wrap.cpp
The duplication of build/cythonized
is not how it's supposed to be.
There is also an issue with dependency checking: when src/sage/libs/eclib/wrap.cpp
is edited and Sage is rebuilt, the new contents of that file are not taken into account.
This started with #27040, which accidentally removed the top-level source directory SAGE_SRC
from the include directories. This way, the include was only found in the build directory, confusing Cython.
Change History (21)
comment:1 Changed 2 years ago by
- Description modified (diff)
comment:2 Changed 2 years ago by
- Description modified (diff)
comment:3 Changed 2 years ago by
comment:4 Changed 2 years ago by
Is it a problem to have src/build/cythonized/setup.py
, a copy of src/setup.py
? That's been there for some time, I think.
comment:5 Changed 2 years ago by
It looks to me like the build/cythonized/build/cythonized
problem first arises in 8.7.beta2.
comment:6 Changed 2 years ago by
Could the problem be #27041?
comment:7 Changed 2 years ago by
No, it's #27040, and in particular, the directories are created when cythonize
is run in sage_build_cython.run
. The dependencies for the elements in module_list
are wrong: before #27040, I see (among other things):
/path/to/SAGE_ROOT/local/include/python2.7/pythread.h /path/to/SAGE_ROOT/src/sage/cpython/cython_metaclass.h /path/to/SAGE_ROOT/src/sage/libs/ntl/ntlwrap.h /path/to/SAGE_ROOT/src/sage/libs/ntl/ntlwrap_impl.h /path/to/SAGE_ROOT/src/setup.py
while after #27040, I see
/path/to/SAGE_ROOT/local/include/python2.7/pythread.h build/cythonized/sage/cpython/cython_metaclass.h build/cythonized/sage/libs/ntl/ntlwrap.h build/cythonized/sage/libs/ntl/ntlwrap_impl.h build/cythonized/setup.py
(I put a print statement in the Cython.Build.Dependencies.cythonize
function: after module_list, module_metadata = create_extension_list(...
, I added
for m in module_list: for dep in m.depends: print(dep)
) Does that clarify anything?
comment:8 Changed 2 years ago by
- Priority changed from major to blocker
comment:9 Changed 2 years ago by
- Description modified (diff)
comment:10 Changed 2 years ago by
- Description modified (diff)
comment:11 Changed 2 years ago by
- Branch set to u/jdemeyer/why_is_there_src_build_cythonized_build_cythonized_sage_libs_eclib_wrap_cpp
comment:12 Changed 2 years ago by
- Commit set to a5c7119f5488a8b34e09fa037aca9b26b3133d5e
- Status changed from new to needs_review
New commits:
a5c7119 | Put back SAGE_SRC/SAGE_LIB in sage_include_directories()
|
comment:13 follow-up: ↓ 14 Changed 2 years ago by
Just to be clear, this is only an oddity when building in source, right? Does it actually break anything? You mentioned in the ticket something about it breaking dependency checking when modifying some files. Did this fix it?
comment:14 in reply to: ↑ 13 Changed 2 years ago by
Replying to embray:
Just to be clear, this is only an oddity when building in source, right?
What do you mean with "building in source"?
Does it actually break anything? You mentioned in the ticket something about it breaking dependency checking when modifying some files. Did this fix it?
Yes, this is needed to fix dependency checking because the build directory build/cythonize
is also in the list of include directories (see line 421 of src/setup.py
). Certain auto-generated files end up in build/cythonize
, so that's needed. Normal C/C++ source files like sage/cpython/cython_metaclass.h
appear in the source directory $SAGE_SRC
. They are also copied by Cython to build/cythonized
. But the dependency should be on the file in $SAGE_SRC
which means that this directory is needed in the list of include directories and it should come before build/cythonized
in that list.
comment:15 follow-up: ↓ 16 Changed 2 years ago by
With this branch in place, will existing build/cythonized/build/cythonized
directories cause any problems? Is there any reason to clean them up?
comment:16 in reply to: ↑ 15 Changed 2 years ago by
Replying to jhpalmieri:
With this branch in place, will existing
build/cythonized/build/cythonized
directories cause any problems?
I don't think so.
comment:17 Changed 2 years ago by
First: with this branch, the build/cythonized/build/
directory is not created.
Second: without this branch (and is this what Erik was asking?), if I use ./configure --prefix=/path/to/somewhere/else
, then build/cythonized/build
is still created in SAGE_ROOT/src
.
comment:18 Changed 2 years ago by
- Reviewers set to John Palmieri
I'm ready to give this a positive review. I'll wait a day to see if there are any objections.
comment:19 Changed 2 years ago by
- Description modified (diff)
- Summary changed from Why is there src/build/cythonized/build/cythonized/sage/libs/eclib/wrap.cpp to Fix src/build/cythonized/build/cythonized directory
comment:20 Changed 2 years ago by
- Status changed from needs_review to positive_review
comment:21 Changed 2 years ago by
- Branch changed from u/jdemeyer/why_is_there_src_build_cythonized_build_cythonized_sage_libs_eclib_wrap_cpp to a5c7119f5488a8b34e09fa037aca9b26b3133d5e
- Resolution set to fixed
- Status changed from positive_review to closed
This seems to be recent: I don't see it in Sage 8.6.