Opened 9 months ago

Closed 9 months 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) Commit: a5c7119f5488a8b34e09fa037aca9b26b3133d5e
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

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 9 months ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 9 months ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 9 months ago by jhpalmieri

This seems to be recent: I don't see it in Sage 8.6.

comment:4 Changed 9 months ago by jhpalmieri

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 9 months ago by jhpalmieri

It looks to me like the build/cythonized/build/cythonized problem first arises in 8.7.beta2.

comment:6 Changed 9 months ago by jhpalmieri

Could the problem be #27041?

comment:7 Changed 9 months ago by jhpalmieri

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 9 months ago by jdemeyer

  • Priority changed from major to blocker

comment:9 Changed 9 months ago by jdemeyer

  • Description modified (diff)

comment:10 Changed 9 months ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)

comment:11 Changed 9 months ago by jdemeyer

  • Branch set to u/jdemeyer/why_is_there_src_build_cythonized_build_cythonized_sage_libs_eclib_wrap_cpp

comment:12 Changed 9 months ago by jdemeyer

  • Commit set to a5c7119f5488a8b34e09fa037aca9b26b3133d5e
  • Status changed from new to needs_review

New commits:

a5c7119Put back SAGE_SRC/SAGE_LIB in sage_include_directories()

comment:13 follow-up: Changed 9 months ago by embray

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 9 months ago by jdemeyer

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: Changed 9 months ago by jhpalmieri

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 9 months ago by jdemeyer

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 9 months ago by jhpalmieri

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 9 months ago by jhpalmieri

  • 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 9 months ago by jdemeyer

  • 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 9 months ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:21 Changed 9 months ago by vbraun

  • 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
Note: See TracTickets for help on using tickets.