Use pipenv to create venv for development

[mkoeppe]

We modify src/setup.py to support editable installs via pipenv (pipenv install -e ., which invokes pip install --editable). Since distribution continues using build/pkgs/sagelib/src/setup.py, the changes to src/setup.py made in this ticket do not affect the existing build process of the Sage library.

With this ticket, the following code works:

./bootstrap
./configure --enable-editable
make build-local
cd src
mkdir .venv
pipenv install --dev --ignore-pipfile
pipenv run pip install -e . --upgrade --exists-action=i --no-build-isolation
pipenv run sage-runtests --all

Instead of the first three commands, one can of course also use tox -e local-sudo-standard build-local.
It generates a virtual env in src/.venv that contains all necessary Python packages, and an editable install of sage.
Moreover, the installation of some packages might fail (depending on your system configuration). In this case one can manually build it using sage and then install the sage-built wheel in the pipenv. For example,

make fpylll
cd src && pipenv run pip install <sage>/local/var/lib/sage/venv-python3.8/var/lib/sage/wheels/fpylll-0.5.6-cp38-cp38-linux_x86_64.whl

The output of the added GH workflow is available here: https://github.com/sagemath/sagetrac-mirror/actions?query=workflow%3A%22Build+%26+Test+using+pipenv%22+branch%3Apublic%2Fbuild%2Finplace
Most tests are working, but a few are failing. Only a few tests are failing:

sage -t --random-seed=0 sage/misc/sagedoc.py  # 4 doctests failed
sage -t --random-seed=0 sage/docs/conf.py  # 1 doctest failed
sage -t --random-seed=0 sage/matrix/matrix_integer_dense.pyx  # 1 doctest failed
sage -t --random-seed=0 sage/cpython/dict_del_by_value.pyx  # 2 doctests failed
sage -t --random-seed=0 sage/tests/cmdline.py  # 8 doctests failed

TODO (as followup tickets):

• Expand on the added documentation on how to set up the virtual environment, using either plain python -m venv, virtualenv (20.x), or pipenv. This is Extend documentation on how to set up the virtual environment #31342.
• Consider not commit pipfile.lock but instead generate it as build artifact on ci (so that build errors can be easily reproduced by downloading the lock file, and re-creating the exact venv locally): Proposal: pipenv patterns and antipatterns for python library project pypa/pipenv#1911 (comment). However, normally you want to put the lock file under version control for the reasons described in https://trac.sagemath.org/timeline?from=2021-02-03T13%3A15%3A22Z. The official statement is "always include in version control", see git - Should Pipfile.lock be committed to version control? pypa/pipenv#598.
• Replace manual definition of build variables (PKG_CONFIG_PATH, LIBRARY_PATH etc) by sage-env.
• Fix failing doctests.

Previous tickets on in-place builds, editable installs:

• build the sage library in place #12659
• Make src/setup.py respect --build-base and --inplace, independent of SAGE_CYTHONIZED #21535

Depends on #30779
Depends on #30672
Depends on #30673
Depends on #30709
Depends on #30706
Depends on #30901
Depends on #31080
Depends on #29850
Depends on #29314
Depends on #30937
Depends on #30935
Depends on #30731
Depends on #30859
Depends on #30923
Depends on #30910
Depends on #30944
Depends on #30770
Depends on #31216
Depends on #31029
Depends on #30912
Depends on #31357
Depends on #31362
Depends on #31365
Depends on #31377

CC: @tobiasdiez @kiwifb @jhpalmieri @isuruf

Component: build

Keywords: sd111

Author: Tobias Diez

Branch/Commit: public/build/inplace @ c736e0c

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

[tobiasdiez]

comment:1

I've worked a bit on it. The current commit is a semi-working prototype, in the sense that all cython files are successfully compiled to C files if invoked via python src/setup.py build_ext --inplace. The second compilation of the c files does not yet work.

I had problems that none of the source cython files were correctly found on my system (wsl) and that a few dependencies in the current build files were not met ('linbox', 'Singular', 'm4ri', 'zlib', 'cblas'). For this reason, I started from scratch and only included what was needed so far. Any pointers to how to install these dependencies (I think they are needed for the second compilation).

---

New commits:

bc6e272

Semi-working prototype for inplace compilation

[tobiasdiez]

Commit: bc6e272

[tobiasdiez]

Branch: public/build/inplace

[mkoeppe]

comment:2

I don't really have time to look into this before the 9.2 release. But if you are looking for where include directories are determined, take a look at sage_setup.command.sage_build_cython

[sagetrac-git]

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

58452ce

Make in-place compilation work

[sagetrac-git]

Changed commit from bc6e272 to 58452ce

[tobiasdiez]

comment:4

