Opened 2 years ago

Closed 2 years ago

Last modified 16 months ago

#29697 closed enhancement (fixed)

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

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.2
Component: build Keywords:
Cc: dimpase, fbissey, mjo, isuruf Merged in:
Authors: Matthias Koeppe Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: 9a50cba (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

$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, is also used by sage.misc.cython.cython.

Change History (14)

comment:1 Changed 2 years ago by mkoeppe

  • Branch set to u/mkoeppe/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

comment:2 follow-up: Changed 2 years ago by fbissey

  • Commit set to 16247ad9675343f1d3e1158cc5e43137aec82645

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

New commits:

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

comment:3 Changed 2 years ago by mkoeppe

  • Description modified (diff)

comment:4 Changed 2 years ago by mkoeppe

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.

comment:5 Changed 2 years ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Milestone changed from sage-9.1 to sage-9.2
  • Status changed from new to needs_review

comment:6 Changed 2 years ago by git

  • Commit changed from 16247ad9675343f1d3e1158cc5e43137aec82645 to 9a50cba23abb1c6ff4b022c62624da800b88e282

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

9a50cbasrc/sage/ (sage_include_directories): Fixup doctest

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

Replying to fbissey:

I don't think it is controversial at all.

No, just needs review.

comment:8 Changed 2 years ago by fbissey

  • Reviewers set to François Bissey
  • Status changed from needs_review to positive_review

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.

comment:9 Changed 2 years ago by mkoeppe


comment:10 Changed 2 years ago by fbissey

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

comment:11 Changed 2 years ago by vbraun

  • Branch changed from u/mkoeppe/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 to 9a50cba23abb1c6ff4b022c62624da800b88e282
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:12 Changed 22 months ago by mkoeppe

  • Commit 9a50cba23abb1c6ff4b022c62624da800b88e282 deleted

comment:13 Changed 22 months ago by mkoeppe

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

comment:14 Changed 16 months ago by mkoeppe

Follow-up: #31335

Note: See TracTickets for help on using tickets.