Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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 #29697

Closed
mkoeppe opened this issue May 17, 2020 · 18 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented May 17, 2020

$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

@mkoeppe mkoeppe added this to the sage-9.1 milestone May 17, 2020
@kiwifb
Copy link
Member

kiwifb commented May 17, 2020

Commit: 16247ad

@kiwifb
Copy link
Member

kiwifb commented May 17, 2020

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

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 17, 2020

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.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 17, 2020

Author: Matthias Koeppe

@mkoeppe mkoeppe modified the milestones: sage-9.1, sage-9.2 May 17, 2020
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 17, 2020

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 17, 2020

Changed commit from 16247ad to 9a50cba

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 17, 2020

comment:7

Replying to @kiwifb:

I don't think it is controversial at all.

No, just needs review.

@kiwifb
Copy link
Member

kiwifb commented May 17, 2020

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.

@kiwifb
Copy link
Member

kiwifb commented May 17, 2020

Reviewer: François Bissey

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 17, 2020

comment:9

Thanks!

@kiwifb
Copy link
Member

kiwifb commented May 17, 2020

comment:10

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 26, 2020

comment:12

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 26, 2020

Changed commit from 9a50cba to none

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jul 26, 2020

comment:13

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

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 4, 2021

comment:14

Follow-up: #31335

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants