Skip to content
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

./configure --enable-editable #31377

Closed
mkoeppe opened this issue Feb 10, 2021 · 152 comments
Closed

./configure --enable-editable #31377

mkoeppe opened this issue Feb 10, 2021 · 152 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 10, 2021

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).

Depends on #30770
Depends on #30912
Depends on #31357
Depends on #31365
Depends on #31389
Depends on #31390

CC: @tobiasdiez @jhpalmieri @fchapoton @videlec @isuruf @kliem @tscrim

Component: build

Author: Tobias Diez, Matthias Koeppe

Branch: 8b3f390

Reviewer: Matthias Koeppe, Tobias Diez, Jonathan Kliem

Issue created by migration from https://trac.sagemath.org/ticket/31377

@mkoeppe mkoeppe added this to the sage-9.3 milestone Feb 10, 2021
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 10, 2021

Dependencies: #30770, #30912, #31357, #31362

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 10, 2021

Changed dependencies from #30770, #30912, #31357, #31362 to #30770, #30912, #31357

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 10, 2021

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 10, 2021

Commit: 3dcfbe9

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 10, 2021

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 10, 2021

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 10, 2021

Changed commit from 3dcfbe9 to f8ad239

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 10, 2021

Changed commit from f8ad239 to edbe3d5

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 10, 2021

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 10, 2021

comment:7

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.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 11, 2021

Changed commit from edbe3d5 to 731ff53

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 11, 2021

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 11, 2021

Changed dependencies from #30770, #30912, #31357 to #30770, #30912, #31357, #31365

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 11, 2021

comment:9

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.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 11, 2021

comment:13

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).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 11, 2021

Changed commit from 731ff53 to 18df2dc

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 11, 2021

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

@fchapoton
Copy link
Contributor

comment:16

please stop adding me in cc

@tobiasdiez
Copy link
Contributor

comment:17

So how does one test this?

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 11, 2021

comment:19

I've added quick instructions to the ticket description

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 11, 2021

Reviewer: Matthias Koeppe, ...

@tobiasdiez
Copy link
Contributor

Changed reviewer from Matthias Koeppe, ... to Matthias Koeppe, Tobias Diez, ...

@kliem
Copy link
Contributor

kliem commented Feb 20, 2021

comment:113

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

@kliem
Copy link
Contributor

kliem commented Feb 24, 2021

Changed reviewer from Matthias Koeppe, Tobias Diez, ... to Matthias Koeppe, Tobias Diez, Jonathan Kliem

@kliem
Copy link
Contributor

kliem commented Feb 24, 2021

comment:114

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.

@tobiasdiez
Copy link
Contributor

comment:115

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 24, 2021

comment:116

Thanks for the review!

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2021

Changed commit from daec3e8 to 8b3f390

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 7, 2021

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

@vbraun
Copy link
Member

vbraun commented Mar 9, 2021

Changed branch from u/mkoeppe/__configure___enable_editable to 8b3f390

@kiwifb
Copy link
Member

kiwifb commented Mar 9, 2021

comment:120

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?

@kiwifb
Copy link
Member

kiwifb commented Mar 9, 2021

Changed commit from 8b3f390 to none

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 9, 2021

comment:121

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 9, 2021

comment:122

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

@kiwifb
Copy link
Member

kiwifb commented Mar 9, 2021

comment:123

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 9, 2021

comment:124

Replying to @kiwifb:

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 9, 2021

comment:125

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

@kiwifb
Copy link
Member

kiwifb commented Mar 9, 2021

comment:126

Replying to @mkoeppe:

Replying to @kiwifb:

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 9, 2021

comment:127

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

@kiwifb
Copy link
Member

kiwifb commented Mar 9, 2021

comment:128

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

@jhpalmieri
Copy link
Member

comment:129

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).

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 9, 2021

comment:130

Replying to @kiwifb:

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants