Use pkg-config --define-variable option to set ${SAGE_ROOT} anytime pkg-config is invoked

[jason]

Currently we rewrite all of the local/lib/pkgconfig/*.pc files every time we move locations. Instead, we should just use the --define-variable option of pkg-config to define a ${SAGE_ROOT} variable. Then we don't have to keep rewriting files every time.

INSTRUCTIONS FOR TESTING:

1. Download a fresh sage-4.7.alpha5.tar source archive

2. Extract and delete the spkg/standard/sage_scripts-4.7.alpha5.spkg

3. Put ​http://sage.math.washington.edu/home/jason/sage_scripts-4.7.alpha5.p0.spkg into the spkg/standard/ directory

4. Make Sage

Note to release manager: you have to edit the sage_scripts spkg-install to copy over the pkg-config shell script; see my sage_scripts spkg above.

P.S. Why is the sage_scripts spkg-install not under version control?!?

[leif]

Slightly abusing this ticket (the Ggle groups are less suited for markup), here's my current situation:

$ egrep '^prefix=.*$|^SAGE_ROOT=.*$' local/lib/pkgconfig/*.pc
local/lib/pkgconfig/bdw-gc.pc:SAGE_ROOT=/home/leif/Sage/sage-4.6
local/lib/pkgconfig/bdw-gc.pc:prefix=${SAGE_ROOT}/local
local/lib/pkgconfig/freetype2.pc:SAGE_ROOT=/home/leif/Sage/sage-4.6
local/lib/pkgconfig/freetype2.pc:prefix=${SAGE_ROOT}/local
local/lib/pkgconfig/gnutls-extra.pc:SAGE_ROOT=/home/leif/Sage/sage-4.6
local/lib/pkgconfig/gnutls-extra.pc:prefix=${SAGE_ROOT}/local
local/lib/pkgconfig/gnutls.pc:SAGE_ROOT=/home/leif/Sage/sage-4.6
local/lib/pkgconfig/gnutls.pc:prefix=${SAGE_ROOT}/local
local/lib/pkgconfig/gsl.pc:prefix=/home/leif/Sage/sage-4.6/local
local/lib/pkgconfig/libpng.pc:SAGE_ROOT=/home/leif/Sage/sage-4.6
local/lib/pkgconfig/libpng.pc:SAGE_ROOT=${SAGE_ROOT}
local/lib/pkgconfig/libpng.pc:prefix=${SAGE_ROOT}/local
local/lib/pkgconfig/libpng12.pc:SAGE_ROOT=/home/leif/Sage/sage-4.6
local/lib/pkgconfig/libpng12.pc:SAGE_ROOT=${SAGE_ROOT}
local/lib/pkgconfig/libpng12.pc:prefix=${SAGE_ROOT}/local
local/lib/pkgconfig/opencdk.pc:SAGE_ROOT=/home/leif/Sage/sage-4.6
local/lib/pkgconfig/opencdk.pc:prefix=${SAGE_ROOT}/local
local/lib/pkgconfig/pynac.pc:prefix=/home/leif/Sage/sage-4.6/local
local/lib/pkgconfig/sqlite3.pc:SAGE_ROOT=/home/leif/Sage/sage-4.6
local/lib/pkgconfig/sqlite3.pc:prefix=${SAGE_ROOT}/local
local/lib/pkgconfig/zlib.pc:SAGE_ROOT=/home/leif/Sage/sage-4.6
local/lib/pkgconfig/zlib.pc:prefix=${SAGE_ROOT}/local
$ (cd ../sage-4.6.rc0 && egrep '^prefix=.*$|^SAGE_ROOT=.*$' local/lib/pkgconfig/*.pc)
local/lib/pkgconfig/bdw-gc.pc:SAGE_ROOT=/home/leif/Sage/sage-4.6.rc0
local/lib/pkgconfig/bdw-gc.pc:prefix=${SAGE_ROOT}/local
local/lib/pkgconfig/freetype2.pc:SAGE_ROOT=/home/leif/Sage/sage-4.6.rc0
local/lib/pkgconfig/freetype2.pc:prefix=${SAGE_ROOT}/local
local/lib/pkgconfig/gnutls-extra.pc:SAGE_ROOT=/home/leif/Sage/sage-4.6.rc0
local/lib/pkgconfig/gnutls-extra.pc:prefix=${SAGE_ROOT}/local
local/lib/pkgconfig/gnutls.pc:SAGE_ROOT=/home/leif/Sage/sage-4.6.rc0
local/lib/pkgconfig/gnutls.pc:prefix=${SAGE_ROOT}/local
local/lib/pkgconfig/gsl.pc:SAGE_ROOT=/home/leif/Sage/sage-4.6.rc0
local/lib/pkgconfig/gsl.pc:prefix=${SAGE_ROOT}/local
local/lib/pkgconfig/libR.pc:SAGE_ROOT=/home/leif/Sage/sage-4.6.rc0
local/lib/pkgconfig/libpng.pc:SAGE_ROOT=/home/leif/Sage/sage-4.6.rc0
local/lib/pkgconfig/libpng.pc:SAGE_ROOT=${SAGE_ROOT}
local/lib/pkgconfig/libpng.pc:prefix=${SAGE_ROOT}/local
local/lib/pkgconfig/libpng12.pc:SAGE_ROOT=/home/leif/Sage/sage-4.6.rc0
local/lib/pkgconfig/libpng12.pc:SAGE_ROOT=${SAGE_ROOT}
local/lib/pkgconfig/libpng12.pc:prefix=${SAGE_ROOT}/local
local/lib/pkgconfig/opencdk.pc:SAGE_ROOT=/home/leif/Sage/sage-4.6.rc0
local/lib/pkgconfig/opencdk.pc:prefix=${SAGE_ROOT}/local
local/lib/pkgconfig/pynac.pc:SAGE_ROOT=/home/leif/Sage/sage-4.6.rc0
local/lib/pkgconfig/pynac.pc:prefix=${SAGE_ROOT}/local
local/lib/pkgconfig/sqlite3.pc:SAGE_ROOT=/home/leif/Sage/sage-4.6.rc0
local/lib/pkgconfig/sqlite3.pc:prefix=${SAGE_ROOT}/local
local/lib/pkgconfig/zlib.pc:SAGE_ROOT=/home/leif/Sage/sage-4.6.rc0
local/lib/pkgconfig/zlib.pc:prefix=${SAGE_ROOT}/local

(Note that the egrep patterns are a bit "redundant", to be partially also used with sed. ;-) )

So briefly, in my Sage 4.6 final (built from scratch), gsl.pc and pynac.pc lack a SAGE_ROOT=... line, while libpng.pc has in addition a superfluous line SAGE_ROOT=${SAGE_ROOT}, and libR.pc lacks a definition of both prefix and SAGE_ROOT (but the file is sane, i.e. contains the proper hard-coded path(s) in rhome and rincludedir.)

I really wonder what might have caused the differences between 4.6.rc0 and 4.6 final. Do these files get modified by other scripts than sage-location?

The redundant SAGE_ROOT=${SAGE_ROOT} is clearly a "bug" in #9210's initialize_pkgconfig_files(), since it doesn't check if a .pc file is just a link to another one (which is the case for libpng.pc). Testing for an already existing SAGE_ROOT=... would of course be possible as well.

[leif]

Oh, may it be the case that sage-location is also called (first) during the build (through sage-sage)?

[jason]

I agree that the current solution is suboptimal. If we can really use environment variables in pkgconfig files, then clearly the best place to fix the pkgconfig files is in the separate spkg-installs for the affected packages. That would be way less error-prone (for example, libpng knows about the symbolic link, so it knows exactly which file to modify). In other words, it is the responsibility of the spkg to make the package relocatable.

I think we ought to just post new versions of the right spkgs, and not modify the pkgconfig files in sage-location.

[leif]

Replying to jason:

I agree that the current solution is suboptimal. If we can really use environment variables in pkgconfig files, then clearly the best place to fix the pkgconfig files is in the separate spkg-installs for the affected packages. That would be way less error-prone (for example, libpng knows about the symbolic link, so it knows exactly which file to modify). In other words, it is the responsibility of the spkg to make the package relocatable.

Possible. Regarding the number of spkgs affected, rather "no" (not now)...

I think we ought to just post new versions of the right spkgs, and not modify the pkgconfig files in sage-location.

We should just check there if the files are already patched.

I also suggested elsewhere to make it more verbose.

[leif]

Replying to leif:

I also suggested elsewhere to make it more verbose.

... and also to make backup copies of the original files.

[leif]

Cf. ​http://trac.sagemath.org/sage_trac/ticket/10162#comment:3

[leif]

Replying to leif:

Oh, may it be the case that sage-location is also called (first) during the build (through sage-sage)?

Yep:

$ ls -rtl local/lib/pkgconfig/*.pc
lrwxrwxrwx 1 leif leif   11 Nov  1 14:21 local/lib/pkgconfig/libpng.pc -> libpng12.pc
-rw-r--r-- 1 leif leif  310 Nov  1 14:23 local/lib/pkgconfig/zlib.pc
-rw-r--r-- 1 leif leif  311 Nov  1 14:23 local/lib/pkgconfig/sqlite3.pc
-rw-r--r-- 1 leif leif  947 Nov  1 14:23 local/lib/pkgconfig/opencdk.pc
-rw-r--r-- 1 leif leif  313 Nov  1 14:23 local/lib/pkgconfig/libpng12.pc
-rw-r--r-- 1 leif leif  973 Nov  1 14:23 local/lib/pkgconfig/gnutls.pc
-rw-r--r-- 1 leif leif 1153 Nov  1 14:23 local/lib/pkgconfig/gnutls-extra.pc
-rw-r--r-- 1 leif leif  324 Nov  1 14:23 local/lib/pkgconfig/freetype2.pc
-rw-r--r-- 1 leif leif  303 Nov  1 14:23 local/lib/pkgconfig/bdw-gc.pc
-rw-r--r-- 1 leif leif  296 Nov  1 14:34 local/lib/pkgconfig/pynac.pc
-rw-r--r-- 1 leif leif  348 Nov  1 14:47 local/lib/pkgconfig/gsl.pc
-rw-r--r-- 1 leif leif  239 Nov  1 14:51 local/lib/pkgconfig/libR.pc
$ ls -l local/lib/sage-current-location.txt 
-rw-r--r-- 1 leif leif   24 Nov  1 14:23 local/lib/sage-current-location.txt

[jason]

This ticket is getting confusing. When I opened it, the goal was to modify the spkg-install file in the freetype spkg so that after installation, the freetype2.pc file would be something like:

prefix=${SAGE_LOCAL}
exec_prefix=${prefix}
libdir=${exec_prefix}/lib
includedir=${prefix}/include

Name: FreeType 2
Description: A free, high-quality, and portable font engine.
Version: 9.16.3
Requires:
Libs: -L${libdir} -lfreetype -lz 
Cflags: -I${includedir}/freetype2 -I${includedir}

I envision as the first of several spkgs that are updated to have similar changes, so that sage-location (eventually) does not have to update the pkgconfig files.

This ticket may necessitate changes in the sage-location script, as the script no longer needs to update the pkgconfig file. However, I don't think any harm is done in sage-location updating the pkgconfig file either.

[leif]

Replying to jason:

This ticket is getting confusing. When I opened it, the goal was to modify the spkg-install file in the freetype spkg [...]

I hesitated to change its title... ;-)

The modification of the freetype spkg should IMHO be addressed at #9523 as well.

I envision as the first of several spkgs that are updated to have similar changes, so that sage-location (eventually) does not have to update the pkgconfig files.

This ticket may necessitate changes in the sage-location script, as the script no longer needs to update the pkgconfig file. However, I don't think any harm is done in sage-location updating the pkgconfig file either.

The situation is much worse since the logic of patching / "initializing" the .pc files exactly once (and all at the same time) is currently completely broken.

I'll write a shell script to (re-)initialize them conditionally.

[jason]

I'm curious where we are on this. I haven't worked on it at all. leif, you were going to do something (see your last comment). I agree that the pkg files should be initialized conditionally. Did you write a script of some sort to check to see if the files had been already modified?

[jason]

This last problem (initializing pc files only once) might be causing a problem behind the scenes:

$ pkg-config --cflags libpng
Duplicate definition of variable 'SAGE_ROOT' in '/Users/grout/sage/local/lib/pkgconfig/libpng.pc'

So I'm not sure that our pkg-config works right now. At least pkg-config --exists libpng still works.

[jason]

One other note. I'm not sure that the basic idea of having ${SAGE_LOCAL} as a variable inside the .pc files works. I just changed libpng.pc to not define ${SAGE_ROOT}, but hopefully just take it out of the environment. However, when I run pkg-config, it says:

$ pkg-config --cflags libpng
Variable 'SAGE_ROOT' not defined in '/Users/grout/sage/local/lib/pkgconfig/libpng.pc'

So I think the main idea of this ticket is doomed.

[jason]

I take that back about this ticket being doomed. In fact, the solution is *much* easier. Instead of defining ${SAGE_ROOT} at the top of pkg-config, we should wrap the system pkg-config using the --define-variable option:

       --define-variable=VARIABLENAME=VARIABLEVALUE
              This sets a global value for a variable, overriding the value in any files. Most packages define the vari-
              able "prefix", for example, so you can say:
                $ pkg-config --print-errors --define-variable=prefix=/foo \
                             --variable=prefix glib-2.0
                /foo

So if we have a pkg-config in $SAGE_ROOT/local/bin which is something like:

#!/bin/sh
pkg-config --define-variable=SAGE_ROOT=<whatever> $*

then we don't have to keep mucking with the .pc files every time our location moves. We just need to do a simple string substitution the first time Sage is built or the first time any package makes a .pc file.

This solves both the main problem of this ticket and the problem with double-initialization.

[jason]

Another very automatic solution for us: patch pkg-config:

​http://lists.freedesktop.org/archives/pkg-config/2010-April/000531.html

The advantage to patching and distributing our own pkg-config is that then we can count on pkg-config, even on osx systems that don't come with it by default.

[kcrisman]

Only one MB. Comes with glib, which is perhaps half the download?

[jason]

apply to sage scripts repository

[jason]

Actually, that path to pkg-config won't solve our problem, since it involves modifying each of the pkg-config files anyway, and then things depend on the package, etc. So I don't think anymore that it would be better to ship pkg-config.

I changed the description to reflect the patch's new solution.

[jason]

I'm going to set this to "needs review" as it needs testing on a lot of systems. My guess is that it needs to be tested by making a new sage_scripts spkg and then a system needs to be built fresh with that new spkg.

[kcrisman]

So what happens if the computer doesn't have pkg-config?

[jason]

Nothing. Same thing that happens now.

[leif]

Finally (and again) returning to this...

• We don't need to "wrap" pkg-config (and if we would, one should use "$@" instead of $*).
• We don't have to --define-variable=SAGE_ROOT=${SAGE_ROOT}, provided we don't put any [hardcoded] definitions of SAGE_ROOT (or SAGE_LOCAL) into the .pc files, which I more than once said is totally superfluous if we for example use (literally) prefix=${SAGE_ROOT}/local:

  def initialize_pkgconfig_files():
      """
      Insert a sage_local variable in each pkg_config file and replace
      them to make the paths portable.
      """
      LIB = os.path.join(os.path.abspath(SAGE_ROOT), 'local', 'lib')
      PKG = os.path.join(LIB,'pkgconfig')
      for name in os.listdir(PKG):
          filename=os.path.join(PKG,name)
          if os.path.splitext(filename)[1]==".pc":
              with open(filename) as file:
                  config = file.read()
  
              new_config = config.replace(os.path.abspath(SAGE_ROOT), "${SAGE_ROOT}")
  
              new_config = 'SAGE_ROOT=%s\n'%os.path.abspath(SAGE_ROOT)+new_config
  
              with open(filename, 'w') as file:
                  file.write(new_config)
  

  (This is from local/bin/sage-location, Sage 4.7.1.rc0.)
  There we have to remove the second assignment to new_config. The function update_pkgconfig_files() should be deleted (or no longer used), since it just updates SAGE_ROOT=... definitions with a new hardcoded path for SAGE_ROOT.
• We may still have to fix a few .pc files in their corresponding spkg-install files. I did this for R / libR.pc (cf. #9668) in a new r-2.10.1.p6 spkg (which I haven't yet uploaded though). For most .pc files defining prefix=${SAGE_ROOT}/local (see above) is sufficient, and initialize_pkgconfig_files() does this, although it could do better by explicitly looking for ^prefix=.... (Also, we could do the same -- more reliably -- from some shell script, e.g. sage-spkg, using sed, since Python is not necessarily available during the build. If we do, we no longer have to deal with .pc files in sage-location at all. sage-spkg already calls sage-make_relative after every spkg installation, so this would fit well.)

Jason's new patch to sage-location already does

• remove the second assignment to new_config, which defined SAGE_ROOT to the current $SAGE_ROOT (i.e., a hardcoded path),
• remove the no longer necessary function update_pkgconfig_files().

(The note on) using --define-variable=SAGE_ROOT=${SAGE_ROOT} is not necessary though, see above.

No offense btw.

[leif]

P.S.: I currently have no idea where the shell script I wrote is... 8/ (IIRC it consisted of more than a single sed line, to be more robust and address some corner cases.)

[leif]

Unfortunately my reasoning is now spread across a few tickets... 8/

First of all, we don't have to wrap pkg-config, which would not necessarily be available during the whole build (including upgrades), and doing so might confuse [dumb] build scripts testing for its presence on systems that lack a "real" pkg-config.

I now seem to recall why using ${SAGE_ROOT} without explicitly defining it in the .pc files "worked for me" last year -- I was using $${SAGE_ROOT}, which postpones expansion to the shell, like it does in Makefiles.

But there is an even safer way (cf. this comment at #11687), namely to define the environment variable PKG_CONFIG_TOP_BUILD_DIR in sage-env and use the corresponding .pc-file variable pc_top_builddir (instead of SAGE_ROOT) in all .pc files, e.g.

# We set PKG_CONFIG_TOP_BUILD_DIR to $SAGE_ROOT in sage-env,
# such that ${pc_top_builddir} always yields this.
prefix=${pc_top_builddir}/local
...
# All further directory definitions are relative to ${pc_top_builddir}
# or ${prefix}.

Substitutions in ("initialization" of) .pc files should IMHO be performed from or better directly within sage-spkg (or an spkg's spkg-install if necessary), of course not "fixing" individual files more than once. We then can drop the pkg-config-related code in sage-location completely, or otherwise have to make sure it is consistent with the other one, i.e. doesn't mess things up afterwards (as it currently does).

We also have to fix setting PKG_CONFIG_PATH in sage-env; see #11687.

[jdemeyer]

Replying to jason:

Note to release manager: you have to edit the sage_scripts spkg-install to copy over the pkg-config shell script; see my sage_scripts spkg above.

I will certainly not do this. I need a simply hg patch which I can apply.

[jdemeyer]

Replying to jason:

Why is the sage_scripts spkg-install not under version control?!?

Inside the spkg, that file is sage-spkg-install which is under revision control. Don't ask me why though...

[leif]

This ticket IMHO needs work anyway (see above), and its title should be changed (or I can open another one with a different approach as pointed out above).

[jason]

The one disadvantage I can see to the PC_TOP_BUILD_DIR solution is that if the user is already using that for some other purpose in their system-installed packages. Using $${SAGE_ROOT} instead avoids this problem.

At any rate, I'm glad you're looking into this so carefully!

[jason]

It also seems like using PC_TOP_BUILD_DIR in this way goes against its purpose. That would confuse users, I think.

[leif]

Replying to jason:

It also seems like using PC_TOP_BUILD_DIR in this way goes against its purpose. That would confuse users, I think.

Well, it's just another situation where its application is useful (and which is quite similar to its original intent). The way Sage uses its own prefix / subtree of typical system directories ($SAGE_ROOT/local/{bin,lib,include}) is just not very common, so the developer of pkg-config didn't have it in mind.

To make it less obscure (to people actually looking at the .pc files), we could use

SAGE_ROOT=${pc_top_builddir} # the latter is set "dynamically" by sage-env
prefix=${SAGE_ROOT}/local
...
# (further references to SAGE_ROOT rather than pc_top_builddir)

(Adding comments to the .pc files isn't bad anyway; I used to also add a comment telling when the file was [last] modified by Sage, e.g. for debugging.)

W.r.t. users already using that variable (pc_top_builddir) for other purposes:
I don't think anybody will have it in his/her system-wide .pc files, but at least in case PKG_CONFIG_TOP_BUILD_DIR is already set, we could issue a warning.

Using $${SAGE_ROOT} instead isn't that safe, since programs using parameters supplied by pkg-config do not necessarily use the shell to execute commands with these parameters (though they usually do; I'm currently not aware of any counter-example), in which case ${SAGE_ROOT}/... would get passed verbatim to the commands, e.g. to the compiler or the linker.