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

sage_setup: Use paths within SAGE_LOCAL when provided via sage_conf #31338

Closed
mkoeppe opened this issue Feb 4, 2021 · 30 comments
Closed

sage_setup: Use paths within SAGE_LOCAL when provided via sage_conf #31338

mkoeppe opened this issue Feb 4, 2021 · 30 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Feb 4, 2021

We add a simple mechanism to sagelib's setup.py (via a new module in sage_setup) to prepend SAGE_LOCAL values to key environment variables needed for the build: PATH, LIBRARY_PATH, CPATH, LDFLAGS. This allows use to build sagelib outside of an environment set by sage-env (which would set these variables among many more) if only sage_conf is installed in the build environment.

To test:

./configure --enable-editable
make sagelib-build-deps      # installs sage_conf and other things
(cd src && ../local/bin/python3 setup.py develop)

Note - the last line is not within a sage-env!

Follow-up, if necessary: Because for misconfigured Pythons, -I options may leak in as described in #31335 and take precedence over the CPATH that we set, we may want to essentially revert #29697, adding some refinements:

  • SAGE_LOCAL vs SAGE_VENV
  • make sure that if $SAGE_LOCAL is unset, nothing is added.

Relevant tickets regarding include/library search paths: #13348, #14709, #29562 (+), #29607 (+), #29697 (?), #31041 (), #30818 (), #30013 (~)

Depends on #31593

CC: @jhpalmieri @kiwifb @orlitzky @antonio-rojas @dimpase

Component: build

Author: Matthias Koeppe

Branch: 31eaf59

Reviewer: Dima Pasechnik

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

@mkoeppe mkoeppe added this to the sage-9.4 milestone Feb 4, 2021
@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 6, 2021

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 6, 2021

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 6, 2021

New commits:

5161d65build/pkgs/sage_conf/src/sage_conf.py.in: Move SAGE_ROOT, SAGE_LOCAL to beginning of file; only use substitution @prefix@ once
986ca18build/pkgs/sage_conf/src/sage_conf.py.in: replace subst by replace
6f05cc2sage_setup.setenv: New, use it in setup.py so that build works outside of sage-env

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 6, 2021

Dependencies: #31593

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 6, 2021

Commit: 6f05cc2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 11, 2021

Changed commit from 6f05cc2 to 6ea676d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 11, 2021

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

6ea676dMerge tag '9.3' into t/31338/sage_setup__use_paths_within_sage_local_when_provided_via_sage_conf

@dimpase
Copy link
Member

dimpase commented May 12, 2021

Reviewer: Dima Pasechnik

@dimpase
Copy link
Member

dimpase commented May 12, 2021

comment:5

lgtm

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 12, 2021

comment:6

Thank you!

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 27, 2021

Changed commit from 6ea676d to 31eaf59

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 27, 2021

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

2078fa6Merge tag '9.4.beta0' into t/31593/configure__paths_within__sage_local___prefix__for_sage_conf
31eaf59Merge #31593

@dimpase
Copy link
Member

dimpase commented Jun 2, 2021

comment:8

ran into missing toml dependency in build/pkgs/pytoml/dependencies while testing (building backall errored out with toml module not found - pytoml was built, but toml was not).

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 2, 2021

comment:9

I think this is actually coming from #31280

@dimpase
Copy link
Member

dimpase commented Jun 2, 2021

comment:10

OK, fine, this works otherwise, will look at #31280 again.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 3, 2021

comment:11

Thanks!

@vbraun
Copy link
Member

vbraun commented Jun 20, 2021

@kiwifb
Copy link
Member

kiwifb commented Jun 22, 2021

Changed commit from 31eaf59 to none

@kiwifb
Copy link
Member

kiwifb commented Jun 22, 2021

comment:13

A bit late to that party. sage_setup/setenv.py is highly "polluting" - i.e. it all works but fill my build logs with useless junk

