Opened 3 years ago
Closed 3 years ago
#27254 closed defect (fixed)
MP_LIBRARY, BLAS, PYTHON need inst_ in Makefile.in
Reported by:  dimpase  Owned by:  

Priority:  major  Milestone:  sage8.7 
Component:  build  Keywords:  
Cc:  embray, jdemeyer  Merged in:  
Authors:  Dima Pasechnik  Reviewers:  Erik Bray 
Report Upstream:  N/A  Work issues:  
Branch:  356621a (Commits, GitHub, GitLab)  Commit:  356621a31567d2d1f5fe5bcf6eacbd31f979ace0 
Dependencies:  Stopgaps: 
Description (last modified by )
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)) \
Change History (14)
comment:1 Changed 3 years ago by
 Description modified (diff)
comment:2 Changed 3 years ago by
 Branch set to u/dimpase/build/correct_generic_deps_t27254
 Commit set to 590e063fe1acaa7f2972787ecf04dd6adef1f07a
 Status changed from new to needs_review
comment:3 followup: ↓ 4 Changed 3 years ago by
 Status changed from needs_review to needs_info
I'm actually not sure this is exactly right after all:

build/make/Makefile.in
diff git a/build/make/Makefile.in b/build/make/Makefile.in index eea7ceb..0bf0fcc 100644
a b INST = $(SAGE_SPKG_INST) 31 31 32 32 # Aliases for optional packages selected at configure time 33 33 TOOLCHAIN = @SAGE_TOOLCHAIN@ 34 PYTHON = python@SAGE_PYTHON_VERSION@35 MP_LIBRARY = @SAGE_MP_LIBRARY@36 BLAS = @SAGE_BLAS@34 PYTHON = $(inst_python@SAGE_PYTHON_VERSION@) 35 MP_LIBRARY = $(inst_@SAGE_MP_LIBRARY@) 36 BLAS = $(inst_@SAGE_BLAS@) 37 37 38 38 # Files to track installation of packages 39 39 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 isnot 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:

build/make/deps
diff git a/build/make/deps b/build/make/deps index e9008d2..f0a9c0b 100644
a b sagelib: \ 156 156 $(inst_mpc) \ 157 157 $(inst_mpfi) \ 158 158 $(inst_mpfr) \ 159 $( MP_LIBRARY) \159 $(inst_$(MP_LIBRARY)) \ 160 160 $(inst_ntl) \ 161 161 $(inst_numpy) \ 162 162 $(inst_pari) \
The problem is when this was just $(MP_LIBRARY)
it expanded to something like:
sagelib: ... local/var/lib/sage/installed/mpfix.y.z ... mpir ... local/var/lib/sage/installed/ntlx.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/mpirx.y.z
if installing the SPKG, or to just .dummy
if not.
comment:4 in reply to: ↑ 3 Changed 3 years ago by
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 stringspython2
orpython3
.
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.
comment:5 followup: ↓ 6 Changed 3 years ago by
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.
comment:6 in reply to: ↑ 5 Changed 3 years ago by
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.
comment:7 Changed 3 years ago by
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.
comment:8 Changed 3 years ago by
To put another way, traditionally these variables have been used to represent what package was selected at configuretime 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.
comment:9 Changed 3 years ago by
 Commit changed from 590e063fe1acaa7f2972787ecf04dd6adef1f07a to 356621a31567d2d1f5fe5bcf6eacbd31f979ace0
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
356621a  correctly setup sagelib dependenciestrac #27254

comment:10 Changed 3 years ago by
 Description modified (diff)
 Status changed from needs_info to needs_review
comment:11 Changed 3 years ago by
comment:12 Changed 3 years ago by
 Reviewers set to Erik Bray
Looks good. I'm trying to build with this noweverything seems normal though.
comment:13 Changed 3 years ago by
 Status changed from needs_review to positive_review
comment:14 Changed 3 years ago by
 Branch changed from u/dimpase/build/correct_generic_deps_t27254 to 356621a31567d2d1f5fe5bcf6eacbd31f979ace0
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
correctly setup sagelib dependenciestrac #27254