The command ./sage src/setup.py build_ext --inplace runs now fine for me. I had to change quite a few things to make it work, and found it easier to completely start over using cythonize instead of using the existing build_cython code. The resulting code is a minimal working version. Please let me know which features of the existing build_cython file you would like to keep (to be honest, I don't really understand what some of the code there is doing, and why its needed).

The code definitely needs cleanup, but otherwise is ready for a first round of review.

[sagetrac-git]

Changed commit from 58452ce to 1a99228

[sagetrac-git]

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

1a99228

Update gitignore

[mkoeppe]

comment:7

--- a/.gitignore
+++ b/.gitignore
@@ -99,7 +99,18 @@ gitlab-build-docker.log
 /src/build
 /src/Makefile
 /src/bin/sage-env-config
-/build/bin/sage-build-env-config
+/build

this is not good. Our /build/ contains important files of the sage distribution

[mkoeppe]

comment:8

And the changes removing old python2 code really need to go on a separate ticket

[sagetrac-git]

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

22b27a0

Remove global gitignore of build folder

[sagetrac-git]

Changed commit from 1a99228 to 22b27a0

[tobiasdiez]

comment:10

Ok, I was under the assumption that the build folder is for build artifacts. If this would be the case, then I suppose it would be better to whitelist those files in the build folder that needs to be checked-in. I've now replaced the global build ignore, by a more fine-grained statement which only ignores the "temporary" files.

Concerning the changes about old Python2 code, I've created #30397.

[mkoeppe]

comment:11

Replying to @tobiasdiez:

I was under the assumption that the build folder is for build artifacts.

No, not at all.

[mkoeppe]

comment:12

The changes I see in setup.py don't seem to fit so well into the plan I have regarding modularization of sagelib. See #29705 and #28925 (which needs help).

[tobiasdiez]

comment:13

I wasn't aware of your changes in #28925. I guess what I'm proposing is a different strategy for the source discovery. The approach in #28925 is to change the source files themselves and enrich them by some form of metadata that decides in which distribution / package they land. Here I was proposing to specify the distribution of the files in the setup.py file. I guess both strategies have pros and cons. The main reason for me was that it seems the responsibility of the setup.py file to construct the distribution; why should the source file need to care about where they are used/included? I.e classical example of separation of concerns. I think, it's also the more flexible approach since it's easy to include a source file into multiple distributions etc. Moreover, you also gain a bit of performance at build time since you don't need to parse the files for some magic comments. What do you think?

[mkoeppe]

comment:14

Replying to @tobiasdiez:

[...] Here I was proposing to specify the distribution of the files in the setup.py file. I guess both strategies have pros and cons. The main reason for me was that it seems the responsibility of the setup.py file to construct the distribution; why should the source file need to care about where they are used/included? I.e classical example of separation of concerns. [...]

One more thing to be aware of is that (1) setup.py by itself actually is not able to control what goes into the source distribution. In #29865 (where I prepare some experimental subset distributions), I additionally use MANIFEST.in to control that.

Separation of concerns is, of course, desirable. But so is locality: (2) My plan for splitting up the distribution is driven to a large extent by the dependencies on different sets of C/C++ libraries. And these are already encoded in Cython distutils directives.

Also I have explored a little bit the packaging options beyond using setuptools (in part motivated because of the shortcoming (1)). See #29854 (flit), #29810 (poetry). Unfortunately currently there does not seem to be a proper mainstream solution for the source layout that we want to keep (a monolithic source tree in src/sage). For example, flit (and even pip out-of-directory installs) do not handle symbolic links well. In the long term, I was hoping to transform sage_setup to implement the PEP 517 API directly (see #29845). That's another reason why I would be reluctant to hard-code distribution information in setup.py.

In any case, I am hoping to have a broader discussion about all of this on sage-devel at the beginning of the 9.3 cycle.

[tobiasdiez]

comment:15

Separation of concerns is, of course, desirable. But so is locality: (2) My plan for splitting up the distribution is driven to a large extent by the dependencies on different sets of C/C++ libraries. And these are already encoded in Cython distutils directives.

One could also take this as an argument to remove the distutil dependency directives and put them into the setup.py, giving you locality and separation of concerns.

Anyway, my goal with this PR is not to redesign or modularize the build. I just wanted to have a working inplace build. For this moving the distribution declaration to setup.py was the easiest option, and I quite liked this also from a conceptual point of view. But if the current way is preferred, I change it back of course.
Anything else that needs to be changed?

[mkoeppe]

comment:16

Replying to @tobiasdiez:

One could also take this as an argument to remove the distutil dependency directives and put them into the setup.py, giving you locality and separation of concerns.

Ha, no, we just moved them there. Getting rid of the messy file modules_list.py was an achievement in the present cycle. See #29706

[mkoeppe]

comment:17

Replying to @tobiasdiez:

Anything else that needs to be changed?

I haven't really had a chance to look at it in more detail yet, sorry. Soon...

[sagetrac-git]

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

55d00d7

Merge branch 'develop' of git://trac.sagemath.org/sage into public/build/inplace

[sagetrac-git]

Changed commit from 22b27a0 to 55d00d7

[tobiasdiez]

Dependencies: #30397

[sagetrac-git]

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

8404a5d	Remove old python 2 code from cython files
e9c06e3	Readd constants in build script
811b78c	Remove unused import
32daec5	Merge branch 'public/build/removeOldCode' of git://trac.sagemath.org/sage into public/build/inplace
40f3503	Fix merge problem

[mkoeppe]

Branch has merge conflicts