/usr/lib/gcc/x86_64-pc-linux-gnu/10.3.0/../../../../x86_64-pc-linux-gnu/bin/ld: skipping incompatible /usr/lib/libm.so when searching for -lm
/usr/lib/gcc/x86_64-pc-linux-gnu/10.3.0/../../../../x86_64-pc-linux-gnu/bin/ld: skipping incompatible /usr/lib/libm.a when searching for -lm
/usr/lib/gcc/x86_64-pc-linux-gnu/10.3.0/../../../../x86_64-pc-linux-gnu/bin/ld: skipping incompatible /usr/lib/libc.so when searching for -lc
/usr/lib/gcc/x86_64-pc-linux-gnu/10.3.0/../../../../x86_64-pc-linux-gnu/bin/ld: skipping incompatible /usr/lib/libc.a when searching for -lc

I am trying to follow the logic of the ticket but I fail to see when SAGE_LOCAL is not defined, which is the only way to avoid these. In env.py we have

SAGE_LOCAL = var("SAGE_LOCAL", SAGE_VENV)

So either something from the environment or SAGE_VENV and it is defined by

SAGE_VENV = var("SAGE_VENV", os.path.abspath(sys.prefix))

So if neither SAGE_LOCAL or SAGE_VENV is in the environment SAGE_LOCAL becomes os.path.abspath(sys.prefix).

I am not sure why the if SAGE_LOCAL: conditional is necessary since it is always defined. What I don't see is a way to avoid the content of that block becoming added to the environment.

Is there another ticket that will make SAGE_LOCAL default to None in some circumstances? Or a process that I overlooked?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 23, 2021

comment:14

The idea for distribution packaging would be that SAGE_LOCAL will not be set at all, which is why we have the if SAGE_LOCAL there.

That it is always set, by the traditional catch-all defaults in sage.env, is a nice catch; let's fix this in #32036.

@kiwifb
Copy link
Member

kiwifb commented Jun 23, 2021

comment:15

I am OK with eliminating SAGE_LOCAL the way SAGE_ROOT is virtually extinct but we are not there yet. It still need to be set at runtime for a few things. Some would gain to be replaced by more specific variables. But let's move things to #32036

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 24, 2021

comment:16

Let's see if we can solve this problem in a different way. Where is this incompatible /usr/lib/libm.so business exactly coming from?

@kiwifb
Copy link
Member

kiwifb commented Jun 24, 2021

comment:17

Replying to @mkoeppe:

Let's see if we can solve this problem in a different way. Where is this incompatible /usr/lib/libm.so business exactly coming from?

Because you are add -L/usr/lib (and some rpath to boot) at linking time, since SAGE_LOCAL is /usr. But of of course on my system the correct location is /usr/lib64, /usr/lib is for 32bit libraries. So, when you add -L/usr/lib it looks for the 32bit version of the libraries first before looking for the correct ones in the default location.

It is not strictly harmful unless you have done something dodgy to your system. I described it as pollution, because my logs gets full of it. Which makes reading them difficult when you are looking for real problems.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 24, 2021

comment:18

OK, this is essentially the same issue that we were hitting in #31578. Should we be looking for a general way to find the multilib path for situations like this? Perhaps something in python3 -m sysconfig gives it?

@kiwifb
Copy link
Member

kiwifb commented Jun 24, 2021

comment:19

It is similar indeed and yes it appears that python -m sysconfig has the correct value in LIBDIR but we should check on a few setup to make sure.

@kiwifb
Copy link
Member

kiwifb commented Jun 24, 2021

comment:20

Hum, on ubuntu I get /usr/lib when I am fairly sure it should be /usr/lib/x86_64-linux-gnu.

@kiwifb
Copy link
Member

kiwifb commented Jun 24, 2021

comment:21

This is really annoying, ubuntu systems have a MULTIARCH defined where my gentoo system doesn't. But conda on ubuntu defines MULTIARCH even so it doesn't use it. There must be a better way to get the right info.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Jun 24, 2021

comment:22

Another possible solution: #32057

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

4 participants