Make src/setup.py respect --build-base and --inplace, independent of SAGE_CYTHONIZED

[mkoeppe]

This is a follow-up on #21480, which put src/setup.py in charge of all sagelibbuilding and removed dependencies on various environment variables, as a step towards the goal of making sagelib an ordinary Python package (#21507).

However, there is still a dependency on $SAGE_CYTHONIZED, which needs to be set in a way that matches the build-base (set by setup.py build --build-base).

This dependency should be removed.

Also, --inplace should be supported.
(See also: #12659: build the sage library in place)

See also #21508 on other cleanup issues of src/setup.py.

Depends on #21480
Depends on #21600
Depends on #21604
Depends on #23744

CC: @jdemeyer @embray

Component: build

Author: Jeroen Demeyer

Branch/Commit: bd91b43

Reviewer: Matthias Koeppe

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

[embray]

comment:1

I think I've brought this up with jdemeyer before, who had some objections (I think where I brought it up originally was in the context of cysignals, but same applies to sage itself). Ultimately what it came down to is that he prefers the source tree to always be clean of build artifacts, which I can certainly respect. But I don't personally have a problem like that, and like to be able to use ./setup.py build_ext --inplace for in-place testing, which I find cuts down on time spent in the "edit-build-install" cycle.

In any case, these are subjective preferences for development practice, and I think either way should work--eliminating dependency on $SAGE_CYTHONIZED should be a step in the right direction.

[mkoeppe]

comment:2

Replying to @embray:

Ultimately what it came down to is that [jdemeyer] prefers the source tree to always be clean of build artifacts, which I can certainly respect. But I don't personally have a problem like that, and

I also like to keep the source tree clean, which is why I usually use VPATH builds. I have created #21469 for that.

like to be able to use ./setup.py build_ext --inplace for in-place testing, which I find cuts down on time spent in the "edit-build-install" cycle.

Thanks for pointing out --inplace. I'll look into this.

[mkoeppe]

Dependencies: #21480, #21600, #21604

[embray]

comment:4

#21682 might help with this one--it makes SAGE_CYTHONIZED mostly irrelevant IMO.

[embray]

comment:6

I should be able to fix this with or without #21682, though I think it helps.

[jdemeyer]

comment:7

Wouldn't it be easier to fix with #21682, given that you are adding a --build-dir option?

[embray]

comment:8

Yes, it would be easier.

[embray]

comment:9

So what would be the impact of removing SAGE_CYTHONIZED entirely? As far as I can tell it is only really used by setup.py/sage_setup. Implementing this ticket makes it pretty pointless I think--it makes SAGE_CYTHONIZED not even make sense, really.

Would it be a problem to outright remove it? Or does it need to still be supported somehow?

[mkoeppe]

comment:10

+1 for getting rid of SAGE_CYTHONIZED completely if you can.

[mkoeppe]

comment:11

Erik and Jeroen, given that #21682 (setup.py build_cython) is merged, can we now get rid of SAGE_CYTHONIZED and support setup.py build --build-base?

[embray]

comment:12

I'd be all for that. Do you need me to work on it?

[jdemeyer]

Changed dependencies from #21480, #21600, #21604 to #21480, #21600, #21604, #23744

[jdemeyer]

comment:13

As a small first step, see #23744.

[mkoeppe]

comment:14

Replying to @embray:

I'd be all for that. Do you need me to work on it?

That would be great!

By the way, in my current attempt at #21469 I noticed that --build-base already works.

This line in src/setup.py:

build_base = 'build' # the distutils default. Changing it is not supported by this setup.sh.

is probably a leftover from my earlier changes, and should probably be removed. The variable build_base does not seem to be used, and the comment is no longer correct.

[jdemeyer]

Branch: u/jdemeyer/no_SAGE_CYTHONIZED

[jdemeyer]

Author: Jeroen Demeyer

[jdemeyer]

comment:16

Here is a branch getting rid of SAGE_CYTHONIZED. Unfortunately, two doctests in sage_setup still require SAGE_CYTHONIZED, so I just hardcoded the value there. This clearly goes against DRY, so I'm not so happy with it. Ideas or improvements are welcome.

---

New commits:

33746a2	Don't use SAGE_CYTHONIZED in sage_include_directories()
769de13	Stop using SAGE_CYTHONIZED

[jdemeyer]

Commit: 769de13

[embray]

comment:17

Hmm--one idea might be for setup.py to write out a sort of build configuration file, similar to what we do with .cython_version, logging attributes of interest from the distutils Distribution. The test could then use that to determine the build_base that was used for the last build. Probably overkill just to make a test work, but having such a file might be useful even for debugging/logging purposes if nothing else.

[mkoeppe]

comment:18

Replying to @jdemeyer:

Here is a branch getting rid of SAGE_CYTHONIZED. Unfortunately, two doctests in sage_setup still require SAGE_CYTHONIZED, so I just hardcoded the value there. This clearly goes against DRY, so I'm not so happy with it. Ideas or improvements are welcome.

Commit 33746a2 is good, but I think some of the changes in 769de13 are not going in the right direction.

1. In particular the following, I think, breaks the already working --build-base feature (see comment 14)!

diff --git a/src/setup.py b/src/setup.py
index 6354427..be4036d 100755
--- a/src/setup.py
+++ b/src/setup.py
@@ -52,12 +52,16 @@ sys.excepthook = excepthook
 
 build_base = 'build' # the distutils default. Changing it is not supported by this setup.sh.
 
+# Directory to store Cython-generated files
+SAGE_CYTHONIZED = os.path.join(build_base, "cythonized")
+

2. The hardcoded paths depending on SAGE_SRC make it harder for the VPATH patch. Perhaps it's best to do this work after VPATH (Enable VPATH builds (several independent build trees connected to one source tree) #21469) is done.

[mkoeppe]

comment:19

Replying to @embray:

Hmm--one idea might be for setup.py to write out a sort of build configuration file, similar to what we do with .cython_version, logging attributes of interest from the distutils Distribution. The test could then use that to determine the build_base that was used for the last build. Probably overkill just to make a test work, but having such a file might be useful even for debugging/logging purposes if nothing else.

Yes, I think this is a good idea. Basically this is part of #21707 (Split sage-env into sage-build-env, sagelib-build-env and sage-env) -- setup.sh should be responsible for writing out a configuration file concerning sagelib.

[jdemeyer]

comment:21

I see. The variable build_base was really added by mistake in #21480 when removing support for --build-base.

[jdemeyer]

comment:22

Working on this...

[sagetrac-git]

Changed commit from 769de13 to bd91b43

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

bd91b43

Stop using SAGE_CYTHONIZED

[jdemeyer]

comment:24

This should address the concerns about build_base. The doctests still hardcode build/cythonized, but I prefer to fix that in a different ticket. I think this branch here does enough good things (even if it's not perfect), so I'm setting it back to needs_review.

[mkoeppe]

comment:25

Replying to @jdemeyer:

I see. The variable build_base was really added by mistake in #21480 when removing support for --build-base.

Well, in #21480 the variable build_base was still used, but some of Erik's later tickets seem to have removed all uses of it.

[embray]

comment:26

Replying to @mkoeppe:

Replying to @jdemeyer:

I see. The variable build_base was really added by mistake in #21480 when removing support for --build-base.

Well, in #21480 the variable build_base was still used, but some of Erik's later tickets seem to have removed all uses of it.

I'm not actively concerned about any of my existing open tickets addressing issues in the setup.py. I'd like to get back to that at some point but if any of those tickets still end up being useful I'll rework them.

[mkoeppe]

comment:27

Hi Erik, there's no problem; to the contrary, as a result of these changes that are already merged, --build-base already works.

[jdemeyer]

comment:28

Indeed. What this ticket does is also use the build_base directory for the cythonized files.

[mkoeppe]

Reviewer: Matthias Koeppe

[vbraun]

Changed branch from u/jdemeyer/no_SAGE_CYTHONIZED to bd91b43