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: sage-8.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:

Status badges

Description (last modified by 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)) \

Change History (14)

comment:1 Changed 3 years ago by dimpase

  • Description modified (diff)

comment:2 Changed 3 years ago by dimpase

  • Branch set to u/dimpase/build/correct_generic_deps_t27254
  • Commit set to 590e063fe1acaa7f2972787ecf04dd6adef1f07a
  • Status changed from new to needs_review

New commits:

590e063correctly setup sagelib dependencies-trac #27254

comment:3 follow-up: Changed 3 years ago by embray

  • 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) 
    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 3 years ago by embray (previous) (diff)

comment:4 in reply to: ↑ 3 Changed 3 years ago by jdemeyer

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 follow-up: Changed 3 years ago by embray

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 jdemeyer

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 embray

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 embray

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 3 years ago by embray (previous) (diff)

comment:9 Changed 3 years ago by git

  • Commit changed from 590e063fe1acaa7f2972787ecf04dd6adef1f07a to 356621a31567d2d1f5fe5bcf6eacbd31f979ace0

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 3 years ago by dimpase

  • Description modified (diff)
  • Status changed from needs_info to needs_review

comment:11 Changed 3 years ago by dimpase

  • Authors changed from Jeroen Demeyer, Dima Pasechnik to Dima Pasechnik

comment:12 Changed 3 years ago by embray

  • Reviewers set to Erik Bray

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

comment:13 Changed 3 years ago by embray

  • Status changed from needs_review to positive_review

comment:14 Changed 3 years ago by vbraun

  • Branch changed from u/dimpase/build/correct_generic_deps_t27254 to 356621a31567d2d1f5fe5bcf6eacbd31f979ace0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.