sagelib spkg-install: Unset but do not poison SAGE_LOCAL

[mkoeppe]

This corrects a change made in #29779 and avoids ld: warning: directory not found for option '-L/doesnotexist/lib' (and also restores/corrects rebasing on the Cygwin platform).

CC: @jhpalmieri @dimpase @kiwifb

Component: build

Author: Matthias Koeppe

Branch/Commit: bca1c69

Reviewer: François Bissey

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

[mkoeppe]

Branch: u/mkoeppe/sagelib_spkg_install__unset_but_do_not_poison_sage_local

[mkoeppe]

Commit: 5d11ff7

[mkoeppe]

New commits:

fb2c9d2	build/pkgs/sagelib/spkg-install: Only unset SAGE_LOCAL and SAGE_PKG_CONFIG_PATH, do not poison them
5d11ff7	build/pkgs/sagelib/spkg-install [CYGWIN]: Rebase in SAGE_VENV, not SAGE_LOCAL

[mkoeppe]

Description changed:

--- 
+++ 
@@ -1,2 +1,3 @@
 This corrects a change made in #29779 and avoids `ld: warning: directory not found for option '-L/doesnotexist/lib'`
+(and also restores/corrects rebasing on the Cygwin platform).

[jhpalmieri]

comment:4

Under what circumstances will it matter whether SAGE_LOCAL is set or unset?

[mkoeppe]

comment:5

The function var in sage.env first takes the value of the environment variable. Only if it is unset, the value is taken from sage_conf or the fallback value.

At the time of installation, sage_setup.setenv manipulates CPATH, LIBRARY_PATH etc. based on the value of SAGE_LOCAL. This mechanism is not actually needed for installation within the Sage distribution because we have already set all of these environment variables in sage-build-env.

The ld: warning: directory not found ... /doesnotexist/lib are not really a problem, but I have marked the ticket as critical because these messages have the potential to confuse users.

(However, the Cygwin rebasing really was broken by #29779 and is also restored here.)

[jhpalmieri]

comment:6

Sorry, I don't understand. Before #29779, we didn't touch SAGE_LOCAL. We have three options:

• unset SAGE_LOCAL
• do nothing — the status quo before #29779, I think
• poison `SAGE_LOCAL — causes all of the warning messages

The old comment (deleted in #29779) about not poisoning SAGE_LOCAL was deemed no longer valid, although I don't know why. Anyway, I'm happy to avoid poisoning that variable, but is the point here that we want to avoid problems if the user has somehow set SAGE_LOCAL? Otherwise, how does unsetting compare to just doing nothing?

[mkoeppe]

comment:7

Doing nothing with SAGE_LOCAL, i.e., keeping it at the value set by the build environment (sage-env + sage-build-env) would also work.

My mistake in #29779 was to believe that the comment, "We cannot poison SAGE_LOCAL because the pkg-config script..." gave the only reason why we cannot poison it. That was true at the time of writing that comment, but was no longer true after I added the sage_setup.setenv mechanism.

[mkoeppe]

comment:8

None of the poisoning and none of the unsetting of variables is needed to ensure correct functioning of the installation script:

All the variables are set to the correct values by sage-env + sage-build-env. The purpose of poisoning / unsetting is only to make sure that future changes to sagelib's build system do not re-introduce undisciplined data flows.

[jhpalmieri]

comment:9

This works for me. The Cygwin fix looks reasonable, but I haven't tested it. Anyone else?

[kiwifb]

comment:11

The git branch currently has

export SAGE_PKG_CONFIG_PATH=/doesnotexist

and later

+unset SAGE_PKG_CONFIG_PATH

please remove the first one like you did with SAGE_LOCAL.

[mkoeppe]

comment:12

Thanks for spotting this

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 5d11ff7 to bca1c69

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

bca1c69

build/pkgs/sagelib/spkg-install: Remove redundant manipulation of SAGE_PKG_CONFIG_PATH

[kiwifb]

Reviewer: François Bissey

[kiwifb]

comment:14

LGTM

[mkoeppe]

comment:15

Thanks!

[vbraun]

Changed branch from u/mkoeppe/sagelib_spkg_install__unset_but_do_not_poison_sage_local to bca1c69