sagemath/sage

src/setup.py, src/sage/env.py (sage_include_directories): Do not add another copy of SAGE_INC, SAGE_LOCAL/lib to include dirs, library dirs

mkoeppe opened this issue · 18 comments

$SAGE_LOCAL/{include,lib} are already added to the front of the search paths CPATH and LIBRARY_PATH by sage-env.

We remove code that adds another copy. This removes a dependency on SAGE_LOCAL during the build of sagelib.

The function sage_include_directories, apart from its use by setup.py, is also used by sage.misc.cython.cython.

CC: @dimpase @kiwifb @orlitzky @isuruf

Component: build

Author: Matthias Koeppe

Branch: 9a50cba

Reviewer: François Bissey

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

Commit: 16247ad

comment:2

I don't think it is controversial at all. Is there anyone who think we are forgetting something here?


New commits:

53f0359src/sage/env.py (sage_include_directories): Do not put SAGE_INC in front of the sage source include directories
16247adsrc/setup.py: Do not put SAGE_LOCAL/lib in front of the library directories

Description changed:

--- 
+++ 
@@ -1,3 +1,5 @@
 `$SAGE_LOCAL/{include,lib}` are already added to the front of the search paths `CPATH` and `LIBRARY_PATH` by `sage-env`.
 
+We remove code that adds another copy. This removes a dependency on `SAGE_LOCAL` during the build of sagelib.
 
+The function `sage_include_directories`, apart from its use by `setup.py`, is also used by `sage.misc.cython.cython`. 
comment:4

I plan to add some doctests to make sure that functions such as sage.misc.cython.cython also work correctly when sage.all is imported into plain Python (without runnning sage-env). Things like this are currently not tested at all (and probably not guaranteed by anything); I plan to use #29446 for this.

Author: Matthias Koeppe

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

9a50cbasrc/sage/env.py (sage_include_directories): Fixup doctest

Changed commit from 16247ad to 9a50cba

comment:7

Replying to @kiwifb:

I don't think it is controversial at all.

No, just needs review.

comment:8

Well, I think you have gone after the only breakage in the last commit. Unless someone can point to a corner case, it is all positive from me.

Reviewer: François Bissey

comment:9

Thanks!

comment:10

I didn't realise at the time, but that will be one less line of patching for me in sage-on-gentoo.

comment:12

This may have caused some breakage: https://groups.google.com/d/msg/sage-release/SdxKEn7CuLM/3ru84S_zAgAJ

Changed commit from 9a50cba to none

comment:13

... because of incorrect usage of extra_compile_args in some of our modules - to be fixed in #30227

comment:14

Follow-up: #31335