MP_LIBRARY, BLAS, PYTHON need inst_ in Makefile.in

[dimpase]

In particular to properly handle "dummy" targets like gmp/mpir,
sagelib should depend upon $(inst_$(MP_LIBRARY)).

Also, sagelib in deps should only depend on one Python:

-               $(inst_python2) \
-               $(inst_python3) \
+               $(inst_$(PYTHON)) \

CC: @embray @jdemeyer

Component: build

Author: Dima Pasechnik

Branch/Commit: 356621a

Reviewer: Erik Bray

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

[dimpase]

Branch: u/dimpase/build/correct_generic_deps_t27254

[dimpase]

Commit: 590e063

[dimpase]

New commits:

590e063

correctly setup sagelib dependencies-trac #27254

[embray]

comment:3

I'm actually not sure this is exactly right after all:

diff --git a/build/make/Makefile.in b/build/make/Makefile.in
index eea7ceb..0bf0fcc 100644
--- a/build/make/Makefile.in
+++ b/build/make/Makefile.in
@@ -31,9 +31,9 @@ INST = $(SAGE_SPKG_INST)
 
 # Aliases for optional packages selected at configure time
 TOOLCHAIN = @SAGE_TOOLCHAIN@
-PYTHON = python@SAGE_PYTHON_VERSION@
-MP_LIBRARY = @SAGE_MP_LIBRARY@
-BLAS = @SAGE_BLAS@
+PYTHON = $(inst_python@SAGE_PYTHON_VERSION@)
+MP_LIBRARY = $(inst_@SAGE_MP_LIBRARY@)
+BLAS = $(inst_@SAGE_BLAS@)
 
 # Files to track installation of packages
 BUILT_PACKAGES = @SAGE_BUILT_PACKAGES@

the problem is that there are some places, for example, that expect the $(PYTHON) variable to be either one of the simple strings python2 or python3. Similarly with these other variables. I would actually leave these as is--not pointing to the $(inst_<pkg>) variables. In fact, if you look at the generated Makefile in build/make/Makefile, you can see that where these variables are defined the $(inst_<pkg>) variables haven't even been defined yet.

Instead, I would do like:

diff --git a/build/make/deps b/build/make/deps
index e9008d2..f0a9c0b 100644
--- a/build/make/deps
+++ b/build/make/deps
@@ -156,7 +156,7 @@ sagelib: \
                $(inst_mpc) \
                $(inst_mpfi) \
                $(inst_mpfr) \
-               $(MP_LIBRARY) \
+               $(inst_$(MP_LIBRARY)) \
                $(inst_ntl) \
                $(inst_numpy) \
                $(inst_pari) \

The problem is when this was just $(MP_LIBRARY) it expanded to something like:

sagelib: ... local/var/lib/sage/installed/mpfi-x.y.z ... mpir ... local/var/lib/sage/installed/ntl-x.y.z

and so on. In other words, for $(MP_LIBRARY) you just get the package name which is just a phony target, and hence always built. Whereas if you replace this with $(inst_$(MP_LIBRARY)) it will expand to $(inst_mpir) and in turn to local/var/lib/sage/installed/mpir-x.y.z if installing the SPKG, or to just .dummy if not.

[jdemeyer]

comment:4

Replying to @embray:

the problem is that there are some places, for example, that expect the $(PYTHON) variable to be either one of the simple strings python2 or python3.

Maybe that just means that we should have two of these variables: one for the Python executable and one for the dependency. Surely we'll need that anyway if we want to support system Python.

[embray]

comment:5

But I mean, even the package name is used as a variable alone. For example, in dependencies files I've formatted those so it's just the package name, not the path to the stamp file for that package. I suppose practically speaking that might still work, but it's still wildly inconsistent.

[jdemeyer]

comment:6

Replying to @embray:

in dependencies files I've formatted those so it's just the package name, not the path to the stamp file for that package.

But that's just for convenience. We are actually textually replacing names like pari by $(inst_pari). It should still work if you write the stamp file.

[embray]

comment:7

I agree, but I still think it's better to be consistent, and not change the current usage if it isn't necessary to. If you look at my patch above you'll see that it's sufficient (when doing the same for BLAS, etc.) to solve the problem this ticket is trying to solve.

[embray]

comment:8

To put another way, traditionally these variables have been used to represent what package was selected at configure-time to satisfy some dependency that can be satisfied by multiple packages. Having BLAS=atlas or BLAS=openblas is potentially useful information about what selection was made. If you're using the system package in particular the stamp file will be local/var/lib/sage/installed/.dummy, so now you'll have BLAS=local/var/lib/sage/installed/.dummy which tells you nothing, and it is harder to reuse the information in that variable.

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

356621a

correctly setup sagelib dependencies-trac #27254

[sagetrac-git]

Changed commit from 590e063 to 356621a

[dimpase]

Changed author from Jeroen Demeyer, Dima Pasechnik to Dima Pasechnik

[embray]

comment:12

Looks good. I'm trying to build with this now--everything seems normal though.

[embray]

Reviewer: Erik Bray

[vbraun]

Changed branch from u/dimpase/build/correct_generic_deps_t27254 to 356621a