Opened 10 months ago

Closed 9 months ago

Last modified 9 months ago

#31377 closed enhancement (fixed)

./configure --enable-editable

Reported by: mkoeppe Owned by:
Priority: critical Milestone: sage-9.3
Component: build Keywords:
Cc: gh-tobiasdiez, jhpalmieri, chapoton, vdelecroix, isuruf, gh-kliem, tscrim Merged in:
Authors: Tobias Diez, Matthias Koeppe Reviewers: Matthias Koeppe, Tobias Diez, Jonathan Kliem
Report Upstream: N/A Work issues:
Branch: 8b3f390 (Commits, GitHub, GitLab) Commit:
Dependencies: #30770, #30912, #31357, #31365, #31389, #31390 Stopgaps:

Status badges

Description (last modified by gh-tobiasdiez)

This configure switch will install sagelib in "develop" ("editable", "in-place") mode instead of using sagelib's custom incremental build system.

This will clutter the source directory with build artifacts (which are .gitignore'd, of course) but it has the benefit that for changes to Python files, one does not need to run ./sage -b; restarting Sage is enough.

It may also have benefits in certain develop environments that get confused by sagelib's nonstandard build system.

This ticket is based on a subset of the changes in #30371, which developed a version of setup.py suitable for the in-place build. This version is src/setup.py and distinct from build/pkgs/sagelib/src/setup.py. The configure switch switches to this version of the build system.

To test:

./bootstrap
./configure --enable-editable
make build

Then use and test as normal. Verify that local/lib/...site-packages/ no longer contains a copy of sage - instead there is an "egg-link" back to the source directory.

Switching to a standard build system (getting rid of the sage-specific "installation cleaner") will also simplify the next step of the modularization effort (#29705).

Change History (130)

comment:1 Changed 10 months ago by mkoeppe

  • Dependencies set to #30770, #30912, #31357, #31362

comment:2 Changed 10 months ago by mkoeppe

  • Dependencies changed from #30770, #30912, #31357, #31362 to #30770, #30912, #31357

comment:3 Changed 10 months ago by mkoeppe

  • Branch set to u/mkoeppe/__configure___enable_editable

comment:4 Changed 10 months ago by git

  • Commit set to 3dcfbe90735023422e4f6a79594b0034290adf96

Branch pushed to git repo; I updated commit sha1. New commits:

3dcfbe9build/pkgs/sagelib: Implement --enable-editable

comment:5 Changed 10 months ago by git

  • Commit changed from 3dcfbe90735023422e4f6a79594b0034290adf96 to f8ad239bea1847b5008de00b8ac5ebfbced6f122

Branch pushed to git repo; I updated commit sha1. New commits:

f8ad239src/setup.py: Do not look for namespace packages in bin, doc

comment:6 Changed 10 months ago by git

  • Commit changed from f8ad239bea1847b5008de00b8ac5ebfbced6f122 to edbe3d5eb69da6d685365be46c7c3d53d242ae54

Branch pushed to git repo; I updated commit sha1. New commits:

043d3abFixup version files/symlinks
edbe3d5Merge branch 't/31357/fixup_src_version_txt_added_in__30912' into t/31377/__configure___enable_editable

comment:7 Changed 10 months ago by mkoeppe

This almost works for me, except for some errors building modules:

[sagelib-9.3.beta7]     gcc -Wno-unused-result -Wsign-compare -Wunreachable-code -fno-common -dynamic -DNDEBUG -g -fwrapv -O3 -Wall -isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk -O2 -g -march=native -Isage/libs/ntl -I/Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local/lib/python3.9/site-packages/cysignals -Isage/cpython -I/Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/src -I/usr/local/Cellar/python@3.9/3.9.1_8/Frameworks/Python.framework/Versions/3.9/include/python3.9 -I/Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local/lib/python3.9/site-packages/numpy/core/include -I/Users/mkoeppe/s/sage/sage-rebasing/worktree-algebraic-2018-spring/local/include -I/usr/local/Cellar/python@3.9/3.9.1_8/Frameworks/Python.framework/Versions/3.9/include/python3.9 -c sage/libs/ntl/ntl_ZZ.cpp -o build/temp.macosx-10.15-x86_64-3.9/sage/libs/ntl/ntl_ZZ.o
[sagelib-9.3.beta7]     In file included from sage/libs/ntl/error.cpp:1283:
[sagelib-9.3.beta7]     ../local/include/NTL/tools.h:1064:1: error: unknown type name 'constexpr'
[sagelib-9.3.beta7]     constexpr bool Relocate_aux_has_trivial_copy(T*)
[sagelib-9.3.beta7]     ^

This is probably coming from the fact that the C++ std options that sage_build_cython.create_extension uses are not used by the new build system.

comment:8 Changed 10 months ago by git

  • Commit changed from edbe3d5eb69da6d685365be46c7c3d53d242ae54 to 731ff530491af9dfcc46a9d9c226485f2f91caa6

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

804ebd7sage_setup.dpcbuild.AllBuilder: Restrict workaround to macOS
b4ceee5sage_setup.docbuild: In the workaround, do not go through build_many to build serially
7369a3eMerge branch 't/31344/homebrew__docbuild_crashes__libtcl_atforkprepare' into t/31365/add_ntl_to_cython_aliases
ce03e6bsrc/sage/schemes/hyperelliptic_curves/hypellfrob.pyx: Merge distutils directives
32576b4sage.misc.cython: Add NTL aliases, cache result of cython_aliases
6e30e3aUse variables NTL_INCDIR, NTL_LIBDIR in sage_conf, separate from NTL_PREFIX used in sage-build-env-config; set -std=c++11 in NTL_CFLAGS
7b1e27bMerge distutils directives for Cython files using ntl
4d2828dSwitch Cython files that use NTL to language = c++
904a494Merge branch 't/31365/add_ntl_to_cython_aliases' into t/31377/__configure___enable_editable
731ff53Switch some Cython files to c++, add some -std=c++11

comment:9 Changed 10 months ago by mkoeppe

  • Dependencies changed from #30770, #30912, #31357 to #30770, #30912, #31357, #31365
  • Status changed from new to needs_review

I have merged #31365, which adds directives to files using NTL; and added a few more distutils directives so that things don't depend so much on the code in sage_build_cython.create_extension.

comment:10 Changed 10 months ago by mkoeppe

  • Description modified (diff)

comment:11 Changed 10 months ago by mkoeppe

  • Description modified (diff)

comment:12 Changed 10 months ago by mkoeppe

  • Description modified (diff)

comment:13 follow-up: Changed 10 months ago by mkoeppe

This seems to reasonably work well here on macOS. There are a small number of doctest failures.

sage -t --random-seed=0 src/sage/misc/sagedoc.py  # 2 doctests failed
sage -t --random-seed=0 src/sage/misc/sageinspect.py  # 13 doctests failed
sage -t --random-seed=0 src/sage/repl/interpreter.py  # 1 doctest failed
sage -t --random-seed=0 src/sage/categories/primer.py  # 1 doctest failed
sage -t --random-seed=0 src/sage/repl/ipython_tests.py  # 1 doctest failed
sage -t --random-seed=0 src/sage/sets/set_from_iterator.py  # 2 doctests failed
sage -t --random-seed=0 src/sage/interacts/debugger.py  # 2 doctests failed
sage -t --random-seed=0 src/sage/misc/lazy_attribute.pyx  # 1 doctest failed
sage -t --random-seed=0 src/sage/misc/nested_class.pyx  # 1 doctest failed
sage -t --random-seed=0 src/sage/structure/dynamic_class.py  # 1 doctest failed
sage -t --random-seed=0 src/sage/all.py  # 1 doctest failed
sage -t --random-seed=0 src/sage/misc/edit_module.py  # 1 doctest failed
sage -t --random-seed=0 src/sage/cpython/dict_del_by_value.pyx  # 2 doctests failed
sage -t --random-seed=0 src/sage/graphs/graph_decompositions/tree_decomposition.pyx  # 35 doctests failed

The last one is due to the fact that in the editable install all .pyx files, even when the modules are not compiled, are available and used for doctesting.

This will need a new solution to suppress the doctests of such modules - see #30778 (sage.doctest.control: Exclude doctests in files from non-installed distributions).

comment:14 Changed 10 months ago by mkoeppe

  • Cc gh-tobiasdiez jhpalmieri chapoton added

comment:15 Changed 10 months ago by git

  • Commit changed from 731ff530491af9dfcc46a9d9c226485f2f91caa6 to 18df2dc7c27d8e583d5ccdc6acfa39c955b8fa51

Branch pushed to git repo; I updated commit sha1. New commits:

1cb260abuild/pkgs/sagelib/spkg-install: Remove --ignore-installed
36bbd6cbuild/make/Makefile.in (sagelib-clean): Remove .so files in editable installs
993c35cbuild/pkgs/ntl/spkg-configure.m4: Fix up
18df2dcMerge branch 't/31365/add_ntl_to_cython_aliases' into t/31377/__configure___enable_editable

comment:16 Changed 10 months ago by chapoton

please stop adding me in cc

comment:17 Changed 10 months ago by gh-tobiasdiez

So how does one test this?

comment:18 Changed 10 months ago by mkoeppe

  • Description modified (diff)

comment:19 Changed 10 months ago by mkoeppe

I've added quick instructions to the ticket description

comment:20 Changed 10 months ago by mkoeppe

  • Reviewers set to Matthias Koeppe, ...

comment:21 Changed 10 months ago by mkoeppe

  • Priority changed from major to critical

comment:22 Changed 10 months ago by mkoeppe

The installation of the Jupyter kernel is missing - done by sage_setup.command.sage_install

comment:23 in reply to: ↑ 13 Changed 10 months ago by mkoeppe

Replying to mkoeppe:

This seems to reasonably work well here on macOS. There are a small number of doctest failures.

...
sage -t --random-seed=0 src/sage/graphs/graph_decompositions/tree_decomposition.pyx  # 35 doctests failed

The last one is due to the fact that in the editable install all .pyx files, even when the modules are not compiled, are available and used for doctesting.

Wrong guess, this is actually #31389.

comment:24 Changed 10 months ago by mkoeppe

  • Dependencies changed from #30770, #30912, #31357, #31365 to #30770, #30912, #31357, #31365, #31389

comment:25 Changed 10 months ago by git

  • Commit changed from 18df2dc7c27d8e583d5ccdc6acfa39c955b8fa51 to f02963b28d939a7f409914a246d74dce7bfc41d4

Branch pushed to git repo; I updated commit sha1. New commits:

19ca17asrc/sage/graphs/graph_decompositions/tdlib/sage_tdlib.cpp: Move one directory level higher
f02963bMerge branch 't/31389/remove_clash_of_tdlib__with_tdlib_pyx_in__src_sage_graphs_graph_decompositions' into t/31377/__configure___enable_editable

comment:26 Changed 10 months ago by git

  • Commit changed from f02963b28d939a7f409914a246d74dce7bfc41d4 to 2b293f889f827ec5769ad6ca601294284d9a1f24

Branch pushed to git repo; I updated commit sha1. New commits:

2b293f8src/setup.py: Install the jupyter kernel using sage_install

comment:27 Changed 10 months ago by mkoeppe

  • Status changed from needs_review to needs_work

sage.misc.sageinspect is broken in an editable build, which causes the docbuild to fail.

comment:28 Changed 10 months ago by git

  • Commit changed from 2b293f889f827ec5769ad6ca601294284d9a1f24 to 271d42733078659c963550e07970440101a73ab7

Branch pushed to git repo; I updated commit sha1. New commits:

b05e4babuild/make/Makefile.in (sagelib-clean): Remove generated .c and .cpp files
271d427src/setup.py: Fix sage.misc.sageinspect by setting Cython.Compiler.Options.embed_pos_in_docstring

comment:29 Changed 10 months ago by git

  • Commit changed from 271d42733078659c963550e07970440101a73ab7 to f6b2fc4f0f461b2f18aedf49ba238db7a7fdc8b3

Branch pushed to git repo; I updated commit sha1. New commits:

f6b2fc4src/sage/repl/interpreter.py: Fix a doctest for editable installs of sagelib

comment:30 Changed 10 months ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:31 Changed 10 months ago by mkoeppe

  • Dependencies changed from #30770, #30912, #31357, #31365, #31389 to #30770, #30912, #31357, #31365, #31389, #31390

comment:32 Changed 10 months ago by git

  • Commit changed from f6b2fc4f0f461b2f18aedf49ba238db7a7fdc8b3 to e5d47ce731d44f099d635172b34654481eb7e13b

Branch pushed to git repo; I updated commit sha1. New commits:

9a0731asage.interacs.debugger: Remove deprecated module
e5d47ceMerge branch 't/31390/remove_deprecated_sage_interacts_debugger' into t/31377/__configure___enable_editable

comment:33 Changed 10 months ago by mkoeppe

Now the documentation builds OK and also make ptest is fine.

comment:34 Changed 10 months ago by gh-tobiasdiez

  • Status changed from needs_review to needs_work

Can you please include the most recent version of setup.py from #30371. Thanks!

comment:35 follow-up: Changed 10 months ago by mkoeppe

  • Status changed from needs_work to needs_review

The changes there conflict with the changes done and tested here already.

comment:36 Changed 10 months ago by mkoeppe

(Let's please get this in - unless there are problems on this ticket that need solving here. Otherwise please merge this branch into #30371, resolving the conflicts, and let me know)

comment:37 Changed 10 months ago by mkoeppe

  • Cc vdelecroix added

comment:38 in reply to: ↑ 35 ; follow-up: Changed 10 months ago by gh-tobiasdiez

Replying to mkoeppe:

The changes there conflict with the changes done and tested here already.

What conflicts do you mean? I was referring to the improvements to this code:

+# Exclude a few files if the corresponding distribution is not loaded
+optional_packages = ['mcqd', 'bliss', 'tdlib', 'primecount',
+                     'coxeter3', 'fes', 'sirocco', 'meataxe']
+not_installed_packages = [package for package in optional_packages
+                          if not is_package_installed_and_updated(package)]
+
+distributions_to_exclude = [f"sage-{pkg}"
+                            for pkg in not_installed_packages]
+files_to_exclude = filter_cython_sources(SAGE_SRC, distributions_to_exclude)
+
+log.debug(f"files_to_exclude = {files_to_exclude}")
+
+python_packages = find_namespace_packages(where=SAGE_SRC, exclude=['bin', 'doc'])
+log.debug(f"python_packages = {python_packages}")
+
+log.info(f"Discovered Python/Cython sources, time: {(time.time() - t):.2f} seconds.")
+
+# from sage_build_cython:
+import Cython.Compiler.Options
+Cython.Compiler.Options.embed_pos_in_docstring = True
+
+try:
+    log.info("Generating auto-generated sources")
+    from sage_setup.autogen import autogen_all
+    autogen_all()
+
+    from Cython.Build import cythonize
+    from sage.env import cython_aliases, sage_include_directories
+    extensions = cythonize(
+        ["**/*.pyx"],
+        exclude=files_to_exclude,
+        include_path=sage_include_directories(use_sources=True) + ['.'],
+        compile_time_env=compile_time_env_variables(),
+        compiler_directives=compiler_directives(False),
+        aliases=cython_aliases(),
+        create_extension=create_extension,
+        nthreads=4)
+except Exception as exception:
+    log.warn(f"Exception while generating and cythonizing source files: {exception}")
+    extensions = None

This has a few problems/issues:

  • The first few lines still raise an exception if Cython (and maybe other packages) are not installed.
  • You want to auto-generate code before filtering it / excluding parts of it.
  • The exception catch is to general and was meant to only capture module import errors.

All these things are fixed in #30371.

comment:39 Changed 10 months ago by mkoeppe

OK, fine, I'll do the merging.

comment:40 follow-up: Changed 10 months ago by gh-tobiasdiez

Concerning this change:

 # distutils: language = c++
+# distutils: extra_compile_args = -std=c++11

Woudn't it make sense to specify -std=c++11 automatically once language = c++ is set? In fact there is code that looks like it is doing this job in sage_build_cython.

I think also the cython files in libs/singular need similar changes (at least in #30371 errors are thrown otherwise).

comment:41 in reply to: ↑ 40 ; follow-up: Changed 10 months ago by mkoeppe

Replying to gh-tobiasdiez:

Concerning this change:

 # distutils: language = c++
+# distutils: extra_compile_args = -std=c++11

Woudn't it make sense to specify -std=c++11 automatically once language = c++ is set? In fact there is code that looks like it is doing this job in sage_build_cython.

Of course, but you decided not to use the existing code. So I have made the changes that make it work.

comment:42 in reply to: ↑ 38 Changed 10 months ago by mkoeppe

Important to keep in mind that I have created this ticket so that we can ship something in Sage 9.3 that works. So let's please keep focused on what is described here on the ticket description.

Replying to gh-tobiasdiez:

  • The first few lines still raise an exception if Cython (and maybe other packages) are not installed.

Not important for this ticket; we do have Cython.

  • You want to auto-generate code before filtering it / excluding parts of it.

Why? Even after modularization, there will be exactly 1 module that will contain the autogenerated files.

comment:43 in reply to: ↑ 41 Changed 10 months ago by gh-tobiasdiez

Replying to mkoeppe:

Replying to gh-tobiasdiez:

Concerning this change:

 # distutils: language = c++
+# distutils: extra_compile_args = -std=c++11

Woudn't it make sense to specify -std=c++11 automatically once language = c++ is set? In fact there is code that looks like it is doing this job in sage_build_cython.

Of course, but you decided not to use the existing code. So I have made the changes that make it work.

I couldn't make it work with the sage_build_cython command, yes. So changing create_extension in sage_setup/extensions.py would be a better solution then?

comment:44 follow-up: Changed 10 months ago by mkoeppe

Your approach of starting the build system from scratch has certainly been valuable, so that we can see what parts of the code are needed.

In terms of complexity of code, I think adding the small of number of explicit directives to the files may actually be preferable to the more complex code regarding the -std flags in sage_build_cython. Let's try it out as is.

In some follow-up ticket, a more general solution may be needed at some point. Especially when use of c++14/17 becomes more widespread, then fixing things to std=c++11 may no longer be the right thing. Let's do this when it becomes an issue; not now.

comment:45 Changed 10 months ago by mkoeppe

  • Cc isuruf added

comment:46 Changed 10 months ago by mkoeppe

  • Cc gh-kliem added

comment:47 Changed 10 months ago by jhpalmieri

Does this now instead implement ./configure --enable-build-as-root? It sounds like that's related to sudo make; is that what's intended?

comment:48 Changed 10 months ago by mkoeppe

Thanks for catching the typo in the help string.

comment:49 Changed 10 months ago by git

  • Commit changed from e5d47ce731d44f099d635172b34654481eb7e13b to 50f5010883bee3f2fc459848f2dac68d699bce87

Branch pushed to git repo; I updated commit sha1. New commits:

50f5010build/pkgs/sagelib/spkg-configure.m4: Fix help string of --enable-editable

comment:50 follow-ups: Changed 10 months ago by gh-kliem

I like it. Sometimes, I wanted to look into a C/C++ file that cython generated and I just couldn't locate it recently. No it's sitting all there for you.

What vanished though is the progress report, how many files yet need to be compiled. I like this feature, because it tells you whether you should wait on it or whether you should carry on and do something else.

comment:51 in reply to: ↑ 50 Changed 10 months ago by mkoeppe

Replying to gh-kliem:

What vanished though is the progress report, how many files yet need to be compiled. I like this feature, because it tells you whether you should wait on it or whether you should carry on and do something else.

Tobias wrote this as a minimal version using standard setuptools + cython. Let's keep adding additional features of this kind in mind for possible follow-up tickets.

comment:52 Changed 10 months ago by gh-kliem

Ok. Just mentioned that this is missing and I like it back, if possible. I think I would even switch to this, without the counter. Just because it is nice to have the files where you think the should be.

You introduced a trailing whitespace in src/sage_setup/find.py.

comment:53 follow-up: Changed 10 months ago by gh-kliem

Why is it python3 and not $PYTHON?

+if [ "$SAGE_EDITABLE" = yes ]; then
+    time python3 -m pip install --verbose --no-deps --no-index --no-build-isolation --isolated --editable . || exit 1
+else
+    time "$PYTHON" -u setup.py --no-user-cfg build install || exit 1
+fi
+

comment:54 Changed 10 months ago by gh-kliem

You freshly introduced from distutils.command.build_ext import build_ext in /src/sage_setup/command/sage_build_ext_minimal.py. I think this should be replaced by setuptools somehow or at least import setuptools before as to get rid of setuptools.

comment:55 Changed 10 months ago by mkoeppe

We should probably merge #30984 too.

comment:56 in reply to: ↑ 53 Changed 10 months ago by mkoeppe

Replying to gh-kliem:

Why is it python3 and not $PYTHON?

+if [ "$SAGE_EDITABLE" = yes ]; then
+    time python3 -m pip install --verbose --no-deps --no-index --no-build-isolation --isolated --editable . || exit 1
+else
+    time "$PYTHON" -u setup.py --no-user-cfg build install || exit 1
+fi
+

Thanks for spotting this inconsistency. Actually, both should be python3 (since #30731, which replaced sage-python23).

comment:57 Changed 10 months ago by git

  • Commit changed from 50f5010883bee3f2fc459848f2dac68d699bce87 to 8c5e9c3b56070a9a1ef1040555da83365908f286

Branch pushed to git repo; I updated commit sha1. New commits:

8c5e9c3build/pkgs/sagelib/spkg-install: python3, not $PYTHON

comment:58 Changed 10 months ago by git

  • Commit changed from 8c5e9c3b56070a9a1ef1040555da83365908f286 to 0df54f76dee2089b5c14e3db8f5ef8c5a2d04796

Branch pushed to git repo; I updated commit sha1. New commits:

0df54f7src/sage_setup/find.py: Fix whitespace

comment:59 Changed 10 months ago by git

  • Commit changed from 0df54f76dee2089b5c14e3db8f5ef8c5a2d04796 to a63e6157874d8be7e29b078d646936142f4bdcea

Branch pushed to git repo; I updated commit sha1. New commits:

acd7e31sage_setup/command/sage_build_ext_minimal.py: Use setuptools instead of distutils, as in #30984
a63e615src/setup.py: Import setuptools before distutils, as in #30984

comment:60 in reply to: ↑ 50 Changed 10 months ago by gh-tobiasdiez

Replying to gh-kliem:

What vanished though is the progress report, how many files yet need to be compiled. I like this feature, because it tells you whether you should wait on it or whether you should carry on and do something else.

I think a progress bar will not be easy. The new version directly uses the cythonize method, which doesn't seem to have any configuration in this direction, see https://cython.readthedocs.io/en/latest/src/userguide/source_files_and_compilation.html?highlight=cythonize#cythonize-arguments.

comment:61 Changed 10 months ago by mkoeppe

I have created #31406 as a place for follow-up items.

comment:62 in reply to: ↑ 44 Changed 10 months ago by gh-tobiasdiez

Replying to mkoeppe:

In terms of complexity of code, I think adding the small of number of explicit directives to the files may actually be preferable to the more complex code regarding the -std flags in sage_build_cython. Let's try it out as is.

Ok, sounds good to me. But notice that explicitly specifying the std changes the behavior on cygwin (for a "normal" build) since the following code in sage_build_cython is no longer invoked:

            if sys.platform == 'cygwin':
                # Cygwin (particularly newlib, Cygwin's libc) has some bugs
                # with strict ANSI C/C++ in some headers; using the GNU
                # extensions typically fares better:
                # https://trac.sagemath.org/ticket/24192
                if cplusplus:
                    cflags.append("-std=gnu++11")
                else:
                    cflags.append("-std=gnu99")

I don't know if this may have negative side effects.

Last edited 10 months ago by gh-tobiasdiez (previous) (diff)

comment:63 follow-up: Changed 10 months ago by mkoeppe

Thanks for pointing this out; we'll see when we add automated tests for --enable-editable to the Cygwin GH Actions. (in a ticket after #31064 is merged)

comment:64 follow-ups: Changed 10 months ago by gh-tobiasdiez

A few more observations:

  • There is currently no documentation for how to use the inplace install (and why).
  • Running the configure command yields the following warning at the end:
    ....
    config.status: creating directory local/var/lib/sage/installed
    configure: WARNING: unrecognized options: --enable-editable
    configure:
    
        notice: the following SPKGs did not find equivalent system packages
    ...
    
  • My only means to test this on mac is with the github action added in #30371. Admittedly, this is using a lightly different setup, but since the sage inplace install fails, I wanted to point this out. First of all, the files in sage/libs/singular don't specify that they should be compiled with language=c++ and std=c++11. Secondly, there are many errors of the form
     /Library/Developer/CommandLineTools/SDKs/MacOSX10.15.sdk/usr/include/nl_types.h:97:1: error: unknown type name 'nl_catd'
        nl_catd  catopen(const char *, int);
        ^
    

See e.g. https://github.com/tobiasdiez/sage/runs/1893534204?check_suite_focus=true

  • On Linux (with wsl), the make build fails with
    [pplpy-0.8.6] Copying package files from temporary location /mnt/d/Programming/sage/local/var/tmp/sage/build/pplpy-0.8.6/inst to /mnt/d/Programming/sage/local
    [pplpy-0.8.6] Running post-install script for pplpy-0.8.6.
    [pplpy-0.8.6] Traceback (most recent call last):
    [pplpy-0.8.6]   File "/mnt/d/Programming/sage/local/bin/sphinx-build", line 5, in <module>
    [pplpy-0.8.6]     from sphinx.cmd.build import main
    [pplpy-0.8.6]   File "/mnt/d/Programming/sage/local/lib/python3.8/site-packages/sphinx/cmd/build.py", line 25, in <module>
    [pplpy-0.8.6]     from sphinx.application import Sphinx
    [pplpy-0.8.6]   File "/mnt/d/Programming/sage/local/lib/python3.8/site-packages/sphinx/application.py", line 31, in <module>
    [pplpy-0.8.6]     from sphinx.config import Config
    [pplpy-0.8.6]   File "/mnt/d/Programming/sage/local/lib/python3.8/site-packages/sphinx/config.py", line 28, in <module>
    [pplpy-0.8.6]     from sphinx.util.tags import Tags
    [pplpy-0.8.6]   File "/mnt/d/Programming/sage/local/lib/python3.8/site-packages/sphinx/util/tags.py", line 11, in <module>
    [pplpy-0.8.6]     from jinja2 import nodes
    [pplpy-0.8.6]   File "/mnt/d/Programming/sage/local/lib/python3.8/site-packages/jinja2/__init__.py", line 6, in <module>
    [pplpy-0.8.6]     from markupsafe import escape
    [pplpy-0.8.6] ValueError: source code string cannot contain null bytes
    [pplpy-0.8.6] make[5]: *** [Makefile:21: html] Error 1
    [pplpy-0.8.6] cp: cannot stat 'build/html/*': No such file or directory
    

That's probably not related to this ticket, but prevents me from testing it.

Last edited 10 months ago by gh-tobiasdiez (previous) (diff)

comment:65 in reply to: ↑ 63 ; follow-up: Changed 10 months ago by gh-tobiasdiez

Replying to mkoeppe:

Thanks for pointing this out; we'll see when we add automated tests for --enable-editable to the Cygwin GH Actions. (in a ticket after #31064 is merged)

The point is that explicitly specifying std in the cython file also overrides it for the sage-distribution compilation, i.e. the one using the setup.py in build.

comment:66 in reply to: ↑ 64 ; follow-up: Changed 10 months ago by mkoeppe

Replying to gh-tobiasdiez:

  • Running the configure command yields the following warning at the end:
    ....
    config.status: creating directory local/var/lib/sage/installed
    configure: WARNING: unrecognized options: --enable-editable
    

Forgot to run ./bootstrap?

comment:67 in reply to: ↑ 64 Changed 10 months ago by mkoeppe

Replying to gh-tobiasdiez:

  • My only means to test this on mac is with the github action added in #30371.

Let's please not pull a discussion of these experimental GH actions from #30371 into this ticket.

If we add automatic testing, we will do it on the basis of the existing GH actions.

comment:68 in reply to: ↑ 65 Changed 10 months ago by mkoeppe

Replying to gh-tobiasdiez:

Replying to mkoeppe:

Thanks for pointing this out; we'll see when we add automated tests for --enable-editable to the Cygwin GH Actions. (in a ticket after #31064 is merged)

The point is that explicitly specifying std in the cython file also overrides it for the sage-distribution compilation, i.e. the one using the setup.py in build.

Yes, it is a change and we are using portability testing to make sure that it does not break anything on the supported platforms.

comment:69 in reply to: ↑ 64 Changed 10 months ago by mkoeppe

Replying to gh-tobiasdiez:

A few more observations:

  • There is currently no documentation for how to use the inplace install (and why).

Well, I hope we can merge this ticket into the next beta and then ask developers to try it. We can document it in a follow-up ticket.

comment:70 in reply to: ↑ 64 Changed 10 months ago by mkoeppe

Replying to gh-tobiasdiez:

  • On Linux (with wsl), the make build fails with
    [pplpy-0.8.6] Copying package files from temporary location /mnt/d/Programming/sage/local/var/tmp/sage/build/pplpy-0.8.6/inst to /mnt/d/Programming/sage/local
    [pplpy-0.8.6] Running post-install script for pplpy-0.8.6.
    [pplpy-0.8.6]     from markupsafe import escape
    [pplpy-0.8.6] ValueError: source code string cannot contain null bytes
    [pplpy-0.8.6] make[5]: *** [Makefile:21: html] Error 1
    [pplpy-0.8.6] cp: cannot stat 'build/html/*': No such file or directory
    

That's probably not related to this ticket, but prevents me from testing it.

I have not seen this before and I don't think it has anything to with the present ticket. Please check if this also occurs with 9.3.beta7 and open a separate ticket if this problem is reproducible.

comment:71 in reply to: ↑ 66 Changed 10 months ago by gh-tobiasdiez

Replying to mkoeppe:

Forgot to run ./bootstrap?

That fixed it indeed.

The pplpy error occurs also with the recent develop branch, so this is indeed unrelated to these changes. I've opened #31407 for this.

comment:72 Changed 10 months ago by gh-kliem

I'm getting a few doctest failures:

sage -t --long --warn-long 62.2 --random-seed=0 src/sage/repl/interpreter.py
**********************************************************************
File "src/sage/repl/interpreter.py", line 77, in sage.repl.interpreter
Failed example:
    print("dummy line"); shell.run_cell('1/0') # see #25320 for the reason of the `...` and the dummy line in this test
Expected:
    dummy line
    ...
    ZeroDivisionError...Traceback (most recent call last)
    <ipython-input-...> in <module>...
    ----> 1 Integer(1)/Integer(0)
    .../sage/rings/integer.pyx in sage.rings.integer.Integer...div... (.../sage/rings/integer.c:...)()
    ...
    -> ...                  raise ZeroDivisionError("rational division by zero")
       ...            x = <Rational> Rational.__new__(Rational)
       ...            mpq_div_zz(x.value, ....value, (<Integer>right).value)
    <BLANKLINE>
    ZeroDivisionError: rational division by zero
Got:
    dummy line
    ---------------------------------------------------------------------------
    ZeroDivisionError                         Traceback (most recent call last)
    <ipython-input-1-72ac74c5f414> in <module>
    ----> 1 Integer(1)/Integer(0)
    <BLANKLINE>
    /srv/public/kliem/sage/src/sage/rings/integer.pyx in sage.rings.integer.Integer.__truediv__()
       2038         if type(left) is type(right):
       2039             if mpz_sgn((<Integer>right).value) == 0:
    -> 2040                 raise ZeroDivisionError("rational division by zero")
       2041             x = <Rational> Rational.__new__(Rational)
       2042             mpq_div_zz(x.value, (<Integer>left).value, (<Integer>right).value)
    <BLANKLINE>
    ZeroDivisionError: rational division by zero
**********************************************************************
1 item had failures:
   1 of  20 in sage.repl.interpreter
    [141 tests, 1 failure, 1.97 s]
----------------------------------------------------------------------
sage -t --long --warn-long 62.2 --random-seed=0 src/sage/repl/interpreter.py  # 1 doctest failed
----------------------------------------------------------------------
Total time for all tests: 7.4 seconds
    cpu time: 0.6 seconds
    cumulative wall time: 2.0 seconds
kliem@lech:~/localhome/sage$ sage -t --long --random-seed=0 src/sage/stats/hmm/hmm.pyx  # 1 doctest failed
Running doctests with ID 2021-02-17-08-07-40-53dddc3c.
Git branch: test_31377
Using --optional=build,debian,dochtml,memlimit,pip,sage,sage_spkg
Doctesting 1 file.
sage -t --long --warn-long 62.2 --random-seed=0 src/sage/stats/hmm/hmm.pyx
**********************************************************************
File "src/sage/stats/hmm/hmm.pyx", line 1234, in sage.stats.hmm.hmm.DiscreteHiddenMarkovModel.baum_welch
Failed example:
    m.baum_welch(v,fix_emissions=True)
Expected:
    (-66.98630856918774, 100)
Got:
    (-66.98630856918776, 100)
**********************************************************************
1 item had failures:
   1 of  14 in sage.stats.hmm.hmm.DiscreteHiddenMarkovModel.baum_welch
    [121 tests, 1 failure, 0.32 s]
----------------------------------------------------------------------
sage -t --long --warn-long 62.2 --random-seed=0 src/sage/stats/hmm/hmm.pyx  # 1 doctest failed
----------------------------------------------------------------------
Total time for all tests: 0.4 seconds
    cpu time: 0.3 seconds
    cumulative wall time: 0.3 seconds
kliem@lech:~/localhome/sage$ sage -t --long --random-seed=0 src/sage_setup/find.py  # 6 doctests failed
Running doctests with ID 2021-02-17-08-07-42-447cd574.
Git branch: test_31377
Using --optional=build,debian,dochtml,memlimit,pip,sage,sage_spkg
Doctesting 1 file.
sage -t --long --warn-long 62.2 --random-seed=0 src/sage_setup/find.py
**********************************************************************
File "src/sage_setup/find.py", line 199, in sage_setup.find.filter_cython_sources
Failed example:
    'sage/graphs/graph_decompositions/tdlib.pyx' in cython_modules
Expected:
    True
Got:
    False
**********************************************************************
File "src/sage_setup/find.py", line 264, in sage_setup.find.find_extra_files
Failed example:
    extras["sage/ext/interpreters"]
Expected:
    ['.../src/sage/ext/interpreters/wrapper_cdf.pxd', ...wrapper_cdf.h...]
Got:
    ['/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_py.pxd',
     '/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_cdf.pxd',
     '/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_el.pyx',
     '/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_rdf.pyx',
     '/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_cc.pyx',
     '/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_rr.pxd',
     '/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_py.pyx',
     '/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_cdf.pyx',
     '/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_rr.pyx',
     '/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_rdf.pxd',
     '/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_cc.pxd',
     '/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_el.pxd']
**********************************************************************
File "src/sage_setup/find.py", line 324, in sage_setup.find.installed_files_by_module
Failed example:
    (f,) = files_by_module['sage.structure.sage_object']; f
Exception raised:
    Traceback (most recent call last):
      File "/srv/public/kliem/sage/src/sage/doctest/forker.py", line 714, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/srv/public/kliem/sage/src/sage/doctest/forker.py", line 1133, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage_setup.find.installed_files_by_module[5]>", line 1, in <module>
        (f,) = files_by_module['sage.structure.sage_object']; f
    ValueError: not enough values to unpack (expected 1, got 0)
**********************************************************************
File "src/sage_setup/find.py", line 326, in sage_setup.find.installed_files_by_module
Failed example:
    (f1, f2) = sorted(files_by_module['sage.structure'])
Exception raised:
    Traceback (most recent call last):
      File "/srv/public/kliem/sage/src/sage/doctest/forker.py", line 714, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/srv/public/kliem/sage/src/sage/doctest/forker.py", line 1133, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage_setup.find.installed_files_by_module[6]>", line 1, in <module>
        (f1, f2) = sorted(files_by_module['sage.structure'])
    ValueError: not enough values to unpack (expected 2, got 0)
**********************************************************************
File "src/sage_setup/find.py", line 327, in sage_setup.find.installed_files_by_module
Failed example:
    f1
Exception raised:
    Traceback (most recent call last):
      File "/srv/public/kliem/sage/src/sage/doctest/forker.py", line 714, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/srv/public/kliem/sage/src/sage/doctest/forker.py", line 1133, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage_setup.find.installed_files_by_module[7]>", line 1, in <module>
        f1
    NameError: name 'f1' is not defined
**********************************************************************
File "src/sage_setup/find.py", line 329, in sage_setup.find.installed_files_by_module
Failed example:
    f2
Exception raised:
    Traceback (most recent call last):
      File "/srv/public/kliem/sage/src/sage/doctest/forker.py", line 714, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/srv/public/kliem/sage/src/sage/doctest/forker.py", line 1133, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage_setup.find.installed_files_by_module[8]>", line 1, in <module>
        f2
    NameError: name 'f2' is not defined
**********************************************************************
3 items had failures:
   1 of   7 in sage_setup.find.filter_cython_sources
   1 of   7 in sage_setup.find.find_extra_files
   4 of  11 in sage_setup.find.installed_files_by_module
    [42 tests, 6 failures, 0.40 s]
----------------------------------------------------------------------
sage -t --long --warn-long 62.2 --random-seed=0 src/sage_setup/find.py  # 6 doctests failed
----------------------------------------------------------------------
Total time for all tests: 0.4 seconds
    cpu time: 0.4 seconds
    cumulative wall time: 0.4 seconds
kliem@lech:~/localhome/sage$ sage -t --long --random-seed=0 src/sage/tests/cmdline.py  # 3 doctests failed
Running doctests with ID 2021-02-17-08-07-44-7353eba5.
Git branch: test_31377
Using --optional=build,debian,dochtml,memlimit,pip,sage,sage_spkg
Doctesting 1 file.
sage -t --long --warn-long 62.2 --random-seed=0 src/sage/tests/cmdline.py
**********************************************************************
File "src/sage/tests/cmdline.py", line 586, in sage.tests.cmdline.test_executable
Failed example:
    out.startswith("3.")
Expected:
    True
Got:
    False
**********************************************************************
File "src/sage/tests/cmdline.py", line 588, in sage.tests.cmdline.test_executable
Failed example:
    err
Expected:
    ''
Got:
    '/srv/public/kliem/sage/src/bin/sage: line 588: exec: sqlite3: not found\n'
**********************************************************************
File "src/sage/tests/cmdline.py", line 590, in sage.tests.cmdline.test_executable
Failed example:
    ret
Expected:
    0
Got:
    127
**********************************************************************
1 item had failures:
   3 of 231 in sage.tests.cmdline.test_executable
    [230 tests, 3 failures, 21.87 s]
----------------------------------------------------------------------
sage -t --long --warn-long 62.2 --random-seed=0 src/sage/tests/cmdline.py  # 3 doctests failed
----------------------------------------------------------------------

The test in src/sage/tests/cmdline.py is just there for completeness and can be ignored.

comment:73 Changed 10 months ago by jhpalmieri

I got failures in repl/interpreter.py and sage_setup/find.py.

comment:74 Changed 10 months ago by git

  • Commit changed from a63e6157874d8be7e29b078d646936142f4bdcea to 78a1587c275e5528a9e020f926302851647da6d4

Branch pushed to git repo; I updated commit sha1. New commits:

78a1587src/setup.py: Add . to sys.path so that pyproject.toml from #30913 does not break the build

comment:75 Changed 10 months ago by git

  • Commit changed from 78a1587c275e5528a9e020f926302851647da6d4 to ccce09e2c8cc7fa802ed18450778096f76364da7

Branch pushed to git repo; I updated commit sha1. New commits:

ccce09ebuild/pkgs/sagelib/spkg-install [SAGE_EDITABLE=yes]: Uninstall distutils-installed sagelib

comment:76 Changed 10 months ago by git

  • Commit changed from ccce09e2c8cc7fa802ed18450778096f76364da7 to 5a918778123e677c2209425bdc9905b45699ab6e

Branch pushed to git repo; I updated commit sha1. New commits:

5a91877sage_setup.find: Fix filter_cython_sources doctests

comment:77 Changed 10 months ago by mkoeppe

The remaining failures in sage_setup/find.py are now all about installed_files_by_module; I'll have to see what to do about them (this function is only needed for the old build system).

The failure in sage/repl/interpreter.py seems to indicate that the C source information (sage/rings/integer.c) is missing.

comment:78 Changed 10 months ago by git

  • Commit changed from 5a918778123e677c2209425bdc9905b45699ab6e to e272614fd26ceb3992754a014099fc644d470f06

Branch pushed to git repo; I updated commit sha1. New commits:

66fa8c0src/setup.py: Add . to sys.path earlier
e272614src/setup.py: Pass gdb_debug to cythonize, as in sage_build_cython

comment:79 Changed 10 months ago by git

  • Commit changed from e272614fd26ceb3992754a014099fc644d470f06 to 67072f34146e7ed547f241854755a462660b53cf

Branch pushed to git repo; I updated commit sha1. New commits:

67072f3src/setup.py: Use include, not exclude in find_namespace_packages to get top_level info right

comment:80 Changed 10 months ago by git

  • Commit changed from 67072f34146e7ed547f241854755a462660b53cf to 0bcf8a40008300ad2fca24310a052b0b2fc6b510

Branch pushed to git repo; I updated commit sha1. New commits:

0bcf8a4build/pkgs/sagelib/spkg-install: When switching from editable install to traditional install, remove the egg-link

comment:81 Changed 10 months ago by git

  • Commit changed from 0bcf8a40008300ad2fca24310a052b0b2fc6b510 to 981124cd020eaa4127a9ae0e6c9009269e45a2ff

Branch pushed to git repo; I updated commit sha1. New commits:

981124csage_setup.find.installed_files_by_module: Make doctest work for editable installs too

comment:82 Changed 10 months ago by mkoeppe

This fixes the remaining doctests for me.

comment:83 Changed 10 months ago by gh-kliem

Not for me:

I still get src/sage/tests/cmdline.py, which can be ignored.

On top:

kliem@lech:~/localhome/sage$ sage -t --long --warn-long 62.2 --random-seed=0 src/sage/repl/interpreter.py
Running doctests with ID 2021-02-18-07-50-51-5ae2b230.
Git branch: test_31377
Using --optional=build,debian,dochtml,memlimit,pip,sage,sage_spkg
Doctesting 1 file.
sage -t --long --warn-long 62.2 --random-seed=0 src/sage/repl/interpreter.py
**********************************************************************
File "src/sage/repl/interpreter.py", line 77, in sage.repl.interpreter
Failed example:
    print("dummy line"); shell.run_cell('1/0') # see #25320 for the reason of the `...` and the dummy line in this test
Expected:
    dummy line
    ...
    ZeroDivisionError...Traceback (most recent call last)
    <ipython-input-...> in <module>...
    ----> 1 Integer(1)/Integer(0)
    .../sage/rings/integer.pyx in sage.rings.integer.Integer...div... (.../sage/rings/integer.c:...)()
    ...
    -> ...                  raise ZeroDivisionError("rational division by zero")
       ...            x = <Rational> Rational.__new__(Rational)
       ...            mpq_div_zz(x.value, ....value, (<Integer>right).value)
    <BLANKLINE>
    ZeroDivisionError: rational division by zero
Got:
    dummy line
    ---------------------------------------------------------------------------
    ZeroDivisionError                         Traceback (most recent call last)
    <ipython-input-1-72ac74c5f414> in <module>
    ----> 1 Integer(1)/Integer(0)
    <BLANKLINE>
    /srv/public/kliem/sage/src/sage/rings/integer.pyx in sage.rings.integer.Integer.__truediv__()
       2038         if type(left) is type(right):
       2039             if mpz_sgn((<Integer>right).value) == 0:
    -> 2040                 raise ZeroDivisionError("rational division by zero")
       2041             x = <Rational> Rational.__new__(Rational)
       2042             mpq_div_zz(x.value, (<Integer>left).value, (<Integer>right).value)
    <BLANKLINE>
    ZeroDivisionError: rational division by zero
**********************************************************************
1 item had failures:
   1 of  20 in sage.repl.interpreter
    [141 tests, 1 failure, 1.94 s]
----------------------------------------------------------------------
sage -t --long --warn-long 62.2 --random-seed=0 src/sage/repl/interpreter.py  # 1 doctest failed
----------------------------------------------------------------------
Total time for all tests: 7.3 seconds
    cpu time: 0.6 seconds
    cumulative wall time: 1.9 seconds
kliem@lech:~/localhome/sage$ sage -t --long --warn-long 62.2 --random-seed=0 src/sage/stats/hmm/hmm.pyx
Running doctests with ID 2021-02-18-07-51-08-57678760.
Git branch: test_31377
Using --optional=build,debian,dochtml,memlimit,pip,sage,sage_spkg
Doctesting 1 file.
sage -t --long --warn-long 62.2 --random-seed=0 src/sage/stats/hmm/hmm.pyx
**********************************************************************
File "src/sage/stats/hmm/hmm.pyx", line 1234, in sage.stats.hmm.hmm.DiscreteHiddenMarkovModel.baum_welch
Failed example:
    m.baum_welch(v,fix_emissions=True)
Expected:
    (-66.98630856918774, 100)
Got:
    (-66.98630856918776, 100)
**********************************************************************
1 item had failures:
   1 of  14 in sage.stats.hmm.hmm.DiscreteHiddenMarkovModel.baum_welch
    [121 tests, 1 failure, 0.33 s]
----------------------------------------------------------------------
sage -t --long --warn-long 62.2 --random-seed=0 src/sage/stats/hmm/hmm.pyx  # 1 doctest failed
----------------------------------------------------------------------
Total time for all tests: 0.4 seconds
    cpu time: 0.3 seconds
    cumulative wall time: 0.3 seconds
kliem@lech:~/localhome/sage$ sage -t --long --warn-long 62.2 --random-seed=0 src/sage_setup/find.py
Running doctests with ID 2021-02-18-07-51-21-d3281c6c.
Git branch: test_31377
Using --optional=build,debian,dochtml,memlimit,pip,sage,sage_spkg
Doctesting 1 file.
sage -t --long --warn-long 62.2 --random-seed=0 src/sage_setup/find.py
**********************************************************************
File "src/sage_setup/find.py", line 264, in sage_setup.find.find_extra_files
Failed example:
    extras["sage/ext/interpreters"]
Expected:
    ['.../src/sage/ext/interpreters/wrapper_cdf.pxd', ...wrapper_cdf.h...]
Got:
    ['/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_py.pxd',
     '/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_cdf.pxd',
     '/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_el.pyx',
     '/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_rdf.pyx',
     '/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_cc.pyx',
     '/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_rr.pxd',
     '/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_py.pyx',
     '/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_cdf.pyx',
     '/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_rr.pyx',
     '/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_rdf.pxd',
     '/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_cc.pxd',
     '/srv/public/kliem/sage/src/sage/ext/interpreters/wrapper_el.pxd']
**********************************************************************
1 item had failures:
   1 of   7 in sage_setup.find.find_extra_files
    [41 tests, 1 failure, 0.47 s]
----------------------------------------------------------------------
sage -t --long --warn-long 62.2 --random-seed=0 src/sage_setup/find.py  # 1 doctest failed
----------------------------------------------------------------------
Total time for all tests: 0.5 seconds
    cpu time: 0.5 seconds
    cumulative wall time: 0.5 seconds

comment:84 Changed 10 months ago by gh-kliem

find_extra_files does exactly what the documentation claims it does, it lists all pyx, pxd, pxi files.

comment:85 Changed 10 months ago by mkoeppe

After make sagelib-clean sagelib I can reproduce this error too. the .h files are missing in the list.

comment:86 follow-up: Changed 10 months ago by mkoeppe

My guess is that the doctest failure in src/sage/repl/interpreter.py will go away if you do make sagelib-clean

comment:87 Changed 10 months ago by gh-kliem

The missing .h files are by design. This function gives only .h. files in cythonized, which is pointless now. I don't know what would be the best way to deal with this.

Can you fix the precision error in src/sage/stats/hmm/hmm.pyx as well (# abs tol 1e-13). It seems pointless to have an extra ticket for this.

comment:88 follow-up: Changed 10 months ago by gh-kliem

Ran make sagelib-clean sagelib and make build and it still persists:

kliem@lech:~/localhome/sage$ sage -t --long --warn-long 62.2 --random-seed=0 src/sage/repl/interpreter.py
Running doctests with ID 2021-02-18-08-59-13-9182572c.
Git branch: test_31377
Using --optional=build,debian,dochtml,memlimit,pip,sage,sage_spkg
Doctesting 1 file.
sage -t --long --warn-long 62.2 --random-seed=0 src/sage/repl/interpreter.py
**********************************************************************
File "src/sage/repl/interpreter.py", line 77, in sage.repl.interpreter
Failed example:
    print("dummy line"); shell.run_cell('1/0') # see #25320 for the reason of the `...` and the dummy line in this test
Expected:
    dummy line
    ...
    ZeroDivisionError...Traceback (most recent call last)
    <ipython-input-...> in <module>...
    ----> 1 Integer(1)/Integer(0)
    .../sage/rings/integer.pyx in sage.rings.integer.Integer...div... (.../sage/rings/integer.c:...)()
    ...
    -> ...                  raise ZeroDivisionError("rational division by zero")
       ...            x = <Rational> Rational.__new__(Rational)
       ...            mpq_div_zz(x.value, ....value, (<Integer>right).value)
    <BLANKLINE>
    ZeroDivisionError: rational division by zero
Got:
    dummy line
    ---------------------------------------------------------------------------
    ZeroDivisionError                         Traceback (most recent call last)
    <ipython-input-1-72ac74c5f414> in <module>
    ----> 1 Integer(1)/Integer(0)
    <BLANKLINE>
    /srv/public/kliem/sage/src/sage/rings/integer.pyx in sage.rings.integer.Integer.__truediv__()
       2038         if type(left) is type(right):
       2039             if mpz_sgn((<Integer>right).value) == 0:
    -> 2040                 raise ZeroDivisionError("rational division by zero")
       2041             x = <Rational> Rational.__new__(Rational)
       2042             mpq_div_zz(x.value, (<Integer>left).value, (<Integer>right).value)
    <BLANKLINE>
    ZeroDivisionError: rational division by zero
**********************************************************************
1 item had failures:
   1 of  20 in sage.repl.interpreter
    [141 tests, 1 failure, 2.90 s]
----------------------------------------------------------------------
sage -t --long --warn-long 62.2 --random-seed=0 src/sage/repl/interpreter.py  # 1 doctest failed
----------------------------------------------------------------------
Total time for all tests: 8.4 seconds
    cpu time: 0.6 seconds
    cumulative wall time: 2.9 seconds

comment:89 Changed 10 months ago by gh-kliem

I mean it makes completely sense that the cythonized file is no longer written in parenthesis, because it is in the obvious location. So no need to specify this anymore.

comment:90 in reply to: ↑ 86 ; follow-up: Changed 10 months ago by gh-tobiasdiez

Replying to mkoeppe:

My guess is that the doctest failure in src/sage/repl/interpreter.py will go away if you do make sagelib-clean

I think this problem comes from the fact that currently find_packages is called before the autogeneration. Thus, the auto-generated files are not included in the cythonization step. This is fixed in the setup.py in #30371.

comment:91 Changed 10 months ago by gh-kliem

I think we can leave those failures for later fix, as long as we take care of them at some point.

None of the doctest failures is serious.

I guess this isn't ready yet for being used for patchbots.

comment:92 in reply to: ↑ 88 Changed 10 months ago by mkoeppe

Replying to gh-kliem:

Ran make sagelib-clean sagelib and make build and it still persists:

I'm pretty sure I fixed this with e272614; perhaps make sagelib-clean still does not clean enough?

comment:93 in reply to: ↑ 90 Changed 10 months ago by mkoeppe

Replying to gh-tobiasdiez:

Replying to mkoeppe:

My guess is that the doctest failure in src/sage/repl/interpreter.py will go away if you do make sagelib-clean

I think this problem comes from the fact that currently find_packages is called before the autogeneration. Thus, the auto-generated files are not included in the cythonization step. This is fixed in the setup.py in #30371.

No, calling autogen earlier does not help.

comment:94 Changed 10 months ago by gh-tobiasdiez

  • Description modified (diff)

comment:95 follow-up: Changed 10 months ago by vdelecroix

For changes in Python source, one might even use importlib and not restart sage at all!

comment:96 Changed 10 months ago by git

  • Commit changed from 981124cd020eaa4127a9ae0e6c9009269e45a2ff to 8b5d51bd0ff4823972f95f0728eba485d3443afe

Branch pushed to git repo; I updated commit sha1. New commits:

8b5d51bsrc/sage_setup: Generalize doctests to editable install using new function _cythonized_dir

comment:97 in reply to: ↑ 95 Changed 10 months ago by mkoeppe

Replying to vdelecroix:

For changes in Python source, one might even use importlib and not restart sage at all!

True, but let's not document this -- too many possible pitfalls related to class redefinitions

comment:98 Changed 10 months ago by mkoeppe

(Too bad that Python does not have UPDATE-INSTANCE-FOR-REDEFINED-CLASS http://clhs.lisp.se/Body/f_upda_1.htm)

Last edited 10 months ago by mkoeppe (previous) (diff)

comment:99 Changed 10 months ago by mkoeppe

Ready for another round of testing. (I won't do the unrelated hmm.pyx doctest fix on this ticket)

comment:100 Changed 10 months ago by gh-kliem

After deleting everything and reinstalling it, I still get the failure in src/sage/repl/interpreter.py. The find.py thing is gone now.

comment:101 Changed 10 months ago by mkoeppe

I'm getting the src/sage/repl/interpreter.py failure now too again... any help with tracking this down is welcome...

comment:102 Changed 10 months ago by mkoeppe

Or we can leave this issue for the follow-up ticket #31406, that would be fine with me too.

comment:103 follow-up: Changed 10 months ago by jhpalmieri

If we're to believe the doctest language ("Check that Cython source code appears in tracebacks"), then this is functioning properly, it's just missing "(.../cythonized/sage/rings/integer.c:...)()". Can we just replace that part of the doctest with "...", or is it important?

  • src/sage/repl/interpreter.py

    diff --git a/src/sage/repl/interpreter.py b/src/sage/repl/interpreter.py
    index befa75fc38..3bd894851d 100644
    a b Check that Cython source code appears in tracebacks:: 
    8080    ZeroDivisionError...Traceback (most recent call last)
    8181    <ipython-input-...> in <module>...
    8282    ----> 1 Integer(1)/Integer(0)
    83     .../sage/rings/integer.pyx in sage.rings.integer.Integer...div... (.../sage/rings/integer.c:...)()
     83    .../sage/rings/integer.pyx in sage.rings.integer.Integer...div...
    8484    ...
    8585    -> ...                  raise ZeroDivisionError("rational division by zero")
    8686       ....:            x = <Rational> Rational.__new__(Rational)

By the way, git status tells me

Untracked files:
  (use "git add <file>..." to include in what will be committed)
	src/cython_debug/

comment:104 Changed 10 months ago by gh-tobiasdiez

I had also these untracked files in src/cython_debug.

The tests seem to pass in general for me. However, they need a lot of ram, like > 25GB which sometimes leads to a crash of the system. In particular, the tests around sage/plot seem to very memeroy intensive (but the memory doesn't go down afterwards). Is this normal? I could also reproduce this on the github actions in #30371 which also crash reproducible after about 2 hours of doctesting.

comment:105 Changed 10 months ago by git

  • Commit changed from 8b5d51bd0ff4823972f95f0728eba485d3443afe to eb631b23bc8f207a57da1550cf799de86983bd94

Branch pushed to git repo; I updated commit sha1. New commits:

89969bf.gitignore: Ignore src/cython_debug
eb631b2build/make/Makefile.in: Remove src/cython_debug

comment:106 in reply to: ↑ 103 Changed 10 months ago by mkoeppe

  • Cc tscrim added

Replying to jhpalmieri:

it's just missing "(.../cythonized/sage/rings/integer.c:...)()". Can we just replace that part of the doctest with "...", or is it important?

I don't know if this information is important, perhaps someone who programs Cython can tell us?

comment:107 Changed 10 months ago by gh-kliem

Using git blame, this doctest was introduced in commit 6e7ad656ba3307ff67949e251550c101758f9963. In this commit Jeroen Demeyer added a IPython patch. Of course this patch has long merged upstream and has now changed several times and I failed to located the place, where things are happening.

However, things are out of our control.

I think the reason why "(.../cythonized/sage/rings/integer.c:...)()" appears is that the cythonized file is located at a different location then the source file. So this might be important, when you are debugging. However, if the cythonized files are no longer hidden somewhere else, then it appears pointless to print this information and this is probably why IPython doesn't do it. I mean the file is right next to the other file. It is redundant information in this case.

Long story short, I think this can go. It is an IPython feature of which we do not have control anyway. What is important, is that the corresponding pyx file appears correctly, which is the case.

comment:108 Changed 10 months ago by git

  • Commit changed from eb631b23bc8f207a57da1550cf799de86983bd94 to daec3e803e8f6a6de8e717459c31b2b7de7a8906

Branch pushed to git repo; I updated commit sha1. New commits:

daec3e8src/sage/repl/interpreter.py: Do not test that IPython displays the location of the Cython-generated .c file

comment:109 Changed 10 months ago by mkoeppe

Thanks for investigating. This is sufficiently convincing, so I have removed the test.

comment:110 Changed 10 months ago by gh-kliem

Ok, all tests pass now (except for those two unrelated tests).

comment:111 Changed 10 months ago by mkoeppe

Any further concerns, or can we get this in?

comment:112 Changed 10 months ago by gh-tobiasdiez

  • Reviewers changed from Matthias Koeppe, ... to Matthias Koeppe, Tobias Diez, ...

From my side this looks good.

comment:113 Changed 10 months ago by gh-kliem

I'm happy with it, but didn't have the time to understand everything yet.

comment:114 Changed 9 months ago by gh-kliem

  • Reviewers changed from Matthias Koeppe, Tobias Diez, ... to Matthias Koeppe, Tobias Diez, Jonathan Kliem
  • Status changed from needs_review to positive_review

LGTM.

I don't know if setting nthreads=4 for cythonizing is perfect. Maybe this could be picked up somewhere in a follow up ticket. E.g. if the environment variable MAKE is set to make ... -jN ... we should probably respect this.

comment:115 Changed 9 months ago by gh-tobiasdiez

Thanks! I've added your suggestion to #31406.

comment:116 Changed 9 months ago by mkoeppe

Thanks for the review!

comment:117 Changed 9 months ago by git

  • Commit changed from daec3e803e8f6a6de8e717459c31b2b7de7a8906 to 8b3f3905c884f86293f103fe5bbb7a78c3d7ce02
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

8b3f390Merge tag '9.3.beta8' into t/31377/__configure___enable_editable

comment:118 Changed 9 months ago by mkoeppe

  • Status changed from needs_review to positive_review

comment:119 Changed 9 months ago by vbraun

  • Branch changed from u/mkoeppe/__configure___enable_editable to 8b3f3905c884f86293f103fe5bbb7a78c3d7ce02
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:120 Changed 9 months ago by fbissey

  • Commit 8b3f3905c884f86293f103fe5bbb7a78c3d7ce02 deleted

I finally tracked this ticket has the breaking point of the way I build the documentation in sage-on-gentoo. This ticket not #30010.

Unlike the current philosophy pursued by vanilla sage in distro we usually build documentation before installing. There are philosophy discussions to be had about the approaches. I am sometimes not even completely sure how things work. But here the reason is simple, before this ticket, all the sources where copied under build/lib/ during the build process. After this ticket only the final compiled cython modules are found under build/lib/. Historically during the documentation building in sage-on-gentoo the just compiled copy of sagelib under build/lib/ was used. This is not possible anymore.

Is there a switch to pass to setup.py that would copy the source in build/lib after this ticket?

comment:121 Changed 9 months ago by mkoeppe

I don't think this ticket changes anything if you don't use configure --enable-editable.

comment:122 Changed 9 months ago by mkoeppe

Just be sure to use build/pkgs/sagelib/src/setup.py, not src/setup.py. This builds in build/pkgs/sagelib/src/build/.

comment:123 follow-up: Changed 9 months ago by fbissey

I am working on top of Volker's merging branch to figure issues before they hit me when a beta is released. When the beta, rc or final release arrive I usually have worked out the issues. I have bisected the change of behavior to this ticket. By the sound of it, the change of behavior is accidental and you have no idea or care about it.

On a side note since you are mentioning it, I am not currently using build/pkgs/sagelib/src/setup.py, links to sources have proved troublesome so far. I am hoping to switch to that at some point when some packaging dust settles. But I should point out that it severely hamper my ability to build the documentation in the same package. Which is a point our current philosophy of building sage are getting divergent. But those are discussions we should have in another space than this ticket.

comment:124 in reply to: ↑ 123 ; follow-up: Changed 9 months ago by mkoeppe

Replying to fbissey:

By the sound of it, the change of behavior is accidental

No. Since #30779, the Sage distribution has no longer used src/setup.py. It switched to using build/pkgs/sagelib/src/setup.py. You should do the same.

comment:125 Changed 9 months ago by mkoeppe

By the way, if you want to discuss philosophy of docbuilding, --> #31356, #29868.

comment:126 in reply to: ↑ 124 Changed 9 months ago by fbissey

Replying to mkoeppe:

Replying to fbissey:

By the sound of it, the change of behavior is accidental

No. Since #30779, the Sage distribution has no longer used src/setup.py. It switched to using build/pkgs/sagelib/src/setup.py. You should do the same.

I hadn't realised how much they diverged from each other. It feels like some tickets patch one but not the other and so on. Regardless of my issues and divergence in philosophy, as I called it, if the version in src/setup.py shouldn't be used or developed against we need to remove it and quickly.

comment:127 Changed 9 months ago by mkoeppe

No, as of this ticket, it's used for ./configure --enable-editable.

comment:128 follow-up: Changed 9 months ago by fbissey

I think I am starting to get the picture. So those large looking differences are on purpose?

comment:129 Changed 9 months ago by jhpalmieri

It sounds like it would be good to add some explanatory comments at the top of src/setup.py (not by me, since I don't understand the situation).

comment:130 in reply to: ↑ 128 Changed 9 months ago by mkoeppe

Replying to fbissey:

So those large looking differences are on purpose?

Yes! And yes, we'll have to add some documentation. I'll do this in #31386 (which will reduce the amount of duplication between the two setup.py files); this ticket is waiting for another big build system ticket, #30913.

Note: See TracTickets for help on using tickets.