Opened 3 years ago

Closed 23 months ago

#21535 closed enhancement (fixed)

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

Reported by: mkoeppe Owned by: embray
Priority: major Milestone: sage-8.1
Component: build Keywords:
Cc: jdemeyer, embray Merged in:
Authors: Jeroen Demeyer Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: bd91b43 (Commits) Commit: bd91b438f6992c21ee4da35a850c81f28563d2e9
Dependencies: #21480, #21600, #21604, #23744 Stopgaps:

Description (last modified by 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.

Change History (30)

comment:1 follow-up: Changed 3 years ago by embray

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.

comment:2 in reply to: ↑ 1 Changed 3 years ago by mkoeppe

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.

comment:3 Changed 3 years ago by mkoeppe

  • Cc jdemeyer embray added
  • Dependencies set to #21480, #21600, #21604
  • Description modified (diff)
  • Summary changed from Make src/setup.py independent of SAGE_CYTHONIZED to Make src/setup.py respect --build-base and --inplace, independent of SAGE_CYTHONIZED

comment:4 Changed 3 years ago by embray

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

comment:5 Changed 3 years ago by mkoeppe

  • Description modified (diff)
  • Milestone changed from sage-7.4 to sage-7.5

comment:6 Changed 3 years ago by embray

  • Owner changed from (none) to embray

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

comment:7 Changed 3 years ago by jdemeyer

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

comment:8 Changed 3 years ago by embray

Yes, it would be easier.

comment:9 Changed 3 years ago by embray

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?

comment:10 Changed 3 years ago by mkoeppe

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

comment:11 Changed 23 months ago by mkoeppe

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?

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

comment:12 follow-up: Changed 23 months ago by embray

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

comment:13 Changed 23 months ago by jdemeyer

  • Dependencies changed from #21480, #21600, #21604 to #21480, #21600, #21604, #23744

As a small first step, see #23744.

comment:14 in reply to: ↑ 12 Changed 23 months ago by mkoeppe

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.

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

comment:15 Changed 23 months ago by jdemeyer

  • Branch set to u/jdemeyer/no_SAGE_CYTHONIZED

comment:16 follow-up: Changed 23 months ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Commit set to 769de13efb2d23fcb511dc1987ab82204e15150c
  • Milestone changed from sage-7.5 to sage-8.1
  • Status changed from new to needs_review

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:

33746a2Don't use SAGE_CYTHONIZED in sage_include_directories()
769de13Stop using SAGE_CYTHONIZED

comment:17 follow-up: Changed 23 months ago by 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.

comment:18 in reply to: ↑ 16 Changed 23 months ago by mkoeppe

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

  • src/setup.py

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

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 (#21469) is done.

comment:19 in reply to: ↑ 17 Changed 23 months ago by mkoeppe

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.

comment:20 Changed 23 months ago by mkoeppe

  • Status changed from needs_review to needs_work

comment:21 follow-up: Changed 23 months ago by jdemeyer

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

comment:22 Changed 23 months ago by jdemeyer

Working on this...

comment:23 Changed 23 months ago by git

  • Commit changed from 769de13efb2d23fcb511dc1987ab82204e15150c to bd91b438f6992c21ee4da35a850c81f28563d2e9

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

bd91b43Stop using SAGE_CYTHONIZED

comment:24 Changed 23 months ago by jdemeyer

  • Status changed from needs_work to needs_review

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.

comment:25 in reply to: ↑ 21 ; follow-up: Changed 23 months ago by 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.

comment:26 in reply to: ↑ 25 Changed 23 months ago by embray

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.

comment:27 Changed 23 months ago by mkoeppe

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

comment:28 Changed 23 months ago by jdemeyer

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

comment:29 Changed 23 months ago by mkoeppe

  • Reviewers set to Matthias Koeppe
  • Status changed from needs_review to positive_review

comment:30 Changed 23 months ago by vbraun

  • Branch changed from u/jdemeyer/no_SAGE_CYTHONIZED to bd91b438f6992c21ee4da35a850c81f28563d2e9
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.