Opened 4 years ago

Closed 4 years ago

#27254 closed defect (fixed)

MP_LIBRARY, BLAS, PYTHON need inst_ in Makefile.in

Reported by: Dima Pasechnik Owned by:
Priority: major Milestone: sage-8.7
Component: build Keywords:
Cc: Erik Bray, Jeroen Demeyer Merged in:
Authors: Dima Pasechnik Reviewers: Erik Bray
Report Upstream: N/A Work issues:
Branch: 356621a (Commits, GitHub, GitLab) Commit: 356621a31567d2d1f5fe5bcf6eacbd31f979ace0
Dependencies: Stopgaps:

Status badges

Description (last modified by Dima Pasechnik)

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 4 years ago by Dima Pasechnik

Description: modified (diff)

comment:2 Changed 4 years ago by Dima Pasechnik

Branch: u/dimpase/build/correct_generic_deps_t27254
Commit: 590e063fe1acaa7f2972787ecf04dd6adef1f07a
Status: newneeds_review

New commits:

590e063correctly setup sagelib dependencies-trac #27254

comment:3 Changed 4 years ago by Erik Bray

Status: needs_reviewneeds_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) 
    3131
    3232# Aliases for optional packages selected at configure time
    3333TOOLCHAIN = @SAGE_TOOLCHAIN@
    34 PYTHON = python@SAGE_PYTHON_VERSION@
    35 MP_LIBRARY = @SAGE_MP_LIBRARY@
    36 BLAS = @SAGE_BLAS@
     34PYTHON = $(inst_python@SAGE_PYTHON_VERSION@)
     35MP_LIBRARY = $(inst_@SAGE_MP_LIBRARY@)
     36BLAS = $(inst_@SAGE_BLAS@)
    3737
    3838# Files to track installation of packages
    3939BUILT_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:

  • build/make/deps

    diff --git a/build/make/deps b/build/make/deps
    index e9008d2..f0a9c0b 100644
    a b sagelib: \ 
    156156               $(inst_mpc) \
    157157               $(inst_mpfi) \
    158158               $(inst_mpfr) \
    159                $(MP_LIBRARY) \
     159               $(inst_$(MP_LIBRARY)) \
    160160               $(inst_ntl) \
    161161               $(inst_numpy) \
    162162               $(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.

Last edited 4 years ago by Erik Bray (previous) (diff)

comment:4 in reply to:  3 Changed 4 years ago by Jeroen Demeyer

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.

comment:5 Changed 4 years ago by Erik Bray

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 4 years ago by Jeroen Demeyer

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 4 years ago by Erik Bray

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 4 years ago by Erik Bray

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.

Last edited 4 years ago by Erik Bray (previous) (diff)

comment:9 Changed 4 years ago by git

Commit: 590e063fe1acaa7f2972787ecf04dd6adef1f07a356621a31567d2d1f5fe5bcf6eacbd31f979ace0

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

356621acorrectly setup sagelib dependencies-trac #27254

comment:10 Changed 4 years ago by Dima Pasechnik

Description: modified (diff)
Status: needs_infoneeds_review

comment:11 Changed 4 years ago by Dima Pasechnik

Authors: Jeroen Demeyer, Dima PasechnikDima Pasechnik

comment:12 Changed 4 years ago by Erik Bray

Reviewers: Erik Bray

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

comment:13 Changed 4 years ago by Erik Bray

Status: needs_reviewpositive_review

comment:14 Changed 4 years ago by Volker Braun

Branch: u/dimpase/build/correct_generic_deps_t27254356621a31567d2d1f5fe5bcf6eacbd31f979ace0
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.