Opened 10 years ago
Closed 5 months ago
#10202 closed enhancement (wontfix)
Use pkg-config --define-variable option to set ${SAGE_ROOT} anytime pkg-config is invoked
Reported by: | jason | Owned by: | leif |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | packages: standard | Keywords: | |
Cc: | leif, drkirkby, kcrisman, jdemeyer, jhpalmieri, dimpase | Merged in: | |
Authors: | Reviewers: | Dima Pasechnik | |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
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:
- Download a fresh sage-4.7.alpha5.tar source archive
- Extract and delete the spkg/standard/sage_scripts-4.7.alpha5.spkg
- Put http://sage.math.washington.edu/home/jason/sage_scripts-4.7.alpha5.p0.spkg into the spkg/standard/ directory
- 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?!?
Attachments (1)
Change History (42)
comment:1 Changed 10 years ago by
- Cc drkirkby kcrisman added
comment:2 Changed 10 years ago by
comment:3 follow-up: ↓ 8 Changed 10 years ago by
Oh, may it be the case that sage-location
is also called (first) during the build (through sage-sage
)?
comment:4 follow-up: ↓ 5 Changed 10 years ago by
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.
comment:5 in reply to: ↑ 4 ; follow-up: ↓ 6 Changed 10 years ago by
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.
comment:6 in reply to: ↑ 5 Changed 10 years ago by
Replying to leif:
I also suggested elsewhere to make it more verbose.
... and also to make backup copies of the original files.
comment:7 Changed 10 years ago by
- Owner changed from tbd to leif
comment:8 in reply to: ↑ 3 Changed 10 years ago by
Replying to leif:
Oh, may it be the case that
sage-location
is also called (first) during the build (throughsage-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
comment:9 Changed 10 years ago by
- Cc jdemeyer added
- Priority changed from major to critical
comment:10 follow-up: ↓ 11 Changed 10 years ago by
- Description modified (diff)
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.
comment:11 in reply to: ↑ 10 Changed 10 years ago by
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.
comment:12 Changed 10 years ago by
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?
comment:13 Changed 10 years ago by
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.
comment:14 Changed 10 years ago by
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.
comment:15 Changed 10 years ago by
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.
comment:16 Changed 10 years ago by
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.
comment:17 Changed 10 years ago by
Only one MB. Comes with glib, which is perhaps half the download?
comment:18 Changed 10 years ago by
- Description modified (diff)
- Summary changed from Make freetype have ${SAGE_LOCAL} as its prefix in the pkgconfig file to Use pkg-config --define-variable option to set ${SAGE_ROOT} anytime pkg-config is invoked
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.
comment:19 Changed 10 years ago by
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.
comment:20 Changed 10 years ago by
So what happens if the computer doesn't have pkg-config?
comment:21 Changed 10 years ago by
Nothing. Same thing that happens now.
comment:22 Changed 10 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:23 Changed 10 years ago by
- Description modified (diff)
comment:24 Changed 10 years ago by
- Reviewers set to Leif Leonhardy
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 ofSAGE_ROOT
(orSAGE_LOCAL
) into the.pc
files, which I more than once said is totally superfluous if we for example use (literally)prefix=${SAGE_ROOT}/local
:(This is fromdef 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)
local/bin/sage-location
, Sage 4.7.1.rc0.)
There we have to remove the second assignment tonew_config
. The functionupdate_pkgconfig_files()
should be deleted (or no longer used), since it just updatesSAGE_ROOT=...
definitions with a new hardcoded path forSAGE_ROOT
. - We may still have to fix a few
.pc
files in their correspondingspkg-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 definingprefix=${SAGE_ROOT}/local
(see above) is sufficient, andinitialize_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
, usingsed
, since Python is not necessarily available during the build. If we do, we no longer have to deal with.pc
files insage-location
at all.sage-spkg
already callssage-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 definedSAGE_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.
comment:25 Changed 10 years ago by
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.)
comment:26 Changed 10 years ago by
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.
comment:27 in reply to: ↑ description Changed 10 years ago by
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.
comment:28 in reply to: ↑ description Changed 10 years ago by
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...
comment:29 Changed 10 years ago by
- Status changed from needs_review to needs_info
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).
comment:30 Changed 10 years ago by
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!
comment:31 follow-up: ↓ 32 Changed 10 years ago by
It also seems like using PC_TOP_BUILD_DIR in this way goes against its purpose. That would confuse users, I think.
comment:32 in reply to: ↑ 31 Changed 10 years ago by
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.
comment:33 Changed 8 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:34 Changed 7 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:35 Changed 7 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:36 Changed 7 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:37 Changed 8 months ago by
- Cc jhpalmieri dimpase added
- Milestone changed from sage-6.4 to sage-duplicate/invalid/wontfix
- Status changed from needs_info to needs_review
This is outdated and should be closed
comment:38 Changed 8 months ago by
- Reviewers Leif Leonhardy deleted
comment:39 Changed 8 months ago by
- Priority changed from critical to major
comment:40 Changed 8 months ago by
- Reviewers set to Dima Pasechnik
- Status changed from needs_review to positive_review
comment:41 Changed 5 months ago by
- Resolution set to wontfix
- Status changed from positive_review to closed
Slightly abusing this ticket (the G
**
gle groups are less suited for markup), here's my current situation:(Note that the
egrep
patterns are a bit "redundant", to be partially also used withsed
. ;-) )So briefly, in my Sage 4.6 final (built from scratch),
gsl.pc
andpynac.pc
lack aSAGE_ROOT=...
line, whilelibpng.pc
has in addition a superfluous lineSAGE_ROOT=${SAGE_ROOT}
, andlibR.pc
lacks a definition of bothprefix
andSAGE_ROOT
(but the file is sane, i.e. contains the proper hard-coded path(s) inrhome
andrincludedir
.)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'sinitialize_pkgconfig_files()
, since it doesn't check if a.pc
file is just a link to another one (which is the case forlibpng.pc
). Testing for an already existingSAGE_ROOT=...
would of course be possible as well.